Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Window tries to snap to invisible window #5382

Closed
DorpsGek opened this issue Dec 5, 2012 · 7 comments
Closed

Window tries to snap to invisible window #5382

DorpsGek opened this issue Dec 5, 2012 · 7 comments
Labels
component: interface This is an interface issue flyspray This issue is imported from FlySpray (https://bugs.openttd.org/)

Comments

@DorpsGek
Copy link
Member

DorpsGek commented Dec 5, 2012

jvlomax opened the ticket and wrote:

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)

Reported version: trunk
Operating system: All


This issue was imported from FlySpray: https://bugs.openttd.org/task/5382
@DorpsGek
Copy link
Member Author

DorpsGek commented Dec 9, 2012

Zuu wrote:

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.


This comment was imported from FlySpray: https://bugs.openttd.org/task/5382#comment11746

@DorpsGek
Copy link
Member Author

DorpsGek commented Dec 9, 2012

Zuu wrote:

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.


This comment was imported from FlySpray: https://bugs.openttd.org/task/5382#comment11747

@DorpsGek
Copy link
Member Author

oafw wrote:

Kind of patch.
r24916.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/5382#comment11872

@DorpsGek
Copy link
Member Author

Zuu wrote:

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.


This comment was imported from FlySpray: https://bugs.openttd.org/task/5382#comment11873

@DorpsGek
Copy link
Member Author

oafw wrote:

r24919.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/5382#comment11874

@DorpsGek
Copy link
Member Author

DorpsGek commented Feb 4, 2013

Zuu wrote:

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


This comment was imported from FlySpray: https://bugs.openttd.org/task/5382#comment11964

@DorpsGek DorpsGek added component: interface This is an interface issue flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) bug labels Apr 7, 2018
@TrueBrain
Copy link
Member

I think this is widely over-complicating a situation that needs no solution. I understand it can be frustrating, but I am going to pass on this :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: interface This is an interface issue flyspray This issue is imported from FlySpray (https://bugs.openttd.org/)
Projects
None yet
Development

No branches or pull requests

2 participants