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

Do not overlap vehicle view window with child windows (orders, timetable, details) #5451

Closed
DorpsGek opened this issue Jan 27, 2013 · 6 comments
Labels
component: interface This is an interface issue enhancement Issue would be a good enhancement; we accept Pull Requests! flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) stale Stale issues

Comments

@DorpsGek
Copy link
Member

RegnerusDux opened the ticket and wrote:

If I want to maintain a special vehicle with the orders/details buttons in the vehicle view window the new windows with orders and details always cover the view window and its buttons.
I have to close or move this window to get access to the buttons again (start/stop, goto depot, force turn, etc.). See screenshot 'current_situation.jpg'.

A better way would be to position the new window close beside the vehicle view window.

I tested it and changed the parent window class of the relevant window description (e.g. _timetable_desc in timetable_gui.cpp)
from WC_VEHICLE_VIEW to WC_NONE. See screenshot 'proposed_situation.jpg'.

The only disadvantage is that the new window sometimes is opened far away from its vehicle view window.

Attachments

Reported version: 1.2.3
Operating system: Windows


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

andythenorth wrote:

The window shuffling is annoying, but provides better handling of smaller screens. Not convinced that's enough to reject this suggestion though :)


This comment was imported from FlySpray: https://bugs.openttd.org/task/5451#comment12864

@DorpsGek
Copy link
Member Author

frosch wrote:

Patch by 3298 for this.

Comments copied from #6568:
- The next hunk is pretty large. At the start you see a few constants for the placement penalties. Feel free to tweak them, the documentation should be enough to tell you what they do.
- The next part of that hunk merges IsGoodAutoPlace1 and IsGoodAutoPlace2 into a single function. The difference between those functions was fairly small already, and with my scoring system the merged function tries to do it the way of IsGoodAutoPlace1, falling back to what once was IsGoodAutoPlace2 if the screen bounds check of IsGoodAutoPlace1 fails (while increasing the penalty, obviously). The loop at the end, which prevented placing a window on top of another, was changed to just count the overlapping windows and increase the penalty for each. I also removed your new toolbar_y stuff in favor of a simple condition in the overlap-checking loop (the toolbars have high Z priority; placement below windows with higher Z priority than the new window is now forbidden) - I see no reason to prevent placement next to the toolbar, which the old code did. While at it, I made the code ignore overlap with tooltips as well (those made for some weird placement before). The merged function returns the penalty instead of true/false now, and it needs a few more parameters.
- Still in the same hunk, GetAutoPlacePosition needs to keep track of the penalty of the best placement yet. This is passed by-reference to IsGoodAutoPlace alongside pt. The first placement attempt (top left, RTL-aware) now tries a true top-left position (next to the toolbar), only falling back to toolbar_y pixels lower if that fails.
The second and third attempts were merged like the functions they depended on. You'll notice a line dealing with window parents here - this is what persuades the game to place child windows next to parents by increasing the penalty if the window has a parent and this spot is not next to it, before the penalty-checking function IsGoodAutoPlace is even started (because that has no access to the parent, and it makes no sense to pass the parent reference down 8 times when the penalty can be calculated 1 time).
I kept the return statements in these attempts - they are triggered if the penalty is 0, which is a perfect spot. If the two attempts run through without a perfect result, I take the best one. Only when no spot worked (screen covered in higher-priority windows?), a third attempt is started, which simply places the window at (0, toolbar_y) so it can be moved / closed, at least when the movable higher-priority windows are out of the way. (Toolbar and statusbar are immovable, but using toolbar_y avoids the toolbar and keeps closebox and caption away from the statusbar.) This replaces the old fourth attempt, which went down-right for each subsequent window, completely ignoring the screen edges. I considered this final attempt impossible enough to put a console warning into it in case it was ever needed.
- At the end of the hunk, a small comment typo I noticed along the way is fixed.
- The final two hunks change LocalGetWindowPlacement so it passes a few more parameters down to GetAutoPlacePosition, and remove the child window special case (except for the child windows of the construction toolbars) so the penalty-based placement can take over, which favors spots next to the parent. That's #5451.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/5451#comment14651

@DorpsGek
Copy link
Member Author

3298 wrote:

Upon frosch's request via IRC, I've split the patch into the part adding the penalty framework, and the part that actually changes the placement of child windows (by increasing the penalty if a window has a parent, except when the parent is next to the attempted spot).

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/5451#comment14653

@DorpsGek DorpsGek added flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) Vehicles labels Apr 7, 2018
@andythenorth
Copy link
Contributor

andythenorth commented Apr 13, 2018

3298 is no longer working on this due to ethical concerns about github. https://www.tt-forums.net/viewtopic.php?f=33&t=83025

I would like to see this issue resolved, but I think it's unlikely to move now. It's also another example where localised fixes to the UI might add more long-term trouble by fragmenting the UI approach (see #6302).

Close it if no progress by, say, October 2018?

@andythenorth andythenorth added component: interface This is an interface issue enhancement Issue would be a good enhancement; we accept Pull Requests! and removed enhancement from FlySpray labels Apr 13, 2018
@andythenorth
Copy link
Contributor

Thanks for this. There's been no activity on this for some time, and as it stands, it doesn't look likely that it will go any further. Since OpenTTD moved to GitHub, we use pull requests rather than patches, as they are a much more productive workflow.

I'm planning to close this soon (in 7 days), as we try to keep the issue count low for OpenTTD, it helps us focus on things that are important and fun.

If anyone would like to continue with this patch, the best way would be to move the patch to your own GitHub fork, update it for the current OpenTTD master, and then create a pull request. For more information, please see our CONTRIBUTING.md.

We are also happy to discuss directly on the issue, or in #openttd irc, including help to get this into a pull request. Thanks for your contribution!

@andythenorth andythenorth added the stale Stale issues label Jan 8, 2019
@andythenorth
Copy link
Contributor

Per previous comment, stale, closing. Thanks!

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 enhancement Issue would be a good enhancement; we accept Pull Requests! flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) stale Stale issues
Projects
None yet
Development

No branches or pull requests

3 participants