FS#3472 - Keyboard scrolling sign list

Attached to Project: OpenTTD
Opened by Leif Linse (Zuu) - Sunday, 03 January 2010, 13:47 GMT
Last edited by andythenorth (andythenorth) - Monday, 14 August 2017, 20:36 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 4
  • Leif Linse (Zuu) (2010-03-03)
  • Nick De Frangh (Nickman) (2010-01-04)
  • Peter T (Petert) (2010-01-03)
  • Jose Soler (Terkhen) (2010-01-03)
Private No


This patch comes in two steps. (separate patches)

1. Adds a edit box at the bottom of the sign list window where you can type a filter string. The filter string is used to only display signs that match the filter string. There is also two buttons: "match case" and "clear filter". When the edit box is focused the enter key scrolls the main viewport to the first sign in the filtered list, and the escape key aborts the filter. A global hotkey Ctrl+L has been added to bring up the sign list.

2. The user can now use the arrow keys (when the edit box is focused) to make a selection within the filtered result. Enter key scrolls the main viewport to the selected sign if a selection has been made.

The part 1 patch has been generated by TortoiseSVN.

The part 2 patch has been taken directly from the HG queue patch directory. It needs the part 1 to first be applied to the used SVN revision.

The idea is that with both parts applied that if some one in a multiplayer game tells you to look up a sign "abc", you should be able to bring the sign list window up and do the searching only from the keyboard. Yet it is of course good if the filter functionality can be found by people who do not use the keyboard that much. To find the "abc" sign you will have to do:

Ctrl+L (bring up the sign list window)
f (focus the edit box)
abc (type the "abc" string)
RETURN (go to the "abc" sign)

If the name is a long name and there is many similar names the user can choose to type the first few letters and then use the arrow keys to go down to the correct sign before hitting enter.

The edit box is not focused by default, since that would be annoying for users that do not wish to use the filter functionality but still open the sign list window to watch the sign list. They of course will later want to hit a hotkey for doing some constructions, so the edit box has not to be focused by default.
This task depends upon

Closed by  andythenorth (andythenorth)
Monday, 14 August 2017, 20:36 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 Peter T (Petert) - Sunday, 03 January 2010, 15:27 GMT
Very nice work, Zuu!
Comment by Leif Linse (Zuu) - Monday, 04 January 2010, 14:58 GMT
I should also say that this patch has for long time been available and discussed at tt-forums:

I've attached a combined patch that applies both part1 and part2 to trunk. It is of the same version (30) as the two patches attached above.
Comment by Remko Bijker (Rubidium) - Tuesday, 05 January 2010, 15:07 GMT
about part 1:
- don't replace enumified constants with magic numbers (DrawWidget)
- if (new_filter_string != 0 && strlen(new_filter_string) != 0) <- in this case the first 0 is actually meant to be NULL. However, I'd advice to use !StrEmpty(new_filter_string).
- strncpy(this->filter_string, new_filter_string, strlen(new_filter_string)); <- this is bad, very bad; the last parameter is the number of bytes at most into this->filter_string, that should not depend on the length of the string you're going to put into there. Nevertheless strncpy should not be used at all because its behaviour is error prone. Use strecpy(this->filter_string, new_filter_string, lastof(this->filter_string)); strecpy makes sure the string is \0 terminated, so you don't need to manually do that.
- please document functions/structs that you add; it might look trivial now, but does it in the future?
- void SetFilterString(const char* new_filter_string); <- space at the wrong side of the *
- in SetFilterString move the memset to the else; the strecpy already clears whatever is needed.
- InvalidateData has 0 as default parameter and does a SetDirty, so it's not needed to do another SetDirty.
- const Sign* si = this->signs[0] <- space at wrong side of the *
- maybe keep the case sensitivity setting between different times you open the window?

about part 2:
- if ((this->selected_sign > -1) && ((uint)this->selected_sign >= this->signs.Length())) { this->selected_sign = this->signs.Length () - 1; <- this->selected_sign = Clamp(this->selected_sign, -1, this->signs.Length() - 1); seems simpler to me
- SelectNextSign/SelectPreviousSign <- see ScrollToSelected in network_content_gui.cpp; can be written much simpler
- DrawString(text_left, text_right, y, STR_SIGN_NAME, TC_BLUE); + 4 other lines -> DrawString(text_left, text_right, y, STR_SIGN_NAME, this->selected_sign == i ? TC_BLUE : TC_YELLOW); Then it's easier to see it's only different in colour
- const Sign* si = this->signs[sign_id]; <- space at the wrong side of the * (there're probably more cases)
- adding whitelines that do nothing isn't wanted
- what about supporting page up/page down/home/end besides only up/down? See network_content.gui.cpp for inspiration.
Comment by Leif Linse (Zuu) - Wednesday, 06 January 2010, 21:49 GMT
Thank you Rubidium for your code review.

I have attempted to address all the things that you have listed. Further follows comments on a few of them.

- I have added comments to new functions that I have added excluding virtual overrides of base class functions since I assume these are documented in the base class.
- I have added comments to the struct defined at the beginning of the class.

- In part 2 patch, I have changed the select next/previous code so that it use the code that you suggested for making sure that the selected item will stay visible. My code worked differently (it moved down the view a half page instead of just one line), but for consistence with existing functionality in trunk and shorter code I have chosen to use your suggestion.
- I have added page up/down and home/end as in the content download dialog. However, I had to make it so that you must hold the control key while pressing home/end; otherwise the edit box input handler will use the home/end input to move the cursor in the edit box. The other option I see is to completely leave out the functionality to jump to the beginning/end of the list.

In this version I have converted all three patches to unix style line endings instead of some being mixed in one way and some other being mixed the other way around. I hope that helps.
Comment by Leif Linse (Zuu) - Wednesday, 06 January 2010, 22:59 GMT
Yexo gave me some feedback on IRC. This is an update based on his feedback.
Comment by Leif Linse (Zuu) - Wednesday, 06 January 2010, 23:15 GMT
Forgot to fix one thing. (255 -> constant)
Comment by Leif Linse (Zuu) - Wednesday, 06 January 2010, 23:30 GMT
One thing to consider when testing this patch:

It might be better for the discoverabillity of the part2 functionallity if a change is made so that already when the edit box becomes focused the first item in the list will be marked as selected.
Comment by Leif Linse (Zuu) - Thursday, 07 January 2010, 10:18 GMT
I just remembered that I haven't addressed Rubidiums question regarding making it remember the state of the "match case" button if you close and reopen the window. It seams like a good idea, but I don't know what is the recommended way to do this. I guess an advanced setting would be overkill even if it is only available in the openttd.cfg file. Is a static member variable good enough? Or do you have a centralized storage for these kind of things?

I can probably figure this out myself my looking around in the code, but if you know the answers and feels like telling me you are welcome. :-)
Comment by Leif Linse (Zuu) - Wednesday, 03 March 2010, 20:29 GMT
* Updated to r19307 (no manual changes and only a small hunk fuzz reported by hg)
* Removed some tabs that had got at the end of a comment line.
* Made the SignList window remember the state of the match case button by making match_case a static member.

I think the match case button was the last thing that blocked this patch from being added.
Comment by Leif Linse (Zuu) - Wednesday, 31 March 2010, 21:06 GMT
* Updated to r19523 (nothing was changed for this)
* Changed the variable type of selected_sign to const Sign * to be more consistent with the content download window code. This is also a change in the behavior of the patch to be more consistent with the content download window.
- When you clear the filter the same sign that was previously selected is still selected.
- When you change the filter so that a previously selected item is removed from the list, the selected sign var is set to NULL instead of another sign getting selected.
* the selected_sign var has also been renamed to just "selected" to be more consistent with the content download window code.

I did however not introduce the list_pos integer variable as I didn't think it was motivated to at class scope cache the list index of the selected sign for the sign list window. In the content download window it might be more motivated. I've tested the patch on #openttdcoop and did not experience any slowness. Only when there is a key press it will have to loop through at worst all signs to find the index of a sign.

* I've changed the widget structure a bit so that there is at the top level a horizontal container with two vertical containers instead of the other way around. This change make it consistent with the town list gui and possible other windows too. If a user selects a big font, the resize-button will now not be resized. I've also added a border around the edit box as eg. the query string window. I've attached an image showing this.

* I've tested this update a bit to see that the things I've changed still works and it seems to be working as I want it to.
* I've also read through the patch files and searched for white spaces and my habit of writing "if(" (without a space before the bracket).
Comment by Leif Linse (Zuu) - Monday, 21 June 2010, 19:38 GMT
Updated to current trunk (r20007)
* Make use of common GUI strings to reduce new strings
* Resolved conflict in english.txt
Comment by Leif Linse (Zuu) - Monday, 21 June 2010, 21:30 GMT
* Re-build list when a sign is renamed and there is a filter string.
* Found and reported that sorting of signs is not stable (this bug also exists in trunk). See  FS#3893 .

Edit: The unstableness of the sign sort was fixed by Rubidium/Remko in r20009

In version 37, part 2 there was an additional change: When the sign list is changed while a sign is selected (in the sign list), then the list will scroll to the selected. I have changed my mind and decided that if you have selected a sign, then you want to have that sign visible even if some other player/AI decides to create *lots* of new signs.
Comment by Leif Linse (Zuu) - Saturday, 07 August 2010, 17:58 GMT
* Updated to r20394
* The patch do no longer add any hotkey for opening the sign window (since there is a hook for users to assign a such hotkey in their hotkeys.cfg)

A later patch might want to add a default hotkey for opening the sign window. (this patch used to use Ctrl+L for that)

A remaining issue is that the patch use a hard coded hotkey 'F' to focus the edit box. This one could be made configurable if wanted. The other hotkeys, Page Up/Down etc. that can only be used while the edit box is focused does not make as much sense to make configurable. Only non-symbol producing keys makes sense as else you would type something into the edit box while using the hotkey for eg. going down in the sign list.
Comment by Leif Linse (Zuu) - Saturday, 07 August 2010, 19:25 GMT
* Make the focus filter edit box hotkey configurable (it accepts global hotkeys)
Comment by Jose Soler (Terkhen) - Friday, 13 August 2010, 16:16 GMT
* GUISignList, Members of the FilterInfo struct, the SignList constructor and the SignListHotkeys enum (and its members) are not documented.
* // <- needed because of variable declarations inside case-block <-- This comment is not required.
* For opening the sign window, you could add a hotkey without default binding (see r20201) as a different patch.

* Changing the if/else to "this->SetWidgetLoweredState(SLW_FILTER_MATCH_CASE_BTN, SignList::match_case);" could be made directly at 10.
* I agree that moving accross the list only makes sense when using the keyboard, but IMO it would be more consistent if clicking over a sign marked it as the currently selected sign. That way you can click on a sign and start moving across the list with the keyboard.

The current version of the patch does not apply to trunk (I'm guessing it is because of the recent scrollbar changes).
Comment by Leif Linse (Zuu) - Saturday, 14 August 2010, 22:09 GMT
Thank you for the review. I'm not sure why you used 10 and 20 for part 1 and 2 instead of just '1' and '2' (or did I misunderstood you?).

* Documented all that you listed plus the widget enum members. (GUI*List is nowhere else documented, and I find my documentation fairly redundant with the code it documents)
* Removed
* In current trunk there is already a hotkey without a default binding for opening the sign list window. (It's called "sign_list" and is located under [maintoolbar] in hotkeys.cfg)

* Fixed (probably a result of changing the implementation in the wrong patch)
* Implemented

- Updated to last trunk. It was indeed the updated scrollbar code that broke it.

Additional changes:
- Reduced code duplication by making the escape-key handler call OnClick(Point(), SLW_FILTER_CANCEL_BTN, 1).
- When clicking on the clear button (or hitting escape) - don't clear the selection
- Added tooltips for clear and match case buttons.
- Removed unwanted changes of rev.cpp from the part2-patch (may reappear as I have only manually edited the produced patch file)
Comment by Jose Soler (Terkhen) - Monday, 16 August 2010, 16:22 GMT
Sorry, I usually number my own queues as 10, 20 and so on :)

Part 1 has been committed (r20516), but we have found some problems in part 2. The most important is that using up/down or page up/page down with an empty sign list will cause assertions (a different one with each pair of keys). I suspect that other limit cases could cause problems too. Also, pressing shift will cause the window to scroll to the selected sign even if the filter string has not changed at all.

After some testing with both short and long lists I find counterintuitive that you can't use the keys when any part of the window is focused. I'm always trying to use them after clicking on a sign before remembering that I can't... This does not seem trivial, as the viewport will move too if any widgets other than the edit box are focused.