Return-To-The-Roots / s25client

Return To The Roots (Settlers II(R) Clone)
http://www.rttr.info
GNU General Public License v2.0
470 stars 75 forks source link

Allow excluding windows from being closed via Escape key #1606

Closed falbrechtskirchinger closed 11 months ago

falbrechtskirchinger commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

falbrechtskirchinger commented 1 year ago

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

pin_window_button

(A reddened hover state of the corner button.)

falbrechtskirchinger commented 1 year ago

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 :wink:) 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 commented 1 year ago

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

Commands used for future reference: ```sh 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 ```
falbrechtskirchinger commented 12 months ago

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 commented 12 months ago

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 commented 12 months ago

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 commented 12 months ago

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

falbrechtskirchinger commented 12 months ago

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

My apologies! You're correct.

Spikeone commented 12 months ago

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 commented 11 months ago

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. :man_shrugging:

falbrechtskirchinger commented 11 months ago

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.

Flamefire commented 11 months ago

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.