FS#5382 - Window tries to snap to invisible windoe

Attached to Project: OpenTTD
Opened by Jørn Lomax (jvlomax) - Wednesday, 05 December 2012, 18:42 GMT
Last edited by Leif Linse (Zuu) - Sunday, 09 December 2012, 19:10 GMT
Type Bug
Category Interface
Status Confirmed
Assigned To No-one
Operating System All
Severity Very Low
Priority Normal
Reported Version trunk
Due in Version Undecided
Due Date Undecided
Percent Complete 0%
Votes 2
Private No


A window will try to snap to a window that is not visible.

To reproduce: go to the main menu and select "Check online content" When you have selected some new downloads and press download a window appears that show the progress of the downloads. When you move this window around it will try to snap to the main menu that is *behind* (i.e. not visible) the" online content window".

Expected behaviour: The download progress window should only try to snap to the topmost (only visible) window

Actual behaviour: The progress window tries to snap to a window that can not be seen

System info:
Fedora 17 x86_64
Openttd r24783 (compiled from source)
This task depends upon

Comment by Leif Linse (Zuu) - Sunday, 09 December 2012, 19:11 GMT
This can be reproduced not only in the main menu, but also in the game. All you need is three windows. A small one at the back. A larger one ontop and a third one to move around to test snapping against the window behind.
Comment by Leif Linse (Zuu) - Sunday, 09 December 2012, 19:21 GMT
Solving this for totally hidden windows only involves detecting which windows that are completely hidden, but for partially hidden windows that is not enough. In that case you want to detect if the part of a border that you try to snap against is hidden or not. Eg. it is still only comparing lines and rectangles, but the cases when a window is half-hidden is a bit more complicated than the completely hidden case.
Comment by Alexey (oafw) - Wednesday, 16 January 2013, 01:37 GMT
Kind of patch.
Comment by Leif Linse (Zuu) - Wednesday, 16 January 2013, 20:54 GMT
To me it looks like your patch have a lot of very similar code. Code which can probably be refactored and generalized.

You can probably also use the helper function IsInsideMM to reduce some code length. Using it you can check if a value is between a lower and an upper limit without having to expressing the value to check twice.

With respect to coding standard, if you write a statement on the same line as if (...), then don't use { and }. Example: if (something) DoSomething();

I have not yet analyzed what the patch does in detail other than that there is a large block of code which looks very similar for each case. Perhaps one idea is to move this block out to a separate function and let it take one or two parameters to decide which case it should operate on or use enough parameters to describe the actual values of each case which may need to be prepared differently for each case and then passed to your generic method. What is best depend on the situation and can need some experimenting to get right.
Comment by Alexey (oafw) - Thursday, 17 January 2013, 17:22 GMT
Comment by Leif Linse (Zuu) - Sunday, 03 February 2013, 23:44 GMT
+ * Checks border visibility between edges of dragging window.

Could you extend the explanation of what the method does. What does it mean to check border visibility between edges? It would help if you can explain the purpose of the method and what it does with 2-3 sentences. I would personally also wish to add a few more comments inside the method that describe the overall working of the method.

+ * @param is_horisontal True if snap to left or to right border.
+ * @param snap_to_left_or_top_border True if snap to left or to top border.

Instead of these rather unclear boolean variables, I would suggest an enum LEFT_BORDER, TOP_BORDER, RIGHT_BORDER, BOTTOM_BORDER or something similar.

+ int a; // upper border of gap
+ int b; // lower border of gap

Tabs are used for indentation, but not for aligning comments.

+ if (is_horisontal){

Insert a space before '{'. Note, that there are more instances of this issue in the patch. Just look for '){'.

+ v_edge = (snap_to_left_or_top_border) ? v->left : v->left + v->width;

The parentheses are not needed here and can be removed.