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

Allow excluding windows from being closed via Escape key #1606

Merged

Conversation

falbrechtskirchinger
Copy link
Contributor

@falbrechtskirchinger falbrechtskirchinger commented Jul 16, 2023

Add a "pin" function to in-game windows. The function replaces the shade (aka. minimize) button and the window can instead be shaded by double-clicking the title bar.
Pinned windows are excluded from being closed by the window manager when hitting the Escape key.

The setting interface.enable_window_pinning has to be set to 1 for the new behavior to take effect.

An option can be added to the UI once the options screen has been reorganized.

To-do:

@Flow86
Copy link
Member

Flow86 commented Jul 16, 2023

The feature has poor discoverability. Should it be added as a separate button instead?

adding a small "pin" button left of the minimize button of windows would be great.

@Spikeone
Copy link
Member

Although one should be able to disable the pin button (for people who like the original interface). Will think about a pin icon.

@falbrechtskirchinger
Copy link
Contributor Author

FYI, this is what the button looks like at the moment:

pin_window_button

(A reddened hover state of the corner button.)

@falbrechtskirchinger
Copy link
Contributor Author

I've updated the PR description with the new mechanic, developed after some back-and-forth with @Spikeone on Discord.

To summarize the rationale: An additional button would've been difficult to fit on some windows (e.g., HQ) and required new assets for the top part of the window chrome. Once we add a setting to the options screen – with a tooltip explaining the behavior – this should be discoverable enough.

The pin/unpin icons are a bit difficult to tell apart (at least to my eyes and without #1594 😉) and @Spikeone created another set of icons using a lock, which is visually easier to distinguish. We could rename the feature and change the icons as well.

@Flow86 Thoughts?

Otherwise, work is paused until after #1608 and #1609 have been (ideally) merged.

@falbrechtskirchinger
Copy link
Contributor Author

For easier viewing, these are all the new assets at 400%:
resource_new

Commands used for future reference:
scale=4
pad=4
s=$((16*scale + pad*2))
for f in *.bmp; do magick "$f" -filter point -resize $((scale*100))% -background white -gravity center \
                               -extent ${s}x${s} "${f%.bmp}.png"; done
magick montage -background white -tile 2x3 000*.png 003*.png 001*.png 004*.png 002*.png 005*.png
               -mode Concatenate resource_new.png

libs/s25main/Loader.cpp Outdated Show resolved Hide resolved
@falbrechtskirchinger
Copy link
Contributor Author

I want to add a new category called [tweakables] to the settings. Intended for all the little knobs like enable_window_pinning etc. This would be useful for #1604 as well.
Thoughts anyone?

@Flamefire
Copy link
Member

I want to add a new category called [tweakables] to the settings. Intended for all the little knobs like enable_window_pinning etc. This would be useful for #1604 as well.

What would you put there from the other PR? IMO the interface category is fitting, i.e. similar to revert-mouse

@falbrechtskirchinger
Copy link
Contributor Author

falbrechtskirchinger commented Jul 25, 2023

I want to add a new category called [tweakables] to the settings.

What would you put there from the other PR? IMO the interface category is fitting, i.e. similar to revert-mouse

One or two settings for the "Follow object" changes. @Spikeone was arguing for some control to retain the vanilla experience.

I'm fine with those going into [interface] but was thinking ahead a little. Might be nice to have particularly granular stuff in one place. Just a thought.

Edit: Tagged the wrong person.

@spikeon
Copy link

spikeon commented Jul 25, 2023

I believe you meant to tag @Spikeone i am unfamiliar with this repo

@falbrechtskirchinger
Copy link
Contributor Author

I believe you meant to tag @Spikeone i am unfamiliar with this repo

My apologies! You're correct.

@Spikeone
Copy link
Member

What would you put there from the other PR? IMO the interface category is fitting, i.e. similar to revert-mouse

I think the idea to put multiple settings into that screen, maybe some that don't change the interface but other behaviors (I have no idea at the moment what settings could be client side only and change behavior, and are not interface options). Maybe at some point we'd split tweakables and interface again.

@falbrechtskirchinger
Copy link
Contributor Author

Oh, the original purpose of the [tweakables] category came back to me. I wanted to suggest putting some of the things that are now hardcoded constants there. For example, the (not yet implemented) pick radius in pixels for #1604.
All these values would also be perfectly at home in the [interface] category, I just thought it might be useful to separate out the smaller things, before [interface] gets crowded. 🤷‍♂️

Copy link
Contributor Author

@falbrechtskirchinger falbrechtskirchinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides two minor things and a final decision on [tweakables], this seems ready to me.

libs/s25main/ingameWindows/IngameWindow.cpp Outdated Show resolved Hide resolved
libs/s25main/Settings.h Show resolved Hide resolved
@falbrechtskirchinger falbrechtskirchinger marked this pull request as ready for review July 27, 2023 18:21
@falbrechtskirchinger falbrechtskirchinger force-pushed the pin-windows branch 3 times, most recently from 0b78f3a to 7bff721 Compare July 29, 2023 06:49
Flamefire
Flamefire previously approved these changes Jul 31, 2023
@Flamefire Flamefire enabled auto-merge July 31, 2023 07:24
falbrechtskirchinger and others added 4 commits August 1, 2023 05:56
Pinned windows won't be closed when pressing Escape.
Add a "pin" function to in-game windows. The function replaces the shade
(aka. minimize) button and the window can instead be shaded by
double-clicking the title bar. Pinned windows are excluded from being
closed by the window manager when hitting the Escape key.
auto-merge was automatically disabled August 1, 2023 04:00

Head branch was pushed to by a user without write access

@falbrechtskirchinger
Copy link
Contributor Author

The required test runs for the auto-merge seem to have been canceled so I rebased onto master and force-pushed over your merge commit to trigger another run. There's one cosmetic change I forgot to push previously, otherwise, it's unchanged.

When the (random) map size happens to be equal to the chunksize the test
fails as the first mock-send finishes the transmission.
Use named constants and ensure sizes.
@Flamefire
Copy link
Member

The appveyor build failed due to a random issue unrelated to this. I added a commit to fix this to your branch so you don't need to do anything.

@Flamefire Flamefire enabled auto-merge August 1, 2023 07:37
@Flamefire Flamefire merged commit a397539 into Return-To-The-Roots:master Aug 1, 2023
@falbrechtskirchinger falbrechtskirchinger deleted the pin-windows branch August 1, 2023 10:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants