Patch : Network game list name filter

[handrake]

Hello,

This patch provides the basic text filter functionality to network game. (FS #3852) Although there are many other criteria I can filter on (number of clients, companies, map size, etc...), I thought it would be better to start small and expand. I'll add as soon as I finish the rest. I hope someone finds this useful.

-handrake

network-name-filter-r24077.diff

(12.54 KiB) Downloaded 168 times

[stevenh]

Great idea... but does it need to be a popup?

A "Filter: [___________]" box up the top next to Connection Type would be ideal.
Saving your previous filter and putting an "[X]" button to clear the box would be cool too.

[FooBar]

I believe the popup has something to do with the fact that there can be only one text input thingy in a window. The player name already takes this position.
Don't ask me the details on this though
Maybe it's better to move the player name to a popup, as I think one changes that less often than a filter.

I agree that it would be useful to see somewhere that a filter is applied, what that filter is, and an option to remove it. Other than that I can only applaud the effort, well done!

[Eagle_rainbow]

Hi folks,

the first time using the network server list I also was dumbstrucked by the long list without any filter capability. Every time I log on to a multiplayer game, I wish to have a filter edit box there as well, because most time I know at least part of the server's name I want to join.
Having now read this thread and the flyspray entry, I thought I could make that better. Your approach with the additional popup box, handrake, however, didn't suite me (nothing against you, though ). Thus, I am quite in line with Stevenh. Frosch already mentioned (in FS) the main problem on the matter is that "there is no focus". Wanting to solve this issue with minimal impact to existing coding (there are too many consumers of class QueryStringBaseWindow - making an error there could have devastating side-effects), I tried to allow emulate multiple edit boxes in a window by switching a single edit box following the current focus that the user has set. Still, you are able to access the edit boxes in the coding seperately, if you are willing to use an enhanced interface which is provided.
As I thought that this issue could occur on other windows as well, I tried to encapsulate this approach in a dedicated class called MultipleQueryStringBaseWindow.
Find attached the patch file how I tried to solve it:

mpfilter-against-24634.patch

[patch] new variant of filtering on network list against r24634

(22.97 KiB) Downloaded 150 times

Here's a screenshot on how this patch visualizes:


PS: "Clear" button and "history of filters" (like you know from a browser's address bar) is not available yet. The visualization that the list is filtered is done natively, as you see that there is something in the filter edit box.

[Eagle_rainbow]

Adding the "Clear" button was much easier than I thought initially.
However, I really don't have a clue how we could add something like the address bar drop down you know from your web browser.

Anyway, here's an updated patch for it (against same rev. than before) which provides the clear button

[Eagle_rainbow]

Adopting my learnings from a code review in this thread, I have adjusted the patch in this thread here as well.
Here's the new version:

[Eagle_rainbow]

Here are some more adjustments to the coding style to make the patch more compliant to the rules.

[Eagle_rainbow]

In today's #openttd.dev session, there was a debate on this patch. I would like to summarize the result here briefly:

• The approach with class MultipleQueryStringBaseWindow to allow multiple edit boxes in a window was rejected. Instead the developers intend to enable the GUI framework to allow multiple edit boxes in general; that is a bigger task and won't be there short-term.
• For the time being the player name available on the network list window should go to a separate window (with a button or something like this, or query string window), whilst retaining the filter on the network list window. The player name should still be visible on the window to see which name has been set. Furthermore, Yexo asked to split these two items (change player name and filtering) into two distinct patches.

I will try to go into that direction (second bullet point from above) and revert back to this topic with updates.

[Eagle_rainbow]

Here's the first patch (draft) as discussed above: It will remove the player's name edit box and move the change option behind a button into a QueryStringWindow to allow adding a filter edit box in a second step (patch in prep).

remove_player_editbox-against-r24661.patch

remove player's name edit box in the network game list
trunk against r24661

(10.29 KiB) Downloaded 160 times

Screenshot of current state

OpenTTD gd45adb25-remove_player_name_002.png (3.73 KiB) Viewed 6456 times

On implementing it, I detected the following things which I think are worthwhile discussing in the upcoming code review:

• When trying to get rid of the constant WID_NG_CLIENT (the edit box for the player's name on the screen) I detected references in script/api/script_window.hpp and thus also in script/api/game/game_window_hpp_sq. Having a short alignment with Yexo (see also IRC log at the very bottom), I also deleted these references. Thus, also the auto-generated file will be shown as changed in the patch - I think it would be better to regenerate that file (seems to be auto-generated via script/api/squirrel_export.sh).
  Furthermore, Yexo was also puzzled about the publishing in sq and thus created a FS entry for that.
• When writing the changed player's name, I reused the original coding to do so. There, I think I found a glitch: A comment tells that only letters would be allowed in player names. However, the coding only checks if the first letter isn't a whitespace. Digits are not checked; in the original version furthermore it is possible to also enter symbols.
  I have marked the location with a FIXME comment, because I think that either the comment is wrong or the coding.

Feedback appreciated as usual!

[Eagle_rainbow]

Here's the first patch (draft) as discussed above: It will remove the player's name edit box and move the change option behind a button into a QueryStringWindow to allow adding a filter edit box in a second step (patch in prep).

Though I did not receive any feedback on this patch, I have updated the patch to the most current revision of trunk:

remove_player_editbox-v2-against-r24676.patch

removes the player edit box on the multiplayer screen
again trunk r24676

(10.29 KiB) Downloaded 152 times

Furthermore, I was able to finish up the patch for the filter edit box:

mpfiltersingleeditbox-v1-against-r24676.patch

adds a filter edit box to the multiplayer screen
requires the patch to remove the player edit box available with this post
against trunk r24676

(18.29 KiB) Downloaded 154 times

Combining those two patches, the multiplayer screen now looks like this:

[MinchinWeb]

You have a growing collection of very neat patched Eagle Rainbow!

I wonder here if the tooltip cannot be simplified. What about if you were to use "Filter available network games" instead?

[Eagle_rainbow]

Thanks for your compliment.

Concerning the tool tip label - I see what you mean. Though, personally text labels are quite smoke and mirrors to me, as long as I am able to understand what is meant, it still may be important for many others
I have changed the label as you suggested.

Here are the new patches version which are now also against the most recent trunk revision:

remove_player_editbox-v2-against-r24695.patch

removes the player edit box on the multiplayer screen
against trunk r24695

(10.29 KiB) Downloaded 156 times

mpfiltersingleeditbox-v2-against-r24695.patch

adds a filter edit box to the multiplayer screen
requires the patch to remove the player edit box available with this post
against trunk r24695

(18.24 KiB) Downloaded 154 times

Is there further feedback which I may incorporate on that topic here?

[Eagle_rainbow]

• The approach with class MultipleQueryStringBaseWindow to allow multiple edit boxes in a window was rejected. Instead the developers intend to enable the GUI framework to allow multiple edit boxes in general; that is a bigger task and won't be there short-term.

Frosch just commited a set of patches to trunk which enables the GUI framework to support multiple edit boxes on the same window. Making use of that feature, I have now adjusted the multiplayer filter patch to bypass the "remove_player_name". The window now looks like this:

screenshot of new multiplayer filter

OpenTTD gee7648acM-mpfiltermultieditbox_008.png (7.14 KiB) Viewed 6117 times

And here's the patch against the most recent revision of trunk:

mpfiltermultieditbox-v3-against-r24751.patch

adds a filter edit box to the multiplayer screen
against trunk r24751

(15.49 KiB) Downloaded 151 times

[Eagle_rainbow]

Here's an updated version of the patch after a code review in #openttd.dev

[Eagle_rainbow]

Here's the next version...

[frosch]

Finally I have a test-patch for multiple editboxes! (there are still some issues with the OSK.)

About this patch: I fixed some issues, mostly about the window layout and drawing filtered list.

However, there are still several bugs with the list while it is filtered. Mostly everything using "list_pos" fails while the list is filtered. E.g. ScrollToSelectedServer or the keyboard navigation.

[Eagle_rainbow]

However, there are still several bugs with the list while it is filtered. Mostly everything using "list_pos" fails while the list is filtered. E.g. ScrollToSelectedServer or the keyboard navigation.

Thanks for testing my stuff.
I will have a look on the "list_pos" thingy soon and revert back to this thread with updates.

[Eagle_rainbow]

I will have a look on the "list_pos" thingy soon and revert back to this thread with updates.

Here we go with my update.
In the previous patches 'list_pos' is not handled properly. This leads to a very strange (not to say erratic ) behaviour when using the cursor keys for example. Also sorting could happen to become quite strange. Having had a second thought, I started to dislike my initial approach pushing down the filtering concept to the _network_game_list. This linked list is more or less a technical thing whilst sorting, filtering and list_pos stuff tend to be "lightweight" GUI-related things. This gets even more apparent, if you are looking at 'GUIGameServerList' which also has a capability of filtering.
I now reimplemented the entire patch to make use of FilterFunctions in the server list. This had the side-effect that BuildNetworkGameList became a bit more powerful, as it now has to handle sorting as well (which IMHO isn't a bad idea either, because it makes life easier keeping the list consistent). As there is a clear difference between _network_game_list and the servers vector now, I also changed the name of BuildNetworkGameList to BuildGUINetworkGameList to stress that difference (previously, the two lists where kept in sync, especially with regard to sorting which isn't worthwhile anymore due to filtering). This resolved a lot of issues which otherwise I would have had to "code around" and would have been evil to understand later on (for example: changing sort order when list is filtered).
Please find attached the newest version of this patch:

mpfiltermultieditbox-v6-against-r24765.patch

adds a filter edit box to the multiplayer screen against r24765

(18.49 KiB) Downloaded 146 times

However, during testing this version, I found one thing that still looks inconsistent to me: Though the cursor keys are working properly now, the effect of pressing Home or End now depends on the current focus: If the focus is with one of the edit boxes, you'll change the cursor position within the edit box. If the focus is with the matrix, you'll navigate through the server's list. What makes it really a mess, is the fact, that the PageUp/Down keys are working regardless of the current focus (and will change the list_pos) . My first thought on this was to disable the Home and End keys for the server list entirely - but that is most likely due to the fact that I don't use them at all (and before having had read the coding, didn't even know that this feature existed ). However, ignorance isn't a good reasoning after all. The other alternative is to disable PageUp/Down, if the focus is with the edit boxes. Yet, I don't have a preference on how to resolve that.
Do you have another good idea?

PS: We might need to have another round of reviews on it later, as several things have changed again. Yet, I didn't have a full passthrough on the diff myself, so there still could be some quirks in there (hopefully coding style only).

[Zuu]

While having a look at your patch, I found a way to trigger an assert:

• (press Find servers to fill list, unless you have enough manual servers)
• Select an item in the list
• Type a filter that causes the selected item to disappear
• Press arrow down/up
• => assertion failed

What makes it really a mess, is the fact, that the PageUp/Down keys are working regardless of the current focus (and will change the list_pos)

This would also apply to arrow up/down if you think it is a problem.

I do however not see it as a problem. Yes, someone may wonder why page up/down + arrow keys works but not home/up to scroll the list while not being aware of the edit box being focused. However, I don't see any good fix for that. And in that situation, I think it is better to let arrows+page up/down work than not.

(however, other devs may have a different opinion)

[Eagle_rainbow]

While having a look at your patch, I found a way to trigger an assert:

• (press Find servers to fill list, unless you have enough manual servers)
• Select an item in the list
• Type a filter that causes the selected item to disappear
• Press arrow down/up
• => assertion failed

Thanks for pointing that out. Here's a new version which fixes this problem:

mpfiltermultieditbox-v7-against-r24765.patch

adds a filter edit box to the multiplayer screen against r24765

(18.68 KiB) Downloaded 141 times

It also incorporates some minor enhancements (mainly on comments).