Petert wrote:

Very nice work, Zuu!

This comment was imported from FlySpray: https://bugs.openttd.org/task/3472#comment7221

Zuu wrote:

I should also say that this patch has for long time been available and discussed at tt-forums: http://www.tt-forums.net/viewtopic.php?f=33&t=38715

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.

Attachments

• filter_sign_list30_part1and2_r18696.patch (11.98 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/3472#comment7245

Rubidium wrote:

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.

This comment was imported from FlySpray: https://bugs.openttd.org/task/3472#comment7261

Zuu wrote:

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.

Attachments

• filter_sign_list31_part1_r18696.patch (10.18 KiB)
• filter_sign_list31_part2_r18696.patch (4.60 KiB)
• filter_sign_list31_part1and2_r18696.patch (12.89 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/3472#comment7276

Zuu wrote:

Yexo gave me some feedback on IRC. This is an update based on his feedback.

Attachments

• filter_sign_list32_part1_r18696.patch (9.89 KiB)
• filter_sign_list32_part2_r18696.patch (4.68 KiB)
• filter_sign_list32_part1and2_r18696.patch (12.60 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/3472#comment7278

Zuu wrote:

Forgot to fix one thing. (255 -> constant)

Attachments

• filter_sign_list33_part1_r18696.patch (9.91 KiB)
• filter_sign_list33_part2_r18696.patch (4.68 KiB)
• filter_sign_list33_part1and2_r18696.patch (12.63 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/3472#comment7279

Zuu wrote:

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.

This comment was imported from FlySpray: https://bugs.openttd.org/task/3472#comment7280

Zuu wrote:

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. :-)

This comment was imported from FlySpray: https://bugs.openttd.org/task/3472#comment7285

Zuu wrote:

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

Attachments

• filter_sign_list34_part1_r19307.patch (10.11 KiB)
• filter_sign_list34_part2_r19307.patch (4.81 KiB)
• filter_sign_list34_part1and2_r19307.patch (12.83 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/3472#comment7651

Zuu wrote:

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

Attachments

• filter_sign_list35_part1_r19523.patch (10.71 KiB)
• filter_sign_list35_part2_r19523.patch (5.54 KiB)
• filter_sign_list35_part1and2_r19523.patch (13.88 KiB)
• SignListPatch35.png (17.12 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/3472#comment7800

Zuu wrote:

Updated to current trunk (r20007)
* Make use of common GUI strings to reduce new strings
* Resolved conflict in english.txt

Attachments

• filter_sign_list36_part1_r20007.patch (10.51 KiB)
• filter_sign_list36_part2_r20007.patch (5.54 KiB)
• filter_sign_list36_part1and2_r20007.patch (13.67 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/3472#comment8155

Zuu wrote:

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

Attachments

• filter_sign_list37_part1_r20007.patch (11.23 KiB)
• filter_sign_list37_part2_r20007.patch (5.74 KiB)
• filter_sign_list37_part1and2_r20007.patch (14.54 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/3472#comment8156

Zuu wrote:

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

Attachments

• filter_sign_list38_part1_r20394.patch (10.17 KiB)
• filter_sign_list38_part2_r20394.patch (5.74 KiB)
• filter_sign_list38_part1and2_r20394.patch (13.47 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/3472#comment8475

Zuu wrote:

Changelog:
* Make the focus filter edit box hotkey configurable (it accepts global hotkeys)

Attachments

• filter_sign_list39_part1_r20394.patch (13.06 KiB)
• filter_sign_list39_part2_r20394.patch (5.74 KiB)
• filter_sign_list39_part1and2_r20394.patch (16.33 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/3472#comment8478

Terkhen wrote:

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

20:
* 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).

This comment was imported from FlySpray: https://bugs.openttd.org/task/3472#comment8510

Zuu wrote:

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?).

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

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

Attachments

• filter_sign_list40_part1_r20495.patch (14.21 KiB)
• filter_sign_list40_part2_r20495.patch (5.21 KiB)
• filter_sign_list40_part1and2_r20495.patch (17.56 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/3472#comment8520

Terkhen wrote:

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.

This comment was imported from FlySpray: https://bugs.openttd.org/task/3472#comment8548

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/3472