[Patch] Filter Sign List (by typing a filter string) - v40

[Zuu]

The first two blocks I guess is because I've exchanged constant values {0, 1, 2} with an enum since I introduce a fourth value. Adding "default: break;" to those switch-case statements could probably silence the warnings.
[...]

If the other two values should actually never occur in those statements, "default: NOT_REACHED();" is a better way to go.

In this particular case that solution is not a good idea. Since the different return values depend on what the user does. The four return values are: "pressed enter", "pressed escape", "edit box does not have focus" and "default" (edit box has focus and user pressed any other key than enter and escape). So having OpenTTD assert on some user behaviour is not a good idea. (But thanks for pointing the method/solution out.)


planetmaker:
Looks like you've used default: NOT_REACHED(); in your fix. Try pressing any other key than enter and escape when you have the edit box of the sign list window in focus. That should yeild return value "default" which would assert your fix.

Through I see that I have forgot to replace 1,2 with HEBR_ENTER and HEBR_ESCAPE in the OnKeyPress of that window in the Filter Sign List patch.

[planetmaker]

Looks like you've used default: NOT_REACHED(); in your fix. Try pressing any other key than enter and escape when you have the edit box of the sign list window in focus. That should yeild return value "default" which would assert your fix.

He, right. Removed my broken fix for now

[Zuu]

Now, I've fixed those warnings reported by planetmaker (I hope, have not verified with gcc) and also I've set up hg queues which is a tool that remembers what changes are done in which patch so I can change something in say widget_focus patch and this tool helps me to push those changes up to this patch. Earlier I've done that totally manually, with one source tree per patch.

What you might notice (if you read the code) is that minor coding style changes that I previously did not bother to push up to this patch are now included.

The published patches them self will however still be against SVN trunk.


Edit: Added a win32 binary for r14508 which is the nightly version currently used by #openttdcoop PS and ProZone.

[Zuu]

Updated to trunk, r14535.

r14534 contains the first commit of code from this patch. It is the enumification of the return values of HandleEditBox function that has been commited to trunk by glx. It can be seen in a separate patch here: http://bugs.openttd.org/task/2382

This trunk commit of course breaks this patch so therefor the need of an update of the patch now.

This update also contains:

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.

[Zuu]

Changes:

• WidgetFocus: Aali reported that if you had the console open, any window would steal the focus when they opens. - This has been fixed.
• FilterSignList: Added a match case button (push button) so you can select if it should match or ignore case, when filtering the list. Match case is off by default. But I am open to change that if you want it to be on by default. Note that it uses strtolower, so I guess only A-Z is converted to a-z, but no non-English characters.
• FilterSignList: In some cases when you pressed 'f' the text box was not focused, this was because the sign list window did not have focus in those situations. Now the sign list window will make itself the focused window when you press 'f' so that 'f' actually work, even if the sign list window is not focused when you type the hotkey.
• FilterSignList: Added possibility to select a sign from the list with the arrow up/down keys and then shows that sign when you press enter, instead of always showing the sign in the top of the list.

If you want the most then use this one:
(Use arrow up/down to select, and then enter to move main viewport to sign)

select_in_filtered_list.png (37.39 KiB) Viewed 4129 times

filter_sign_list12_r14659.patch

Recommended patch file

(41.97 KiB) Downloaded 159 times

Or if you want to stick to FilterSignList without arrow up/down selection of sign: (The main reason that I even provide this version is that I have it available through hg queues, but most users should find this version useless and use the one above, which works like this one, but with one more feature)

filter_sign_list12_r14659_without_arrow_key_selection.patch

(40.2 KiB) Downloaded 99 times

[Zuu]

r14660, the revision currently used by #openttdcoop Public Server compiled with Filter Sign List 12

[Zuu]

Today Rubidium committed my unified open OSK patch which is a foundation to the widget_focus patch that this patch builds on. Of course this breaks the widget-focus and filter sign list patch. So i've updated both patches to trunk.

The only usage change that *should* be from version 12 is that when you click on the 'X' to close another window, that window will no longer gain focus. So if you are typing in an edit box and decides to close a window, the edit box is still focused.

filter_sign_list13_r14818.patch

This patch applies to trunk r14818

(34.67 KiB) Downloaded 108 times

As an experiment I attach a patch that is against trunk + Widget Focus. So it only contains the changes needed given that Widget Focus is applied. This patch is taken directly from .hg/patches/

filter_sign_list13_r14818WidgetFocus.patch

This patch is against r13818 with WidgetFocus18 patch applied.

(11.87 KiB) Downloaded 92 times

[Zuu]

Widget focus was just updated to trunk, and here follows an updated filter sign list patch against trunk. There has been no changes to the filter sign list patch however, so the old patch against trunk + widget focus is still up to date.

filter_sign_list14_r14916.patch

Against trunk r14916

(34.13 KiB) Downloaded 133 times

Edit:
Version 15
Belugas found out that a change of newgrf_text.h had in some way reached in to the patch. This is removed in version 15. But the change is just of the file identifier comment, so no actual code was changed. Anyway, right should be right, so here is version 15 without changes to newgrf_text.h.

filter_sign_list15_r14916.patch

Against trunk r14916

(33.47 KiB) Downloaded 103 times

Edit2:
Version 16
In r14927 Belugas introduced enums for the widgets in network chat window, this small update make use of the enum for the text box in a call to SetFocusedWidget instead of previously use widget number.

[Zuu]

Small update resolving a minor conflict against trunk. Actually my UnifiedOpenOSK patch that had a tab character ( ) that Rubidium today fixed to a space. The widget focus patch makes OSK windows only open if you click on a already focused edit box and that is why the unification of open OSK was necessary.


If anyone have any comments on this patch, or even more interesting, the Widget Focus patch which this patch builds on, please tell. If not else, to not make me feel like a spammer for my now seventh post in a row.

Is widget focus annoying or good? My intentions with it was that you should be able to keep the sign list window open and still be able to use hot keys. Also as adding an additional window with edit box I figured it eventually will become a problem that to many windows get into the category query windows of which only one in the category can be open at a given time.

[planetmaker]

Hi,

I noticed a funny part in the patch file:

Code: Select all

Index: src/tar_type.h
===================================================================
--- src/tar_type.h	(revision 14980)
+++ src/tar_type.h	(working copy)
@@ -1,10 +1,10 @@
 /* $Id$ */
 
-/** @file tar_type.h Structs, typedefs and macros used for TAR file handling. */
-
 #ifndef TAR_TYPE_H
 #define TAR_TYPE_H
 
+/** @file tar_type.h Structs, typedefs and macros used for TAR file handling. */
+
 #include <map>
 #include <string>
 

Kind of unnecessary and makes the diff fail there (no problem obviously).

[Zuu]

Hi,

I noticed a funny part in the patch file:

Code: Select all

Index: src/tar_type.h
===================================================================
--- src/tar_type.h	(revision 14980)
+++ src/tar_type.h	(working copy)
@@ -1,10 +1,10 @@
 /* $Id$ */
 
-/** @file tar_type.h Structs, typedefs and macros used for TAR file handling. */
-
 #ifndef TAR_TYPE_H
 #define TAR_TYPE_H
 
+/** @file tar_type.h Structs, typedefs and macros used for TAR file handling. */
+
 #include <map>
 #include <string>
 

Kind of unnecessary and makes the diff fail there (no problem obviously).

Thanks for noticing it. Actually I have fixed that one in the widget focus patch on FS, just that I was to lazy to update also the filter sign list patch for every change in the underlying widget focus patch. But this change should have been done also here, as it affects the ability to apply the patch.

The reason behind that I can see is some kind of conflict between svn and hg or so. After a 'SVN up' and a 'hg commit -m "r12345"' the command 'hg qpush' fails because hg detects changes. The solution is often to do a 'svn revert' on the correct file. The problem is that I have not found a way to get the correct file. So in this case I just did a 'hg qpush --force' in hope that it didn't do anything bad. But as you noticed there was something that shouldn't get included there.

[planetmaker]

The reason behind that I can see is some kind of conflict between svn and hg or so.

Why not go with a pure hg repository?
What I usually do is
hg qpop --all
hg pull -u
hg qpush --all
The latter step will fail, of course sometimes which means work at the patch which fails.

regards,
pm

[Zuu]

Here is filter sign list patch for r15268 with widget_focus version 25 included. That version of widget_focus.patch has survived from before both BaNaNaS and NoAI was merged into trunk. Now neither of them uses edit boxes so they shouldn't conflict.

I have read through this one and made sure there are no absurd changes in it.


And a reminder, the widget_focus patch is not technically required by this patch. Filter Sign List could work on its own. The motivation behind the widget_focus patch is that adding more and more edit boxes to OpenTTD would become problematic as they work currently in trunk. (only one window with edit box can/should be open at a given time) So therefor I wrote the widget_focus patch so trunk can accept more windows with edit boxes. If you are interested only in the widget focus patch I suggest visiting FS#2297.

[planetmaker]

I was just updating my patches when I noticed that there was a small change in misc_gui.cpp which broke this patch. Easy fix, here's the update to r15286.

[belugas]

Are you sure you fixed it correctly?
Mhhh...
I wonder what was the commit that broke the patch. And what it brought to the code base...

Want more hints?

[planetmaker]

I took filter_sign_list18 which is the most recent in the latest post here by Zuu. This is what my misc_gui.cpp.rej looks like:
***************

Code: Select all

*** 1004,1010 ****
  
  void QueryString::HandleEditBox(Window *w, int wid)
  {
- 	if (HandleCaret(&this->text)) w->InvalidateWidget(wid);
  }
  
  void QueryString::DrawEditBox(Window *w, int wid)
--- 1017,1023 ----
  
  void QueryString::HandleEditBox(Window *w, int wid)
  {
+ 	if (HasEditBoxFocus(w, wid) && HandleCaret(&this->text)) w->InvalidateWidget(wid);
  }
  
  void QueryString::DrawEditBox(Window *w, int wid)

which was broken by r15273. So I think it's quite valid to change to do

Code: Select all

@@ -1004,7 +1017,7 @@
 
 void QueryString::HandleEditBox(Window *w, int wid)
 {
-	if (HandleCaret(&this->text)) {
+	if (HasEditBoxFocus(w, wid) && HandleCaret(&this->text)) {
 		w->InvalidateWidget(wid);
 		/* When we're not the OSK, notify 'our' OSK to redraw the widget,
 		 * so the caret changes appropriately. */

as that's just adopting this line to the new "environment". This shouldn't have changed anything in the behaviour of this patch... Or do I grossly miss something which I fail to see from my tiny island of knowledge? Obviously I need more hints

[Roujin]

Hello,

just wanted to throw in here that a framework for filtering GUILists is now in trunk (r15367), so if this patch is updated to trunk, it should be changed accordingly to make use of the new functions..

It works pretty much like the sorting of GUILists already works: You can make one or multiple callback functions that specify what the filter exactly does - like the sorting function(s) which specifies how to decide if A is smaller, equal, or bigger than B (-1/0/1); the filtering function specifies for an item A if it will be filtered out or not (true/false), given the data you pass to the filter function (in this case you'd pass the pointer to the entered filter string).

edit: now (r15372) Rubidium committed a working application of the GUIList: filtering for Tags in the content download window. This commit is a good example how to work with the framework.

[Zuu]

So I think it's quite valid to change to do

Code: Select all

@@ -1004,7 +1017,7 @@
 
 void QueryString::HandleEditBox(Window *w, int wid)
 {
-	if (HandleCaret(&this->text)) {
+	if (HasEditBoxFocus(w, wid) && HandleCaret(&this->text)) {
 		w->InvalidateWidget(wid);
 		/* When we're not the OSK, notify 'our' OSK to redraw the widget,
 		 * so the caret changes appropriately. */

as that's just adopting this line to the new "environment". This shouldn't have changed anything in the behaviour of this patch... Or do I grossly miss something which I fail to see from my tiny island of knowledge? Obviously I need more hints

I just brought the Widget Focus patch up to date, which is the patch that change belong to. And I did exactly what you did. The new code that was added to trunk fixes a problem where the edit box of the OSK window was not re-painted so that the carret flashed when the network chat window is open (and have focus). This was a trunk-problem and thus was fixed in trunk too.

@Roujin:
Thanks for letting me know about the inclusion of your work. My patch did not got broken by your work so while I saw your discussion about it on IRC, I would not get forced to change my code by any reject or so. So thanks for notifying me about the inclusion of your work.

When working on using your framework it seams like you never implemented the idea of not needing to rebuild the sign list when changing filter. So it seams that the biggest change is that instead of checking in the for-loop of rebuild-function which signs to add, with this framework one has to define one function for each filter type. Since this function is static there need to be one case insensitive and one case sensitive filter function. You probably had your reasons for dropping your initial ideas, but as those is all I heard about this framework it was a bit confusing at the start when I had the wrong idea of what it was doing.


So here is a next version of the Filter Sign List patch that have been brought up to trunk* and adopted to the new filter framework to the extent I have found reasonable.

filter_sign_list20_r15406.patch

Applies to r15406. Also contains widget focus patch version 28.

(36.48 KiB) Downloaded 99 times

* = All changes needed to make it apply to current trunk was made in the Widget Focus patch, version 27.

[Roujin]

I am sorry that I got you confused. Indeed I did not (yet?) implement some sort of backup so the list does not have to be rebuilt when you want to change the filter. That's because a) some dev(s) suggested otherwise, i.e. like it is now, b) I faced some difficulties while coding in that direction, and c) I thought, let's just get it working like this, backing up the list so one doesn't have to rebuild can still be introduced at a later stage.


I did not understand the part about two filters, one case-sensitive and one case-insensitive. I will have a look at your patch to try to understand what you mean
edit: ah, I see, you offer a button so select if the filter will be case sensitive or not. Indeed since the filter function is static you cannot use some members of your window. But I think you can change the second parameter your filtering function gets from the default char* to something like pair<char*, bool>.

[Roujin]

I did some changes similar to what I said in my last post (that remove the need to use two different Filter callback functions) to your patch.
Instead of std::pair I used a struct FilterInfo though that consists of the const char * and a bool.

No functional changes to your patch, only code-wise.

By the way, I really like the widget focus stuff, but are both patches available individually? Because I think that would increase chances of inclusion to trunk