FS#2297 - Window & widet focus -> many windows with edit box can be open at the same time

Attached to Project: OpenTTD
Opened by Leif Linse (Zuu) - Saturday, 13 September 2008, 19:20 GMT
Last edited by Remko Bijker (Rubidium) - Monday, 09 February 2009, 01:23 GMT
Type Work in progress
Category Interface
Status Closed
Assigned To No-one
Operating System All
Severity Low
Priority Normal
Reported Version trunk
Due in Version Undecided
Due Date Undecided
Percent Complete 100%
Votes 0
Private No


At global scope:
* Keeps track of focused window

At window scope:
* Keeps track of focused widget

Used for:
* If the focused window has a focused edit box then
-> give all key input to that window
-> don't allow arrow keys to be used to scroll main viewport

* Only draw the caret of edit boxes when they have focus (so the user can see which edit widget that has focus)
* Invalidate widget that loses focus (so the caret get removed from edit boxes when they lose focus)

* In the constructor of the existing windows that has a edit box I set this->focused_widget to the edit box so users can start type directly as in current trunk.
* Focus is changed when user clicks on any part of any window.
* I've relaxed the amount of closed query windows because of another query window is to be opened a little so you can test this. (In trunk all other query windows are closed when a new query window is opened) You can now for example have a sign edit window and the save dialog open at the same time, but not several query string windows as that become problematic.
* This patch does not lift the limitation of only one edit box per window, through it fixes one reason for this limitation.
* The main gain with this patch is that more windows can use edit boxes without having to limit what windows can be open at the same time to much.
* This patch is mutual exclusive to dynamic key focus stealing patch (, as this one takes another approach to the problem.

WIP = I will probably find things that can be commented better or simplified. But the functionality is there.
This task depends upon

Closed by  Remko Bijker (Rubidium)
Monday, 09 February 2009, 01:23 GMT
Reason for closing:  Implemented
Additional comments about closing:  In r15424. Seems fine to me; but I can't tell what bugs others might find.
Comment by Leif Linse (Zuu) - Saturday, 13 September 2008, 19:30 GMT
To clarify, when there is no focused edit box on the focused window, keys are handled as normal by asking window after window to handle the input until one window returns that they have handled the input.
Comment by Leif Linse (Zuu) - Tuesday, 30 September 2008, 21:12 GMT
+ Added enum for the result of the HandleEditBoxKey function. Previously in trunk 0, 1, 2 was used as return values. This patch add return value 3 when the EditBox does not has focus.

+ Network message window and other windows raised an assert if HandleEditBoxKey returned 3 = edit box not focused. This is now fixed.

+ Use pointer arithmetic instead of loop in SetFocusedWindow function. (Thanks to peter1138 for reminding me about pointer arithmetics)

If you want to have network message window in single player go to main_gui.cpp, row 334 and add following code just after that row:

ShowNetworkChatQueryWindow(DESTTYPE_BROADCAST, 0);

The attached patch file is for r14420.
Comment by Leif Linse (Zuu) - Tuesday, 30 September 2008, 21:36 GMT
Updated for r14423 - many variable names/types was renamed in r14421 - r14423. So this is only an update to address that, no changes to the patch itself.
Comment by Leif Linse (Zuu) - Monday, 06 October 2008, 20:38 GMT
I've read through the entire patch-file and fixed the issues I have found. Also added a small feature that I think make this patch less annoying.

* Removed a code line from the patch that was commented out. - Just silly to add commented out code :)

* Added a comment to HandleKeyScrolling function where the if-statement has been changed.

* Feature: When clicking on the caption bar of a window the focused widget variable of that window is not changed. So that you can move a window without unfocusing the edit-box.
Comment by Leif Linse (Zuu) - Monday, 06 October 2008, 21:37 GMT
The comment on row 12 in querystring_gui.h was wrong.

I am working on a documentation of the code that can be of help when reviewing the code. Including what things I think are most questionable so the review can start there.
Comment by Leif Linse (Zuu) - Sunday, 12 October 2008, 20:51 GMT
When writing on the documentation of this patch I've listed some arguments for and against some of the decisions making me realize that the focused_widget variable should most likely be pointing to a const widget instead of a non-const widget as before. - So I've changed that in this version which simplifies the code little at one place.

Also found two more lines I've commented out and removed them.
Comment by Leif Linse (Zuu) - Tuesday, 21 October 2008, 09:24 GMT
Change in version 11 of this patch:
* Found and fixed a place where I've forgotten to change '0' to 'HEBR_DEFAULT' in a case statement.

I have over the last weeks produced a text document that describes the patch which is attached to this post as "WidgetFocusDoc.pdf". The patch document contains:

1. Background
1.1 Definitions
1.2 Aim
1.3 What is it not
2. Limitations in trunk
3. Solution
3.1 Variables
3.2 Implementation
4. Alternative solutions
5. Suggestions on what to be looked on when reviewing this patch
Comment by Leif Linse (Zuu) - Tuesday, 21 October 2008, 09:35 GMT
Change in version 12 of this patch:
* Updated to trunk r14505, and resolved two conflicts of which all was just changes to code close to each other.

No changes are needed for the WidgetFocusDoc.pdf, so while it still states version 11 of the patch it is compatible with version 12 of the patch also.
Comment by Leif Linse (Zuu) - Tuesday, 21 October 2008, 13:53 GMT
On my Todo for the widget focus patch is to make the on screen keyboard (OSK) window less annoying:
* Close the OSK window when text edit looses focus
* Only open the OSK window if text edit had focus before it was clicked. (so you can click on a text edit to give it focus without having the OSK window opened)

Also I've found and fixed a few spaces in the end of the line in some files. (not yet uploaded as I think it is quite a minor thing, given that the OSK window annoyance need to be addressed)
Comment by Leif Linse (Zuu) - Tuesday, 21 October 2008, 20:37 GMT
* Hunted down and removed some more spaces in the end of lines.

* In trunk each window that contains a text edit needs to call a function to open the OSK window when the text edit is clicked. This requirement I have removed by storing the necessary parameters in QueryStringBaseWindow and in DispatchLeftClickEvent in window.cpp check if a edit box is clicked and if so open the OSK for that edit box widget.

This part I have made a separate patch of as it so far works for trunk:

In addition to above I make sure that the clicked edit box had focus previously to being clicked so that you can click ones to give an edit box focus, and to show the OSK you have to click once more. Another thing is that when focus is changing from an edit box to another widget or another window the OSK window is closed.
Comment by Leif Linse (Zuu) - Tuesday, 21 October 2008, 23:02 GMT
Fixed if-statements that did not confirm with the coding style - thanks to SmartZ for notifying me of not confirming with the coding styles.
Comment by Leif Linse (Zuu) - Saturday, 25 October 2008, 13:21 GMT
* Updated to contain all changes from UnifiedOpenOSK patch. (It still applies against clean trunk)

* Coding style: Check pointers against NULL instead of 0.

* Fixes a few warning messages from gcc reported by planetmaker

* Version 15 has been used internally by me, so that is why this is version 16 and not 15.

* Version 14 had a debug change included by mistake (possibility to write chat messages in single player)
Comment by Leif Linse (Zuu) - Saturday, 25 October 2008, 21:36 GMT
Updated since FS2382 (the HandleEditBox return value enum added by this patch) has been merged with trunk.

This version also contains a change in behavior:
* When a new window is opened it will not get focused if globally focused widget is an edit box. This prevents the economy window and other popup windows to steal focus while typing.
Comment by Leif Linse (Zuu) - Saturday, 03 January 2009, 19:09 GMT
* Updated to trunk. (Unified Open OSK patch was committed to trunk at r14804 by Rubidium. Also _no_scroll behavior has changed in trunk, so removal of _no_scroll needed updating)

* Clicking on main toolbar or bottom toolbar does no longer give clicked toolbar focus. (this has been implemented in filter_sign_list patch but have for some reason not been included in widget_focus patch)

* Clicking on the 'X' (close button) does no longer give focus to the clicked window. Makes it possible to close windows without loosing input focus of a text box in another window.

* Documented 5 functions that in version 17 had no function documentation. (all 5 functions are introduced by the patch)

Edit: Attached patch also :-)
Comment by Leif Linse (Zuu) - Thursday, 08 January 2009, 16:11 GMT
* Update to r4916

* Found and fixed an instance of me having put the * next to the type, and not the variable.

I have compiled it, and just tested that closing a window with sub windows does not steal the focus.
Comment by Leif Linse (Zuu) - Thursday, 08 January 2009, 22:29 GMT
* Removed change of totaly unrelated file newgrf_text.h (thanks Belugas)
Comment by Leif Linse (Zuu) - Thursday, 08 January 2009, 22:48 GMT
Update to use the new enums for widgets of NetworkChatWindow that Belugas introduced in r14927.

Version 20 should still apply to trunk and work as intended.
Comment by Leif Linse (Zuu) - Saturday, 10 January 2009, 20:36 GMT
Updated to trunk r14980. Minimal conflict resolved.
Comment by Remko Bijker (Rubidium) - Sunday, 11 January 2009, 17:48 GMT
See the attached patch for some review comments coding style wise.
Comment by Leif Linse (Zuu) - Monday, 12 January 2009, 00:42 GMT
Thank you for your review of the coding style. I have changed the code according to your comments. And those changes are in version 23.

In version 24 I have fixed a bug I found when I play tested it after having made the changes mentioned above. In version 24 I mark widgets that loses focus as dirty. Else the caret of the edit box will not be erased when you click on another widget in the same window.

Before uploading version 24 I read through the patch and used the spell checker in Vim to see that I have not made any more spelling mistakes in my comments. I found three misspelled words that I have corrected (only in version 24, not in version 23).

The WC_CONSOLE hack you commented on where I indeed should call EditBoxInGlobalFocus, I have replaced all that with EditBoxInGlobalFocus() and moved the WC_CONSOLE 'hack' to that function. The function itself I am thinking about adding an 'Is' to the begining of the name. But the function doesn't work on a specific EditBox so I'm not sure if there should be an 'Is' or not in the beginning of the name. One reason to have an 'Is' would be that the return value is a boolean value.
Comment by Remko Bijker (Rubidium) - Monday, 12 January 2009, 09:26 GMT
Huh? I don't quite understand... does 24 contain all changes of 23 or not? Or are they kind of branched froom 22 and both change other things?
Comment by Leif Linse (Zuu) - Monday, 12 January 2009, 11:05 GMT
24 contains all changes of 23
23 contains all changes of 22

So no branching, just like trunk revisions.

Sorry if I did not make it clear enough. My intention was to keep the changes i made in response to your review apart from the others. But probably made it all to confusing instead.
Comment by Leif Linse (Zuu) - Monday, 12 January 2009, 19:21 GMT
I must have been tired or so, because I only allowed scrolling the main viewport when a edit box was in focus, not the opposite. :D Also I assumed that the console will have an edit box focused when I made the console hack but that is just plain wrong, as if the console did have an edit box then the hack would be unnecessary.

So this version 25 fixes
* that you couldn't type 'a' and other global hotkey keys in the console.
* that you could only scroll the map with the arrow keys when a EditBox has focus.

These two errors were introduced in version 23.
Comment by Leif Linse (Zuu) - Sunday, 25 January 2009, 14:12 GMT
I have checked version 25 against trunk r15268, and it applies fine.

Comment by Leif Linse (Zuu) - Sunday, 25 January 2009, 21:07 GMT
* When a window that has an edit box opens, it steals focus, even if an edit box of another window currently has global focus. For example if you edit a sign and opens the save window, the new behavior is that the edit box in the save window receives the global focus.
* Includes coding style changes by Rubidium.

* Have a discussion with the OpenTTD community about which widgets that can receive focus and not. If you click on a button, should the button receive focus or not etc.
Comment by Leif Linse (Zuu) - Sunday, 08 February 2009, 09:46 GMT
Update to current trunk
* WDF_NO_FOCUS used 1 << 3, which now has been taken by another flag. So I changed it to 1 << 8, which is at the bottom of the WindowDefaultFlag enum list. This should be okay I think as the flags are stored in a variable of the type uint.
* Since the content download window got a edit box, there is a small change to make the edit box focused when one opens that window.
* misc_gui.cpp conflict resolved

Hmm, maybe make virtual function "OnSetDefaultFocusedWidget" that by default iterate over the widgets and set the focus to first found edit box. Currently no window needs to override this behaviour as only one edit box can be present. On the other hand if a tab-stop number or similar is added to the widget definition in future that would probably be better to use that to set default focused widget. Though then the default OnSetDefaultFocusedWidget function could be changed to use the tab-stop number. (Currently all windows that has an edit box has to call this->SetFocusedWidget( <widget-id> ) in their constructors.)
Comment by Leif Linse (Zuu) - Sunday, 08 February 2009, 12:19 GMT
Fix: "if(" => "if (" at one place.
Comment by Leif Linse (Zuu) - Sunday, 08 February 2009, 12:36 GMT
hg qpop, failed without me noticing it, so I uploaded the Filter Sign List patch as widgetfocus28 patch.

Since I don't have sufficient rights to remove files I have uploaded, I need to make a new version "29" with the correct patch file.