OpenTTD

Tasklist

FS#3468 - Cycle focused edit boxes using keyboard + escape = unfocus

Attached to Project: OpenTTD
Opened by Leif Linse (Zuu) - Saturday, 02 January 2010, 01:09 GMT
Last edited by andythenorth (andythenorth) - Monday, 14 August 2017, 20:27 GMT
Type Patch
Category Interface
Status Closed
Assigned To andythenorth (andythenorth)
Operating System All
Severity Low
Priority Normal
Reported Version trunk
Due in Version Undecided
Due Date Undecided
Percent Complete 0%
Votes 0
Private No

Details

This patch adds

* Pressing Ctrl + Tab key cycles the focus between the edit boxes in the focused window. If no edit box has focus, the first one gets focused.
* Pressing Escape key when an edit box has focus unfocuses it.

These two things are two code-wise unrelated features, but since I think they relate to each other in the way we want focusing to behave they are in the same patch file. Also the full patch is fairly small.


The patch does not remove the code of the fund town window that unfocus the edit widget when you hit escape. This has to be done before the patch is done, but before that I would be happy to get feedback on the patch.
This task depends upon

Closed by  andythenorth (andythenorth)
Monday, 14 August 2017, 20:27 GMT
Reason for closing:  Won't implement
Additional comments about closing:  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).
Comment by Leif Linse (Zuu) - Saturday, 02 January 2010, 10:32 GMT
Related task:  FS#3411 : "focus of "found town" windows in scenario editor " - /task/3411

Especially the comment by Rubidium.
Comment by Alberth (Alberth) - Saturday, 02 January 2010, 10:45 GMT
Patch file is empty
Comment by Leif Linse (Zuu) - Saturday, 02 January 2010, 10:50 GMT
Thats really smart of me :-)

Here is a non-empty one (I hope).
Comment by Alberth (Alberth) - Saturday, 02 January 2010, 11:47 GMT
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<NWidgetCore>(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.
Comment by Leif Linse (Zuu) - Saturday, 02 January 2010, 13:05 GMT
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.

----

1.

+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.


-----

2.

+ 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.

-----

3.

+ const NWidgetCore *widget = this->GetWidget<NWidgetCore>(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.

-----

4.

+ /* 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.


-----

5.

..
..

Coding style!!

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

-----

6.

+ 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.
Comment by Leif Linse (Zuu) - Saturday, 02 January 2010, 13:51 GMT
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.
Comment by Leif Linse (Zuu) - Saturday, 02 January 2010, 16:04 GMT
A patch that actually compiles. (has also made my homework and tested the resulting binary that it seems to work)
Comment by Alberth (Alberth) - Sunday, 03 January 2010, 09:15 GMT
uploaded review comment
Comment by Leif Linse (Zuu) - Sunday, 03 January 2010, 12:53 GMT
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.

Loading...