Proposal to turn off/on bird sounds

[okromario]

Hi there,

here is a proposal to turn on/off the birds sounds from the game settings menus (in-game & main settings). The new setting is also reflected in the config file as boolean with the name bird_sounds. This PR should resolve issue #1806. 💪

I tried to fit in my changes as good as possible in the UI and code as well.

Here are some screenshots of the changed UI windows in 800x600 with 100% UI scaling:

In-Game Settings:

I placed the new bird toggle button between the effects and the music buttons/sliders. I had to grow the settings window slightly to fit in the new button. I couldn't use the existing free space, because this space is reserved for a future Load button according to the code.

Main Settings:

I reversed the effects and music buttons/sliders and placed the new bird switch buttons between the effects and music buttons.

I tested it also with my full resolution at 3440x1440 with 200% UI scaling and it works fine.

Issues:

• I don't find a possibility to translate the new UI text into another language, maybe someone can tell me pls, how to do it, if needed.
• If the new switch buttons are used in the main settings, the new state isn't reflected in the in-game settings menu, but it actually works. Other way around, a state change in the in-game settings are reflected in the main settings. I don't know why and how to fix it.

Don't be surprised by my 1st commit, I had to fix the "NotificationManager.h" with a oneliner first, so I was able to build the project successfully.

Pls let me now, if I missed something. 😊

[Flamefire]

Great work thanks! Just a few small remarks.

I don't find a possibility to translate the new UI text into another language, maybe someone can tell me pls, how to do it, if needed.

This is done in the https://github.com/Return-To-The-Roots/languages repo but needs updating the language files after the code changes which is a bit involved. I did that update based on the current state of the PR, so you can create a PR in that repo with translations inside the respective files if you want. After the merge of that you need to update the submodule here:

cd external/languages
git checkout master
git pull
cd ..
git commit languages

If the new switch buttons are used in the main settings, the new state isn't reflected in the in-game settings menu, but it actually works. Other way around, a state change in the in-game settings are reflected in the main settings. I don't know why and how to fix it.

You forgot to set the state of that checkbox, see inline comment for fix.

Don't be surprised by my 1st commit, I had to fix the "NotificationManager.h" with a oneliner first, so I was able to build the project successfully.

Using GCC 15? We merged a fix for that same thing with #1814, so no issue with that. :-)

[okromario]

Hi @Flamefire, many thanks for your answers and explanations.

This is done in the https://github.com/Return-To-The-Roots/languages repo...

aah sry, I didn't checked the other repos for that. Thx for updating the base. I will add the german translation soon. I could add it for more languages, but only translator assisted.

Do I have something to re-generate at the languages repo to reflect my new changes, before I can start with my translation?

Using GCC 15? We merged a fix for that same thing with #1814, so no issue with that. :-)

yes GCC 15. Ah I see the commit now. My local master branch wasn't up-to-date during branching.

Additionally I found outdated current years at the licence header at the files I touched. Should I correct this as well?

                        vvvv
// Copyright (C) 2005 - 2021 Settlers Freaks (sf-team at siedler25.org)

Would it be an idea to sort the include lines in alphabetical order, at least at my touched files?

[Flamefire]

yes GCC 15. Ah I see the commit now. My local master branch wasn't up-to-date during branching.

No worries, it was very recent indeed.

Additionally I found outdated current years at the licence header at the files I touched. Should I correct this as well?

                    vvvv

// Copyright (C) 2005 - 2021 Settlers Freaks (sf-team at siedler25.org)

You can do that for the files you changed, yes

Would it be an idea to sort the include lines in alphabetical order, at least at my touched files?

This is actually already the case and the linter (clang-format based) enforces this. See the failed check:

 --- libs/s25main/ingameWindows/iwOptionsWindow.cpp	(original)
+++ libs/s25main/ingameWindows/iwOptionsWindow.cpp	(reformatted)
@@ -9,6 +9,7 @@
 #include "Settings.h"
 #include "SoundManager.h"
 #include "WindowManager.h"
+#include "controls/ctrlCheck.h"
 #include "controls/ctrlImageButton.h"
 #include "controls/ctrlProgress.h"
 #include "drivers/AudioDriverWrapper.h"
@@ -20,7 +21,6 @@
 #include "iwTextfile.h"
 #include "ogl/FontStyle.h"
 #include "gameData/const_gui_ids.h"
-#include "controls/ctrlCheck.h"

However the order follows a certain pattern: 1. own includes, 2. third-party includes, 3. stdlib includes. And they are sorted within each group.
You can run the "clangFormat" target (make clangFormat if I remember the spelling correctly) if you have clang-format-10 installed locally to automatically format sources including the sorting.

Do I have something to re-generate at the languages repo to reflect my new changes, before I can start with my translation?

After adding/removing translatables from the source the template (*.pot file) needs to be regenerated using poedit and that reflected in the individual translation files (*.po) which I did.
With that done you can now pull the new branch, add translations and open a PR for that repo similar to this one. When this is merged the submodule update can be added in this repo to include it.
Only add translations where you are reasonably sure they are correct. Untranslated strings show up in several tools so they are easy to find and English strings are still readable for most. So if in doubt rather leave that.

[okromario]

Hi @Flamefire, here we go, I'm done. The diff looks not very review friendly, but I can't do anything about it I guess.

The in-game settings menu has now the same placement pattern as the main settings menu (desktops). The placement is only slightly different as before, but better as before in my opinion and the spacing is now standardized.

I gave the (still hidden) Load button now it's own IDs for future use and it's still prepared to be enabled and positioned correctly together with the Save button with ease, if the load function get's implemented. Just comment in the code for the Load button (row 103-105) and remove 2 lines of code marked for this purpose (row 107 & 110) and that's it. Nothing to recalculate or playing with variables. 💪

In the screenshot you see the 1st dialog was the prev. one, the 2nd the (current checked in) one and the 3rd one with enabled Load button. The whole window size remained unchanged.

Pls let me know, if I need to revise anything or if I have overlooked something.

[Flamefire]

Awesome, thanks!

I only have 2 suggestions for naming changes and I'd define offsets in terms of sizes which is a major strength of this way of layouting: If the size changes the controls will move accordingly.

And a small layout change that wasn't easily possible before: The bird sounds should be closer to the effects volume than to the next control to make it visually clear that they belong together: The volume of the birds are also affected.
Similar for the music player.
What do you think about that?

Also thanks for attaching screenshots. That makes it much easier to review!

[okromario]

Hi @Flamefire, ah you are right, I don't considered the sizes of the controls 🫣. Thx for your suggestions, I have implemented your changes, removed now unused spacing constants, but added 1 for the general spacing of the 5px (because it's used on many places and I guess it's convenient to have a general spacing one).

In the screenshot above your see now the 1st dialog as the prev. one again, the 2nd one with my new changes and the 3rd one same but with enabled load button.

I hope I did it well. Pls let me know again, if there is something more. 💪

[Flamefire]

There is a duplicate include now which triggers the linter. Merge master and run clang-format to get rid of it.

but added 1 for the general spacing of the 5px (because it's used on many places and I guess it's convenient to have a general spacing one)

Good idea! 👍

I would use that for the checkbox&button too. Can you see how that looks and if you like it apply it? My idea is that this a) keeps it even closer and b) avoids using another spacing (effectively now: imgButtonSize - optionSizeSmall)

[okromario]

Hi @Flamefire, ah sry this include was my own fix from my 1st commit, that is of course now redundant. I reverted this commit now.

I implemented your suggestions. It looks like following:

The 1st dialog is the prev. one, the 2nd your suggestion with "optionSizeSmall" and the 3rd one with "optionSizeBig".

The bird/music player button looks now to cramped to me, "overlapping" the image button on the left side, so I did the (3rd dialog) one with "optionSizeBig.y" that is 2px larger and then there would be no "overlapping" anymore, but the spacing between these buttons and the buttons at the bottom isn't the same anymore.

Here is a screenshot in detail:

What spacing do you prefer? I like the "optionSizeBig" a bit more, but I will do as you like. I committed the big one currently.

[okromario]

ah because there is now more space at the botton, I could reduce the window size to the original value again, or?

[Flamefire]

What spacing do you prefer? I like the "optionSizeBig" a bit more, but I will do as you like. I committed the big one currently.

I'm fine with either one. Having the top and bottom align is also nice.
However the position calculation looks odd: I'd expect curPos.y += imageButtonSize.y not optionSizeBig which is only used for the buttons at the bottom (maybe should be renamed accordingly?)
I guess imageButtonSize.y + generalSpacing + optionOffset.y only happens to yield the same result.
So I guess the easiest way would be to revert to curPos.y += imageButtonSize.y and use curPos + Offset(optionOffset.x, 0) for the placement. Is that correct?

ah because there is now more space at the bottom, I could reduce the window size to the original value again, or?

Sure why not. Maybe such that the space between the last button and the bottom is the same as between the buttons? Or twice that? Just check what looks good to you and use that. It isn't worth to make that too exact, just a little less than after the current moves.

[okromario]

Hi @Flamefire, ahaaa now I know why imageButtonSize.y led to a too large gap that prevented me to do so with the imageButtonSize.y alone. Many thanks for pointing that out. I hope this is okay now.

I corrected the calculation and reduced the vertical size of the dialog. The spacing is only very slighly different.

The screenshot shows the same again, prev., current and current with enabled save button.

[Flamefire]

Looks good, thanks a lot for all the work :-)