Zuu wrote:

Related task: #3411: "focus of "found town" windows in scenario editor " - http://bugs.openttd.org/task/3411

Especially the comment by Rubidium.

This comment was imported from FlySpray: https://bugs.openttd.org/task/3468#comment7200

Alberth wrote:

Patch file is empty

This comment was imported from FlySpray: https://bugs.openttd.org/task/3468#comment7201

Zuu wrote:

Thats really smart of me :-)

Here is a non-empty one (I hope).

Attachments

• edit_box_focus_key_control-r18684.patch (2.63 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/3468#comment7202

Alberth wrote:

Yes, it is non-empty :)

The biggest problem of this patch is that it is completely useless at this time, since we don't have a window with more than one edit box.

The patch itself is also not yet optimal, see below

+void Window::FocusNextEditWidget()
+{
+ /* Start looking for next edit box on the next widget index or first if no widget has focus /
+ const uint start_find_at = this->nested_focus == NULL? 0 : this->nested_focus->index + 1;
+
+ / If there is no focused widget, one more widget has to be searched */
+ const uint num_loops = this->nested_focus == NULL? this->nested_array_size : this->nested_array_size - 1;

Why not one if ?

+ for (uint i = 0; i < num_loops; i++) {
+ const uint widget_index = (start_find_at + i) % this->nested_array_size;

Mod computations (the '%') are expensive, why not increment widget_index, and wrap around when widget_index == this->nested_array_size ?

+ const NWidgetCore *widget = this->GetWidget(widget_index);

This will cause a crash for non NWidgetCore widgets. Most likely candidate is the NWidgetStacked widget, used in a window with a shading box.

+ /* Focus next edit box */
+ if (widget != NULL && widget->type == WWT_EDITBOX) {
+ this->SetFocusedWidget(widget_index);
+ return;

I'd use 'break;' but perhaps that is a personal style matter.

+ }
+ }
+}
+
+/**
* Sets the enabled/disabled status of a list of widgets.
* By default, widgets are enabled.
* On certain conditions, they have to be disabled.
@@ -1919,7 +1942,20 @@

Window *w;
FOR_ALL_WINDOWS_FROM_FRONT(w) {
if (w->window_class == WC_MAIN_TOOLBAR) continue;
- if (w->OnKeyPress(key, keycode) == Window::ES_HANDLED) return;
+ if (_focused_window == w) {
+ switch (keycode) {
+ case WKC_CTRL | WKC_TAB:
+ w->FocusNextEditWidget();
+ break;
+
+ default:
+ if (w->OnKeyPress(key, keycode) == Window::ES_HANDLED) return;
+ }

+ }
+ else
+ {

Coding style!!

+ if (w->OnKeyPress(key, keycode) == Window::ES_HANDLED) return;
+ }
}

This is highly confusing. Why do it here?

As I see it, if ctl+tab is used for switching, no other window will handle it, right?
Therefore, all windows before _focused_window will fail.

Also, when encountering the right window, and ctl+tab is pressed, you act on it, and then continue. Wouldn't the key press be considered handled then?
Note that under the above assumption (ctl+tab is used for switching), all windows after _focused_window will also fail.

Last but not least, duplicating 'if (w->OnKeyPress(key, keycode) == Window::ES_HANDLED) return;' seems undesirable.
Perhaps the issue will resolve itself after fixing the above.

This comment was imported from FlySpray: https://bugs.openttd.org/task/3468#comment7205

Zuu wrote:

I don't think the patch is useless before there is more edit boxes on the same window. It is true that it has been written to be future safe for when that is the case. Right now it can be used to focus the only edit box in a window when it is not focused. While many windows has code in their constructors to give focus to the edit box, that is not mandatory. I think there shouldn't be too many windows that or we will get problems with the hotkeys. The idea with this patch is to provide a standardized way to give focus to the edit boxes so that each window do not has to implement their own hot keys for that or give focus to the edit box by default.

In the filter sign list patch I have for example used the f-key to focus the edit box, but if each window will start to use their own keys for that, then it will become really messy.

I have broken apart your comment and numbered the parts for easier future discussion. My replies start with a '->'.

I have not worked with the code in the patch yet, but I have chosen to answer your questions first and then I'll start fixing the code.

----

+void Window::FocusNextEditWidget()
+{
+ /* Start looking for next edit box on the next widget index or first if no widget has focus /
+ const uint start_find_at = this->nested_focus == NULL? 0 : this->nested_focus->index + 1;
+
+ / If there is no focused widget, one more widget has to be searched */
+ const uint num_loops = this->nested_focus == NULL? this->nested_array_size : this->nested_array_size - 1;

Why not one if ?

-> Because this code is shorter and can make use of const. I can change it if you want, but that was my reason of making it like this.

-----

+ for (uint i = 0; i < num_loops; i++) {
+ const uint widget_index = (start_find_at + i) % this->nested_array_size;

Mod computations (the '%') are expensive, why not increment widget_index, and wrap around when widget_index == this->nested_array_size ?

-> Didn't knew they was that expansive especially in the light that this function is only used when a user hit a keyboard combination. The function would be done already when the user releases his/her key. But your suggestion sounds quite okay.

-----

+ const NWidgetCore *widget = this->GetWidget(widget_index);

This will cause a crash for non NWidgetCore widgets. Most likely candidate is the NWidgetStacked widget, used in a window with a shading box.

-> Why will this crash? I looked at the implementation of GetWidget, it can in some cases return NULL, but that is checked for further down.

-----

+ /* Focus next edit box */
+ if (widget != NULL && widget->type == WWT_EDITBOX) {
+ this->SetFocusedWidget(widget_index);
+ return;

I'd use 'break;' but perhaps that is a personal style matter.

-> Hmm, yep 'break;' could work here now that the function does no longer contain any code after the for-loop. (I previously had code after the for-loop, but now the early return isn't needed anymore) Good spotting.

-----

..
..

Coding style!!

-> Yep the } else { isn't correct there.

-----

+ if (w->OnKeyPress(key, keycode) == Window::ES_HANDLED) return;
+ }
}

This is highly confusing. Why do it here?

-> Do you mean the entire ctrl+tab handling or just the duplicate of the OnKeyPress call?

-> It has to be done one before the virtual OnKeyPress function is called. I could make a non-virtual Window:: function that is called, which encapsulates the ctrl+tab checking and either calls OnKeyPress or return a boolean whether it has handled the input or not. I agree that it should return when the ctrl+tab key has been handled.

This comment was imported from FlySpray: https://bugs.openttd.org/task/3468#comment7207

Zuu wrote:

Updated patch

1: Changed
2: Changed
3: Changed
4: Changed
5 & 6: Moved the check to before the for-loop. Since it only execute the code for focused windows and there is a pointer for the focused window it is quite silly to do it in the loop.

Attachments

• edit_box_focus_key_control-2-r18684.patch (2.45 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/3468#comment7208

Zuu wrote:

A patch that actually compiles. (has also made my homework and tested the resulting binary that it seems to work)

Attachments

• edit_box_focus_key_control-3-r18684.patch (2.44 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/3468#comment7209

Alberth wrote:

uploaded review comment

Attachments

• edit_box_focus_key_control-3_comment.txt (5.49 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/3468#comment7216

Zuu wrote:

Thanks for the review.

I've understood that the ctrl+tab part of the patch will not be accepted for now. For the sake of prettier code, I've attempted to fix the issues you have put forward. I've split the patch into two pieces so that the escape part can be included if you want it. As I've understood there is not worth for me to push this patch further for now. I'll focus on patches that add edit boxes that does not fit to be focused by default so that this will become an issue later on and then this patch can be revisited. ;-)

I've made an attempt to fix the issues you pointed out. For 5. I couldn't see a good way to remove the independent 'i' index in the for loop after having removed 'start_find_at'.

Regarding the usefulness of the patch etc.

8b: Okay, future safe is not an argument

8c: We don't have a key for users to focus the edit box. We have code for focusing most edit boxes by default, but if the edit box loses focus, then the user can't get it back using the keyboard.

8d: Addressed in my initial comment above.

Attachments

• edit_box_focus_key_control-4-r18684.patch (1.81 KiB)
• edit_box_esc_unfocus-4.patch (1.15 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/3468#comment7217

andythenorth closed the ticket.

Reason for closing: Won't implement

Mass closure of patch tickets with no commentary for >5 years. Goal is to reduce patch queue as an experiment to see if it aids faster reviewing and rejection/acceptance (it may not). If this offends you and the patch is maintained and compiles with current trunk, discuss with andythenorth in irc. (andythenorth has no ability to review patches but can re-open tickets).

This comment was imported from FlySpray: https://bugs.openttd.org/task/3468