Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
DorpsGek opened this issue Sep 13, 2008 · 29 comments
Closed
Labels
component: interface This is an interface issue flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) patch from FlySpray This issue is in fact a Patch, but imported from FlySrpay

Comments

@DorpsGek
Copy link
Member

Zuu opened the ticket and wrote:

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)

Note:
* 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 (http://bugs.openttd.org/task/2275), 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.

Attachments

Reported version: trunk
Operating system: All


This issue was imported from FlySpray: https://bugs.openttd.org/task/2297
@DorpsGek
Copy link
Member Author

Zuu wrote:

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.


This comment was imported from FlySpray: https://bugs.openttd.org/task/2297#comment4742

@DorpsGek
Copy link
Member Author

Zuu wrote:

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

  		else
  			ShowNetworkChatQueryWindow(DESTTYPE_BROADCAST, 0);

The attached patch file is for r14420.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/2297#comment4801

@DorpsGek
Copy link
Member Author

Zuu wrote:

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.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/2297#comment4802

@DorpsGek
Copy link
Member Author

DorpsGek commented Oct 6, 2008

Zuu wrote:

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.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/2297#comment4838

@DorpsGek
Copy link
Member Author

DorpsGek commented Oct 6, 2008

Zuu wrote:

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.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/2297#comment4839

@DorpsGek
Copy link
Member Author

Zuu wrote:

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.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/2297#comment4863

@DorpsGek
Copy link
Member Author

Zuu wrote:

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

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/2297#comment4919

@DorpsGek
Copy link
Member Author

Zuu wrote:

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.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/2297#comment4921

@DorpsGek
Copy link
Member Author

Zuu wrote:

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)


This comment was imported from FlySpray: https://bugs.openttd.org/task/2297#comment4924

@DorpsGek
Copy link
Member Author

Zuu wrote:

Changes:
* 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: http://bugs.openttd.org/task/2375

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.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/2297#comment4928

@DorpsGek
Copy link
Member Author

Zuu wrote:

Fixed if-statements that did not confirm with the coding style - thanks to SmartZ for notifying me of not confirming with the coding styles.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/2297#comment4929

@DorpsGek
Copy link
Member Author

Zuu wrote:

* 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)

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/2297#comment4943

@DorpsGek
Copy link
Member Author

Zuu wrote:

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.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/2297#comment4951

@DorpsGek
Copy link
Member Author

DorpsGek commented Jan 3, 2009

Zuu wrote:

Changes:
* 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 :-)

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/2297#comment5190

@DorpsGek
Copy link
Member Author

DorpsGek commented Jan 8, 2009

Zuu wrote:

Changes
* 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.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/2297#comment5247

@DorpsGek
Copy link
Member Author

DorpsGek commented Jan 8, 2009

Zuu wrote:

Changes:
* Removed change of totaly unrelated file newgrf_text.h (thanks Belugas)

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/2297#comment5254

@DorpsGek
Copy link
Member Author

DorpsGek commented Jan 8, 2009

Zuu wrote:

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.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/2297#comment5255

@DorpsGek
Copy link
Member Author

Zuu wrote:

Updated to trunk r14980. Minimal conflict resolved.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/2297#comment5282

@DorpsGek
Copy link
Member Author

Rubidium wrote:

See the attached patch for some review comments coding style wise.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/2297#comment5294

@DorpsGek
Copy link
Member Author

Zuu wrote:

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.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/2297#comment5300

@DorpsGek
Copy link
Member Author

Rubidium wrote:

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?


This comment was imported from FlySpray: https://bugs.openttd.org/task/2297#comment5304

@DorpsGek
Copy link
Member Author

Zuu wrote:

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.


This comment was imported from FlySpray: https://bugs.openttd.org/task/2297#comment5305

@DorpsGek
Copy link
Member Author

Zuu wrote:

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.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/2297#comment5315

@DorpsGek
Copy link
Member Author

Zuu wrote:

I have checked version 25 against trunk r15268, and it applies fine.


This comment was imported from FlySpray: https://bugs.openttd.org/task/2297#comment5433

@DorpsGek
Copy link
Member Author

Zuu wrote:

Changed:
* 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.

Todo:
* 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.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/2297#comment5434

@DorpsGek
Copy link
Member Author

DorpsGek commented Feb 8, 2009

Zuu wrote:

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( ) in their constructors.)

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/2297#comment5541

@DorpsGek
Copy link
Member Author

DorpsGek commented Feb 8, 2009

Zuu wrote:

Fix: "if(" => "if (" at one place.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/2297#comment5542

@DorpsGek
Copy link
Member Author

DorpsGek commented Feb 8, 2009

Zuu wrote:

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.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/2297#comment5543

@DorpsGek
Copy link
Member Author

DorpsGek commented Feb 9, 2009

Rubidium closed the ticket.

Reason for closing: Implemented

In r15424. Seems fine to me; but I can't tell what bugs others might find.


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

@DorpsGek DorpsGek closed this as completed Feb 9, 2009
@DorpsGek DorpsGek added component: interface This is an interface issue flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) patch from FlySpray This issue is in fact a Patch, but imported from FlySrpay labels Apr 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: interface This is an interface issue flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) patch from FlySpray This issue is in fact a Patch, but imported from FlySrpay
Projects
None yet
Development

No branches or pull requests

1 participant