Feature: Add alternative setting for right-click close window option to exclude pinned windows

[MasonGulu]

Motivation / Problem

The right-click to close windows option is a great gameplay improvement, however it makes it very easy and common to close windows you don't intend to.

Description

My solution to this issue is to change the right-click to close windows setting to be a dropdown rather than a simple toggle, with a third option to exclude pinned windows. When set to this third mode, simply pin a window and right-click will no longer close the window.

Limitations

Since I changed the type of the config option from a bool to a uint8 the game will give you a warning upon launching if you last saved settings from a version that did not make this change. This window can be easily dismissed and as far as I can tell there are no negative effects from this.

I've been looking into solutions for this, but modifying the setting loading code to smoothly change this format to the new type is not a satisfactory solution.

I'm also not very satisfied with my if condition in window.cpp, it's quite long, but I'm not sure of a better way to split it.

Checklist for review

Some things are not automated, and forgotten often. This list is a reminder for the reviewers.

• The bug fix is important enough to be backported? (label: 'backport requested')
• This PR touches english.txt or translations? Check the guidelines
• This PR affects the save game format? (label 'savegame upgrade')
• This PR affects the GS/AI API? (label 'needs review: Script API')
  • ai_changelog.hpp, gs_changelog.hpp need updating.
  • The compatibility wrappers (compat_*.nut) need updating.
• This PR affects the NewGRF API? (label 'needs review: NewGRF')
  • newgrf_debug_data.h may need updating.
  • PR must be added to API tracker

[LC-Zorg]

Player #623071 review: For my way of navigating the game interface, this would be a redundant option, although I do occasionally close a window I wanted to keep open. As the new behavior would be an added option, not a replacement, I have nothing against. Ok, just one thing: the red/green button is more visible, but I'm not convinced that's very important. However, I would have two suggestions.

1. Please make right-click closing the default setting. I fully agree that this is a great gameplay improvement. It makes the game so much easier, I can't see any compelling arguments against it, so why shouldn't new players be able to take advantage of it? :)

2. In the case of setting where clicking on a pinned window doesn't close it, maybe a good solution would be to allow it to be closed using Ctrl + click?

[MasonGulu]

I don't really have any authority to change default behavior, I'm just adding another option to help with accessibility.

I could add a modifier key to close the window, but I feel like at that point it's just as easy to unpin the window and close it. Adding a modifier key also increases the complexity of documenting this, would I hardcore the modifier, or would I use an existing keybind?

I'd like to receive some more input before trying to learn how user keyboard and keybind input works. But thank you for the feedback!

[MasonGulu]

I have addressed the changes requested. I moved the enum out of it's own file, and below the other few setting enum types in settings_type.h. I appended RCC_ like requested, and I simplified the enum names with that additional prefix. Additionally I simplified the no option related variable names.

[MasonGulu]

I've changed the enum type.

Is the setting type warning something that needs fixed/mitigated?

Implemented

[2TallTyler]

Is the setting type warning something that needs fixed/mitigated?

I think so, yes. Most players don't know that the warning is harmless, and being greeted by a warning immediately upon opening a newly-updated game release is bound to result in bug reports, complaints, and a bad taste in the mouth of some players.

How to actually fix that, I don't know. 🤷

[MasonGulu]

I am looking into changing the setting type to SDT_OMANY so I can define a load function to handle conversion.

How do I fix the last commit name? The coding style guide doesn't mention a commit name for catching up to master.

[2TallTyler]

Merge commits shouldn't be included. You'll need to rebase and force push.

[2TallTyler]

@MasonGulu: Still interested in working on this? I would suggest asking in the #openttd-development channel in the official Discord server if you aren't sure how to update the setting to avoid the error message.

[MasonGulu]

I am still interested, thank you for the recommendation!

[MasonGulu]

I have resolved the warning on first launch issue by renaming the option to right_click_wnd_close from right_mouse_wnd_close. This has the added benefit of not interfering with older game versions, in cases when the config is shared between different instances.

[2TallTyler]

Try an interactive rebase (git rebase -i head~2 to edit the last two commits, then fixup the latest commit and force-push) to squash that commit and make commit checker happy 😉.

[2TallTyler]

Looks good to me. Thanks for the contribution!

(Since you haven't done this before: from here it waits for another developer to look it over and either request changes, or agree that it's suitable for inclusion in OpenTTD and hit the Merge button 🙂.)

[TrueBrain]

Sorry, I think @2TallTyler jumped the gun here. A few more things to pick up here :)

[MasonGulu]

That should be all my references to "pinned" swapped out for "sticky", rebased on the latest master branch, and with the config loading code provided

[TrueBrain]

Tnx again for your work on this, and your patience while we figured out what we actually wanted :)