FEAT(client): Settings Profiles #6681

Hartmnt · 2025-01-05T19:39:27Z

This is very much WIP and I need to add a proper description, but it is functional on a fundamental level.

Feedback for both UI and settings refactor welcome

version3.mp4

Krzmbrzl

For reference, this implements #1264

One thing we need to think about/discuss is whether we believe whether all settings are created equal or whether there might be some settings that should be profile-dependent and some which are global-independent.
In the latter category, one might have things like the ask-on-quit setting.

If we believe that some settings should be independent of profiles, then the entire feature would become much more complex as this dependence would also have to be reflected in the UI in order to not make users guess whether a given setting depends on the profile or not 👀

src/mumble/Settings.cpp

src/mumble/Settings.h

Hartmnt · 2025-01-07T12:26:59Z

One thing we need to think about/discuss is whether we believe whether all settings are created equal or whether there might be some settings that should be profile-dependent and some which are global-independent.
In the latter category, one might have things like the ask-on-quit setting.

I am 100% in favor of "all settings are created equal".

The implementation complexity (and maintainability) would be a nightmare. We would need 2 settings versions and keep track of global vs non-global ourselves.
The user should decide what settings are important per profile. Maybe they want Ask-on-Quit profile specific 😆 And I imagine the outcry for "I can't set this setting per profile" will be much harder than "why don't you save this globally?".
The "Add" will actually duplicate the current profile, which should make it pretty easy to deal with this UX-wise from the user side. And in the worst case where you have to change every profile for a single setting: I don't see the benefit in making this so much more complex to implement just because the user will have to set like 5 checkboxes in that scenario...

@Krzmbrzl One thing that I am unsure about as of yet is "settings_version". That should be included in the individual profiles, but when we leave it out of the JSON root, we might - in rare, error caused circumstances - have users that might not be able to start Mumble... I/We have to come up with a nice solution for this

Krzmbrzl · 2025-01-08T09:33:46Z

I am 100% in favor of "all settings are created equal".

I'm also like 80% or 90% sure that this is likely the correct thing to do, so let's stick with that 👍

One thing that I am unsure about as of yet is "settings_version". That should be included in the individual profiles, but when we leave it out of the JSON root, we might - in rare, error caused circumstances - have users that might not be able to start Mumble... I/We have to come up with a nice solution for this

settings_version is meant as a version number for the format in which we store the settings. Therefore, it has to be a global property rather than a profile-specific one (unless you want to allow different profiles to use different syntax/semantics in which they are stored on disk, but that would likely just be a nightmare all around).

It is meant for scenarios such as

mumble/src/mumble/Settings.cpp

Lines 846 to 866 in c0eaa32

    
           // Compatibility layer for overtaking the old (now deprecated) settings 
        
           // This block should only be called once at the first start of the new Mumble version 
        
           // As echo cancellation was not available on macOS before, we don't have to run this compatibility 
        
           // code on macOS (instead simply use the new default as set in the constructor). 
        
           if (settings_ptr->contains("audio/echo")) { 
        
           	bool deprecatedEcho      = false; 
        
           	bool deprecatedEchoMulti = false; 
        
           	LOAD(deprecatedEcho, "audio/echo"); 
        
           	LOAD(deprecatedEchoMulti, "audio/echomulti"); 
        
           	if (deprecatedEcho) { 
        
           		if (deprecatedEchoMulti) { 
        
           			echoOption = EchoCancelOptionID::SPEEX_MULTICHANNEL; 
        
           		} else { 
        
           			echoOption = EchoCancelOptionID::SPEEX_MIXED; 
        
           		} 
        
           	} else { 
        
           		echoOption = EchoCancelOptionID::DISABLED; 
        
           	} 
        
           }

So with settings_version you could simply check the version and decide based on that how you have to parse the settings file, without having to do this awkward "always do the new format but also always the old in case the file is not yet updated" procedure. Similarly, if we rename a setting, this could cause the version to increase, though that is not (always) necessary as we have

mumble/src/mumble/JSONSerialization.cpp

Lines 152 to 172 in c0eaa32

    
           void migrateSettings(nlohmann::json &json, int settingsVersion) { 
        
           	// Perform conversions required to transform the given JSON into the format applicable to be read out by the most 
        
           	// recent standards 
        
           	// Check if the old ask_on_quit key exists and the new one does not exist within the json file 
        
           	if (json.contains("ask_on_quit") 
        
           		&& (!json.contains(static_cast< const char * >(SettingsKeys::QUIT_BEHAVIOR_KEY)))) { 
        
           		if (!json.at("ask_on_quit").get< bool >()) { 
        
           			json[SettingsKeys::QUIT_BEHAVIOR_KEY] = QuitBehavior::ALWAYS_QUIT; 
        
           		} else { 
        
           			json[SettingsKeys::QUIT_BEHAVIOR_KEY] = QuitBehavior::ALWAYS_ASK; 
        
           		} 
        
           	} 
        
           	if (json.contains("play_transmit_cue") 
        
           		&& (!json.contains(static_cast< const char * >(SettingsKeys::TRANSMIT_CUE_WHEN_PTT_KEY)))) { 
        
           		json[SettingsKeys::TRANSMIT_CUE_WHEN_PTT_KEY] = json.at("play_transmit_cue").get< bool >(); 
        
           	} 
        
           	(void) settingsVersion; 
        
           }

Finally, if not already implemented, we will probably want a hotkey to switch to a specific settings profile (and one to cycle through them?).

AvatarTheOld · 2025-03-31T14:40:58Z

Finally, if not already implemented, we will probably want a hotkey to switch to a specific settings profile (and one to cycle through them?).

This would be a really usefull addition to Mumble. At least for us, community of 30 people, one of the major things that stops us from fully migrating to Mumble from Teamspeak is inability to assign different Audio and Hotkey profiles. (we use said functions a lot, to the point that they become essential for us) Implementing both options, switch and\or cycle, would be very welcome.

Hartmnt · 2025-06-06T16:42:33Z

settings_version is meant as a version number for the format in which we store the settings. Therefore, it has to be a global property rather than a profile-specific one (unless you want to allow different profiles to use different syntax/semantics in which they are stored on disk, but that would likely just be a nightmare all around).

So with settings_version you could simply check the version and decide based on that how you have to parse the settings file, without having to do this awkward "always do the new format but also always the old in case the file is not yet updated" procedure. Similarly, if we rename a setting, this could cause the version to increase, though that is not (always) necessary as we have

This was easier said than done. A real brain twister, if you like 😆

I think I got it now, however we need two migrate methods (migrateProfiles in addition to migrateSettings) as the parsing of the latter is part of the former and we have to decide what to do with the profiles before creating the settings object. But this is the logical conclusion of using a global settings_version instead of per-profile I think. Phew...

Hartmnt · 2025-06-06T16:47:16Z

What do you think of the UI (frontend) part? I have considered different options such as creating a new MenuBar entry for profiles, but I think that's not a good approach. We could also create a dedicated sub-settings page. Or keep the current thing were there is just a drop-down in the main settings interface? Maybe hide the buttons in a burger menu?

src/mumble/Settings.cpp

src/mumble/Settings.h

Krzmbrzl · 2025-06-08T18:23:31Z

As for the UI: I think the current location of the profiles is a bit prone to be missed at first glance. I would prefer if it was a bit more "in-your-face" which profile is currently in use. We should definitely have it always visible. However, from my gut, I would put it at the very top of the settings page like this:

---------------------------------------
<profile info & selection>
---------------------------------------
<regular settings page as-is>

though that might end up looking awful xD

keep the current thing were there is just a drop-down in the main settings interface? Maybe hide the buttons in a burger menu?

yeah, that sounds reasonable to me (both of it)

Something that would be a really neat bonus was if inside the settings file we didn't duplicate every setting but have only one complete set of settings and then only store those settings in a profile that are different from those base settings. That would facilitate manual editing of the settings file. However, this is definitely not a requirement.

Hartmnt · 2025-06-23T15:15:21Z

I am having a hard time with implementing the shortcut. The main aspect, changing the profile, works. But each ConfigWidget, the things that map settings to UI elements, has a complex save routine which does additional things such as setting the needsRestart flag. All of this does not run in the current iteration of the shortcut.
I am not sure how to solve this properly, yet. I currently consider adding a ConfigDialog constructor which initializes all of the ConfigWidgets, but does not show its own UI in the end. Then calling the switchProfile method on that object before quietly closing and deleting it again. That would certainly work, but it feels kinda wrong. Does anyone have any other ideas?

Krzmbrzl · 2025-06-24T08:19:16Z

Couldn't you simply call the same function that is called when clicking Save or Apply in the settings GUI?

Hartmnt · 2025-06-24T10:08:04Z

Couldn't you simply call the same function that is called when clicking Save or Apply in the settings GUI?

Yes, but this requires the config widgets to be loaded which they only are if (and as long as) the config dialog is active.
I guess I can refactor the code to initialize the config widgets statically and not destroy them when closing the dialog. That way I could share the "validation" code even if the config dialog is closed 🤔

I'll look into that

Krzmbrzl · 2025-07-09T10:57:19Z

@CodiumAI-Agent /improve
(I'm looking into whether this might be a useful tool for us)

QodoAI-Agent · 2025-07-09T10:59:08Z

PR Code Suggestions ✨

Latest suggestions up to eadf494

Category	Suggestion	Impact
Possible issue	Insert profile by key not push_back Use direct key assignment into the JSON object instead of `push_back`, which is meant for arrays. This ensures the active profile entry is inserted with its name as the object key. src/mumble/Settings.cpp [162-163] profilesJSON.erase(profiles.activeProfileName.toStdString()); -profilesJSON.push_back({ profiles.activeProfileName, activeProfileJSON }); +profilesJSON[profiles.activeProfileName.toStdString()] = activeProfileJSON; Suggestion importance[1-10]: 9 __ Why: Replacing `push_back` with key assignment fixes incorrect JSON handling for object insertion and ensures the active profile is saved properly.	High
Possible issue	Use parsed version in migration Pass the `settings_version` you just read from the JSON instead of the global default. Feeding the correct version into `migrateProfiles` ensures that legacy data is migrated properly. src/mumble/JSONSerialization.cpp [241-249] void from_json(const nlohmann::json &j, Profiles &profiles) { - profiles.settings_version = 0; + int settingsVersion = 0; if (j.contains(SettingsKeys::SETTINGS_VERSION_KEY)) { - profiles.settings_version = j.at(SettingsKeys::SETTINGS_VERSION_KEY).get< int >(); + settingsVersion = j.at(SettingsKeys::SETTINGS_VERSION_KEY).get< int >(); } - // Copy since we might have to make modifications + profiles.settings_version = settingsVersion; nlohmann::json json = j; - migrateProfiles(json, Global::get().profiles.settings_version); + migrateProfiles(json, settingsVersion); Suggestion importance[1-10]: 9 __ Why: Passing the parsed `settings_version` to `migrateProfiles` ensures migrations use the correct version rather than a default global value.	High
General	Include settings version key Re-add writing of the settings version at the top of the serialized object. This maintains version tracking for future migrations. src/mumble/JSONSerialization.cpp [96-97] void to_json(nlohmann::json &j, const Settings &settings) { + j[SettingsKeys::SETTINGS_VERSION_KEY] = s_current_settings_version; const Settings defaultValues; Suggestion importance[1-10]: 7 __ Why: Restoring the version key in `to_json` maintains consistency with migrations and leverages the introduced `s_current_settings_version`.	Medium

Previous suggestions

Suggestions up to commit eadf494

Category	Suggestion	Impact
Possible issue	Fix profile JSON object assignment Using `push_back` on a JSON object may not insert the key-value pair as intended and could cause runtime errors. Instead, assign the value directly using the key to ensure the profile is correctly updated in the JSON object. src/mumble/Settings.cpp [162-163] profilesJSON.erase(profiles.activeProfileName.toStdString()); -profilesJSON.push_back({ profiles.activeProfileName, activeProfileJSON }); +profilesJSON[profiles.activeProfileName.toStdString()] = activeProfileJSON; Suggestion importance[1-10]: 8 __ Why: This suggestion correctly identifies a potential bug: using `push_back` on a JSON object may not insert a key-value pair as intended and can cause runtime errors. Assigning the value directly using the key ensures the profile is updated correctly, which is crucial for settings persistence.	Medium
Possible issue	Use correct version for profile migration Passing `Global::get().profiles.settings_version` to `migrateProfiles` may be incorrect, as it could be uninitialized or not reflect the version in the loaded JSON. Use the `settings_version` just parsed from the JSON to ensure correct migration logic. src/mumble/JSONSerialization.cpp [264-279] void from_json(const nlohmann::json &j, Profiles &profiles) { profiles.settings_version = 0; if (j.contains(SettingsKeys::SETTINGS_VERSION_KEY)) { profiles.settings_version = j.at(SettingsKeys::SETTINGS_VERSION_KEY).get< int >(); } // Copy since we might have to make modifications nlohmann::json json = j; - migrateProfiles(json, Global::get().profiles.settings_version); + migrateProfiles(json, profiles.settings_version); #define PROCESS(category, key, variable) load(json, SettingsKeys::key, profiles.variable, profiles.variable, true); PROCESS_ALL_PROFILE_SETTINGS #undef PROCESS } Suggestion importance[1-10]: 7 __ Why: The suggestion improves correctness by ensuring the migration function uses the version parsed from the loaded JSON, not a potentially uninitialized or incorrect global value. This is important for robust migration logic, though not as critical as a direct runtime bug.	Medium

Suggestions up to commit eadf494

Category	Suggestion	Impact
Possible issue	Fix undefined `s` usage The local variable `s` is undefined here and causes a compile error. Use the loaded settings directly or introduce a properly scoped `Settings` variable before passing it to each widget. src/mumble/ConfigDialog.cpp [264-267] Global::get().s.loadProfile(newProfile); -s = Global::get().s; +Settings currentSettings = Global::get().s; for (ConfigWidget *cw : s_existingWidgets.values()) { - cw->load(s); + cw->load(currentSettings); } Suggestion importance[1-10]: 9 __ Why: The code uses an undeclared `s` member in `switchProfile`, causing a compile error; introducing a local `Settings` variable fixes this critical bug.	High
	Use JSON file’s version for migration The migration call uses the runtime profile version instead of the version from the JSON file. Extract the version from `json` and pass that to `migrateSettings` so migrations run correctly. src/mumble/JSONSerialization.cpp [176] -// ... -migrateSettings(json, Global::get().profiles.settings_version); +// extract version from JSON +int settingsVersion = json.contains(SettingsKeys::SETTINGS_VERSION_KEY) + ? json.at(SettingsKeys::SETTINGS_VERSION_KEY).get<int>() + : s_current_settings_version; +migrateSettings(json, settingsVersion); Suggestion importance[1-10]: 8 __ Why: Passing the global profile version to `migrateSettings` ignores the actual JSON version, risking incorrect migrations; extracting the version from the JSON ensures correctness.	Medium
	Fix profile migration version The call to `migrateProfiles` should use the just-read `profiles.settings_version` instead of the old global value, otherwise version-based migrations won't trigger correctly. src/mumble/JSONSerialization.cpp [272] void from_json(const nlohmann::json &j, Profiles &profiles) { profiles.settings_version = 0; if (j.contains(SettingsKeys::SETTINGS_VERSION_KEY)) { profiles.settings_version = j.at(SettingsKeys::SETTINGS_VERSION_KEY).get< int >(); } // Copy since we might have to make modifications nlohmann::json json = j; - migrateProfiles(json, Global::get().profiles.settings_version); + migrateProfiles(json, profiles.settings_version); // ... } Suggestion importance[1-10]: 8 __ Why: Using `Global::get().profiles.settings_version` instead of the freshly read `profiles.settings_version` prevents applying profile migrations properly, so switching to the correct version value is necessary.	Medium
General	Clear map before deserialization The target `map` may not be empty before deserialization, leading to stale entries. Clear it at the start of the function to ensure only the JSON contents are present. src/mumble/JSONSerialization.cpp [499-503] template< typename Key, typename Value > void from_json(const nlohmann::json &j, std::map< Key, Value > &map) { + map.clear(); for (auto it = j.cbegin(); it != j.cend(); ++it) { map[details::from_string< Key >(it.key())] = it.value().get< Value >(); } } Suggestion importance[1-10]: 5 __ Why: Without clearing, existing entries in `map` may persist across deserializations, leading to stale data; inserting `map.clear()` ensures only JSON contents are loaded.	Low

Hartmnt · 2025-07-09T11:06:06Z

@Krzmbrzl At first glance it looks like all of these are wrong or at least questionable 👀 Maybe the one where it clears the map before serializing has some merit to it.

Hartmnt · 2025-07-09T11:08:40Z

Hm? It automatically removed some suggestions again?

It keeps changing lol.

Krzmbrzl · 2025-07-09T11:14:24Z

At first glance it looks like all of these are wrong or at least questionable 👀 Maybe the one where it clears the map before serializing has some merit to it.

Yeah. It worked better in another PR where I tried it (not for Mumble) but even so, I think it identified some points of interest that at least warrant a reviewer to take a closer look and ponder whether the way it is done, is correct.
So I still wouldn't count it as a complete failure.

Hm? It automatically removed some suggestions again?
It keeps changing lol.

Indeed 😀
Seems like it refines its suggestions once it had more time to "think" about them 👀
Or maybe it noticed that it is being made fun of and tries to correct :D

TODO: Write commit message

Hartmnt added client feature-request This issue or PR deals with a new feature labels Jan 5, 2025

Krzmbrzl reviewed Jan 7, 2025

View reviewed changes

src/mumble/Settings.cpp Show resolved Hide resolved

src/mumble/Settings.h Outdated Show resolved Hide resolved

Hartmnt force-pushed the feat_profiles branch 11 times, most recently from e784b92 to b14de43 Compare June 6, 2025 16:24

Krzmbrzl reviewed Jun 8, 2025

View reviewed changes

src/mumble/Settings.cpp Outdated Show resolved Hide resolved

src/mumble/Settings.cpp Outdated Show resolved Hide resolved

src/mumble/Settings.h Outdated Show resolved Hide resolved

src/mumble/Settings.h Outdated Show resolved Hide resolved

Hartmnt force-pushed the feat_profiles branch 9 times, most recently from a5ab053 to 3b20087 Compare June 14, 2025 13:31

Hartmnt force-pushed the feat_profiles branch 2 times, most recently from 8192d5b to eadf494 Compare June 20, 2025 17:02

Hartmnt added 5 commits September 13, 2025 12:18

FEAT(client): Settings Profiles (backend)

271ba9a

TODO: Write commit message

FEAT(client): Settings Profiles (frontend)

f118e14

TODO: Write commit message

FIX(client): Tests

3ef0dd1

FEAT(client): Add global shortcut to switch profile

433c6cc

TMP

a9979c6

Hartmnt force-pushed the feat_profiles branch 13 times, most recently from 136efd2 to 12e01f8 Compare September 15, 2025 18:45

REFAC: Statically initialize ConfigWidgets

59b9dd6

Hartmnt force-pushed the feat_profiles branch from 12e01f8 to 59b9dd6 Compare September 15, 2025 18:54

Uh oh!

FEAT(client): Settings Profiles #6681

Are you sure you want to change the base?

FEAT(client): Settings Profiles #6681

Uh oh!

Conversation

Hartmnt commented Jan 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Krzmbrzl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Hartmnt commented Jan 7, 2025

Uh oh!

Krzmbrzl commented Jan 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AvatarTheOld commented Mar 31, 2025

Uh oh!

Hartmnt commented Jun 6, 2025

Uh oh!

Hartmnt commented Jun 6, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Krzmbrzl commented Jun 8, 2025

Uh oh!

Hartmnt commented Jun 23, 2025

Uh oh!

Krzmbrzl commented Jun 24, 2025

Uh oh!

Hartmnt commented Jun 24, 2025

Uh oh!

Krzmbrzl commented Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

QodoAI-Agent commented Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Previous suggestions

Uh oh!

Hartmnt commented Jul 9, 2025

Uh oh!

Hartmnt commented Jul 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Krzmbrzl commented Jul 9, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Hartmnt commented Jan 5, 2025 •

edited

Loading

Krzmbrzl commented Jan 8, 2025 •

edited

Loading

Krzmbrzl commented Jul 9, 2025 •

edited

Loading

QodoAI-Agent commented Jul 9, 2025 •

edited

Loading

Hartmnt commented Jul 9, 2025 •

edited

Loading