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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/left closebox #10133

Open
wants to merge 7 commits into
base: develop
from

Conversation

@tassaron2
Copy link

tassaron2 commented Oct 23, 2019

I play OpenRCT2 on a Linux desktop with window controls on the left (customized KDE). Having the close button in the right of windows within the game is mildly inconvenient because of this. So to help get myself familiar with the code of OpenRCT, I've tried to implement a left-closebox feature. It works but uses a cmake option so it's only useable if you re-compile the game (although it could be the default option for macOS where left-closebox is guaranteed).

I think it would be better as a runtime choice instead of compile-time (so add an --option to the commandline interface of orct2 instead of cmake). But doing that would require more substantial changes to the source code (I think) and I'm not sure if it's a good idea or not. Feedback on this? I am willing to rewrite my pull request if the feature is valid and someone can tell me how they'd prefer it was done. I don't have much experience with C++ but I'll do what I can to help this fantastic project 馃榾 (Playing RCT natively on Linux is a dream come true.)

I've edited every window except for the multiplayer windows, but I will do the multiplayer windows soon once I've had a chance to play the multiplayer mode. Here's a screenshot of the game with a bunch of windows open:
leftcloseboxrct2

@AaronVanGeffen

This comment has been minimized.

Copy link
Member

AaronVanGeffen commented Oct 23, 2019

This is a feature that was first requested a long time ago: #2468. Nice to see someone picking it up.

Having said that, I'm not a big fan of #define-ing this sort of thing with a compile-time switch. I would prefer the buttons were moved depending on a config setting in, say, the window invalidate event.

Other team members might have more suggestions, though. Don't just take my opinion and change everything. ;)

@Gymnasiast

This comment has been minimized.

Copy link
Member

Gymnasiast commented Oct 23, 2019

Having a runtime way to switch (with, on Linux, intelligent to detect the default) would be better. But on the other hand, this is better than what we have and if modifying the code to allow for it takes too much effort, I think this could work as well.

If we go down that route, I do prefer a more sophisticated define for the title bar, though, rather than the if-else construction the PR currently uses. The same applies to resizing. Basically, I think one should be able to assume that if the positioning works with right-x, it will also work on left-x.

Like Aaron, I'm interested in hearing what the other team members think.

@IntelOrca

This comment has been minimized.

Copy link
Contributor

IntelOrca commented Oct 23, 2019

This is a prime example of how inflexible the window system is, and why no one has yet done it. I have attempted a few window system re-writes in the past but they are huge undertakings. All windows repeat the same code for the same things including but not limited to the title bar and close button. As @AaronVanGeffen suggested, the cleanest way to go about doing this with the current system is to modify the window shim on the fly in the invalidate event thus allowing you to add a runtime setting. There is already a shared method for resizing the shim widgets to the size of the window, the same method could also place the close button on the desired side.

@ocalhoun6

This comment has been minimized.

Copy link

ocalhoun6 commented Oct 24, 2019

Seems like the kind of thing that would make a good plugin?

@Gymnasiast

This comment has been minimized.

Copy link
Member

Gymnasiast commented Oct 24, 2019

@ocalhoun6 That would still require extensive work to provide hooks for this kind of thing.

@tassaron2

This comment has been minimized.

Copy link
Author

tassaron2 commented Oct 24, 2019

I did notice that most windows had functions with invalidate in the name which would reset the position of the widgets. There are a few windows with anchor_border_widgets functions which appear to do the same thing. But there are also windows which have neither function, such as the save prompt. What would be the best option there? Should all windows have anchor_border_widgets or invalidate? I'm not sure what 'invalidate' actually means in relation to windows, so I personally prefer the first name, but I'm probably missing something here.

There is an InterfaceConfiguration struct which should probably have the use_left_closebox boolean if it's going to be a runtime option. But I'm not sure how to add a new option into this area yet... I might be able to figure it out myself. As I said, I'm inexperienced with C++ 馃槄 But I'm willing to do the tedious editing of 50+ windows in my free time if I can figure out how to get started.

@AaronVanGeffen

This comment has been minimized.

Copy link
Member

AaronVanGeffen commented Oct 24, 2019

Should all windows have anchor_border_widgets or invalidate?

The two are not mutually exclusive. The former is a static function called by the latter. Consider anchor_border_widgets a helper function rather than an event, like invalidate.

As for the name, I agree invalidate is a little confusing. For OpenLoco, we ended up renaming this same event to prepare_draw instead.

Feel free to join us on Gitter for development questions, by the way :)

@Gymnasiast Gymnasiast self-assigned this Oct 25, 2019
#ifndef LEFT_CLOSEBOX
// close button on right of window
#define WINDOW_SKELETON \
{ WWT_FRAME, 0, 0, WW - 1, 0, WH, 0xFFFFFFFF, STR_NONE }, \

This comment has been minimized.

Copy link
@Gymnasiast

Gymnasiast Oct 30, 2019

Member

This is indeed roughly what I had in mind. I'd like to see two things changed:

  • WINDOW_SKELETON should be a "function" that takes the window name, width and height: #define WINDOW_SKELETON(title, width, height) \ and change the body accordingly.
  • On the old WIDGETS_MAIN, we deliberately left out the first and last brace, so that we could use { WIDGETS_MAIN } in our window definitions. Please replicate that.

When you apply these changes, the window definitions themselves will look like this:

static rct_widget window_staff_overview_widgets[] = {
    { WINDOW_SKELETON(STR_STRINGID, WW, WH) },
    { WWT_RESIZE,   1, 0,       WW - 1,     43,         WH - 1, 0xFFFFFFFF,             STR_NONE },             // Resize
Copy link
Member

Gymnasiast left a comment

This PR needs rebasing. I don't know how familiar you are with Git, though, so if you help please ask.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can鈥檛 perform that action at this time.