window focus

[tnt oss]

I think it would be handy if when you click on a window, it should come to the top.
So I wrote this code to do it, I don't know though if it is the correct solution, so i'd like some feedback on it.

this is the code that focuses a window:

     /* Move the clicked window to the top. */
     if (this->current_window != this->top && this->current_window->wtype != WC_MAINDISPLAY) {
         /* Remove window from the list. */
         if (this->current_window->higher != NULL) {
             this->current_window->higher->lower = this->current_window->lower;
         }
         if (this->current_window->lower != NULL) {
             this->current_window->lower->higher = this->current_window->higher;
         }
 
 
         /* Add window to the top of the list. */
         this->top->higher = this->current_window;
         this->current_window->lower = this->top;
         this->current_window->higher = NULL;
         this->top = this->current_window;
 
         /* Force redraw of our window. */
         this->current_window->MarkDirty();
     }

The patch for version r787 is in the attachment.

[Alberth]

Sorry for not looking into your other patches, but I am concentrating on getting coaster building working at the moment.

About your patch:

Under the assumption that the pointer manipulations are done correctly (maybe removal and re-insertion could be refactored to functions?), I think putting the window towards the top when clicking it is a good idea.

I am missing use of the GetWindowZPriority function from windows.cpp which gives each window a relative priority, higher numbers are always above lower numbers. (A window can thus raise until it is the top window, or until there is a window with a higher number directly above it.)
Currently it just keep the main display at the bottom, but in the future we could have error windows, pop up windows, or tooltips or so.

[tnt oss]

Perhaps using the std::list? It is implemented as a double linked list. Then you don't have to worry about dangling pointers. And about the GetWindowZPriority i'll look into that.

[tnt oss]

Added a function to move a window above another, and keep in mind the GetWindowZPriority.

[Alberth]

Hi,

Thanks for the improved patch that uses the Z priority.
It is not correct though, and it can also get much further simplified, after a more detailed look at the WindowManager code.

I'll first give some comments on the current patch, mostly to show how you can code it smarter.
Then I'll reflect on what the patch is doing, and alternative ways to reach its functionality in a much more integrated way.


I realize my comments are very brief and can be read as very harsh, but that's just my style of commenting.
If they are harsh in any way, they are harsh only to the patch itself, never to you as person or as developer.
I very much appreciate that you take the time and effort to get the project forward.

(I know from experience that reading these comments for the first few times can really ruin your day. If they do, stop reading, and don't react on them while you are angry. Instead, let it sink in for a few days, reread them, and try to decide objectively whether I am talking rubbish or not for each point that I make. Adopt things that you think are good, ask for clarification or explain why you think I am wrong for the other things. It is perfectly possible that I am wrong on some things, or we can agree to disagree on some points :) )

The purpose of commenting your patch is to increase the code quality as much as possible. I also found that doing this made me grow as programmer because I got explained alternative (and often better) solutions, even though I did have a some bad days after someone else burned my patch to the ground.


>  /**
> + * Move window 'w' above window 'position'.
> + * @param w The window to move.
> + * @param position The window we have to put 'w' above.
> + */
> +void WindowManager::MoveWindowAbove(Window *w, Window *position)

We already have 'move window', which means moving in the (x,y) plane. To avoid confusion, a different name than 'move' is useful. Perhaps use RaiseAbove or so?

"position" makes no sense to me in this method, "below" seems better to me.

In the documentation, you can use \a (as in "\a w") to refer to arguments of the method.

Last but not least, if you generalize the method to also allow NULL for 'position' (meaning 'insert w at the bottom), the method is fully generic with very little extra work.

> +{
> +    if (w == position) {
> +        return;
> +    }

The doxygen text implies that w and position can never be the same window (a window cannot be above itself).
Silently ignoring violations like this means the program keeps running, but with faulty data (either w or position is not what it should be). Quite likely, it creates havoc in some weird way, and eventually crashes in some totally weird way 5 minutes later. 5 minutes later is however a *LOT* of statements to search for the cause of the error.

The alternative way is to crash the program in a controlled way, eg by

assert(w != position);

This is the earliest possible point where you can detect that something is not right. The sequence of statements to this point should be much shorter than in the above case.

> +    /* Remove our window. */
> +    if (w->higher != NULL) {
> +        w->higher->lower = w->lower;
> +    }
> +    if (w->lower != NULL) {
> +        w->lower->higher = w->higher;
> +    }

Code is fine. I'd write the 'then' statement after the 'if' and omit the curly brackets, but that's just personal style.

> +    bool is_top = (position == this->top ? true : false);

Are you aware that comparisons are functions that give a boolean result?

bool is_top = (position == this->top);

does exactly the same.

> +    if (position->higher != NULL) {
> +        position->higher->lower = w;
> +        w->higher = position->higher;
> +        w->lower = position;
> +        position->higher = w;
> +    }
> +    else {
> +        position->higher = w;
> +        w->lower = position;
> +    }

Since this->top->higher == NULL, the "if (position->higher != NULL) {" test is actually the same as testing !is_top. You can merge the "this->top = w;" code below into the 'else'.

> +
> +    if (is_top == true) {
> +        this->top = w;
> +    }

'if' takes a boolean value inside the parentheses, so you can just do "if (is_top) { ..."

> +    if (is_bottom == true) {
> +        this->bottom = w;
> +    }

This is wrong.
'position' is below w, so the latter is never at the bottom in your patch.

> +    /* Give the clicked window focus. */
> +    if (this->current_window->higher != NULL) {
> +        bool move_up = (GetWindowZPriority(this->current_window->wtype) >= GetWindowZPriority(this->current_window->higher->wtype) ? true : false);
> +        if (move_up) {

Use the condition immediately in the 'if' statement, and delete the 'move_up' variable entirely?

> +            /* Find new location for our window. */
> +            Window *position = this->current_window;
> +            while (position->higher != NULL && GetWindowZPriority(this->current_window->wtype) >= GetWindowZPriority(position->higher->wtype)) {
> +                position = position->higher;
> +            }
> +            this->MoveWindowAbove(this->current_window, position);

It looks a bit long to me, first test whether you can move up, and if you can, move up until you cannot move further.
If you always look for the new place to insert the window, and afterwards check whether it needs moving, the code is simpler (both tests get merged).
A 'while' that fails on the first try is much like an 'if' that fails.

> +            this->current_window->MarkDirty();

I was surprised about this one, and wondered whether you had forgotten something here.
In the end however, I concluded this is indeed the right solution.


>      void StartWindowMove();
> 
> +    void MoveWindowAbove(Window *w, Window *position);

This part of the patch triggered my "Move" comment, two functions both using "move", but with a totally different meaning.




So far about your current patch. I hope you understand what I meant to say. Feel free to open a discussion about my points if you think I am unclear, or when I seem to be missing some important point.



Now about the alternative solution.

When you look in the existing code, WindowManager::DeleteWindow does the remove-window part (and a bit more), and WindowManager::AddTostack inserts a window at the right point (in a somewhat different way). An alternative is thus to split the DeleteWindow in two pieces (namely, the 'remove window' part and 'delete w'), and use those two functions to move the window to the right point in the stack.

I agree it's less efficient in terms of CPU use, the 'add' function may walk through more windows than absolutely needed. On the other hand, there are not so many windows displayed (it should also implement automatically closing of windows at the bottom if it does not do that already), and clicking at a window is a human action, so it is not executed very often and CPU time is mostly irrelevant.

(Just now I see that "AddTostack" should be "AddToStack" (with uppercase S). Amazing how many silly mistakes you make :) )

[Alberth]

No way to change my post :(

Anyway, "(it should also implement automatically closing of windows at the bottom if it does not do that already)" means, that it should have this functionality at some point in the lifetime of the program. I was not implying that you would need to implement that now.

(if you would implement it, it should be a separate patch, imho)

[tnt oss]

I agree with all the comments you made. I Really appreciate the feedback since I don't have much experience in open source projects, this is very valuable to me.

[tnt oss]

I programmed it the way you said, and you are right, the code is 100x cleaner this way.
Btw could you tell me what to include in patches, for example should I include the 'AddTostack' -> 'AddToStack'  or some other typo/wrong comments. Or should it include just the code for raising a window?

[tnt oss]

Here is the new patch

[Lord Aro]

There seem to be a commented out if statement in that...

(hint: commented out code is a no-no)

[tnt oss]

Ye sry, those shouldnt be commented, just uncommented them if you want. I cant change it till monday.

[tnt oss]

Found some spare time, and fixed it. Thanks for the patience you guys have with me :p.
Btw, if this patch seems ok to you guys, should I post it in issue tracker?
And Alberth, could you explain what you mean with closing the bottom window?

[Alberth]

Looking good, I just committed the patch, thanks for your efforts.
Unfortunately, I forgot to credit you for r798. Sorry about that.

What  I meant with "closing windows at the bottom" is as follows:

Currently, we open windows as needed. When you do that for a while, you'll end up with a lot of opened windows very soon.
Thus it seems reasonable to add some mechanism to automagically close 'old' windows in some way.

[tnt oss]

Isn't that how it should be? Opening windows and let them open untill you close them?
I mean, I wouldn't like it if I opened some windows on my desktop, and my computer starts to close some of the old ones.

[Hazzard]

It could have a (hopefully user-set) limit on the number of windows.
It could have a timer on each window.
There might be some system to have different windows behave differently.

Or it could have nothing to automatically close windows.

[tnt oss]

Here are 2 patches, one that puts the raise window code in a function + some changes, and the other will raise a window when opening an already open window.

PS: Could you explain me the use of 'WindowNumber' in e.g 'GetWindowByType'? It looks like it should return all windows of a specific type when passed 'ALL_WINDOWS_OF_TYPE', but it just returns one.

[Alberth]

Thank you for the patches, I applied them in r801 to r803, with small changes.

Normal desktop applications are multi-document oriented, Word has a 'collection of documents' in itself, Firefox and many editors use tabs to switch between web-pages/files. It allows you to edit 15 files in one single window. This reduces the number of opened windows dramatically.
In the original tycoon programs, each ride and guest has its own window.

I cannot currently vision how it would work if you had a "rides" window and a "guests" window, and a 'tab' or whatever for each ride or guest. In the original you can have a number of ride windows open next to each other so it is easy to keep an eye on them. This seems like a useful idea to me. If you want to do that in a multi-document window like the normal desktop applications, it would probably mean (at least, I believe that at this moment) that you have to write a window manager inside a window.

The current implementation aims for the same idea as the original program, have separate windows instead. (Without considering other options mostly, so if you want a better solution, feel free to invent one :) )
This will lead to an exploding number of windows at the game screen (if you want to experience that yourself, play some OpenTTD, with a high 'max open windows' limit), which in turn lead to my statement that 'old' windows should be automagically closed in some way.

The separate windows also result in the WindowNumber parameter. When you open a window for a guest, and the same window for another guest, both windows have the same window-type. Without the window number, you can thus never find find and highlight the open window for the first guest.
(The same also holds for rides, like the shops currently.)

Besides the above kind of windows, there are also unique windows, like the financial overview, the toolbars at the top and bottom, etc. They don't really need a window number, but it's an obligatory parameter. The ALL_WINDOWS_OF_TYPE value matches with all window numbers (and thus all windows) so you can still find the unique windows.
In this case, perhaps a better name would be ANY_WINDOW_OF_TYPE, but I can imagine other cases, like broadcasting an event through OnChange where you really intend 'all windows of this window type' (perhaps that even works already). We could have 2 names, but the difference is so subtle that context seems enough to understand what is meant, I think.