11565 confirmation dialog for reset preferences and fixes #25400

krasko78 · 2024-11-01T19:50:55Z

Resolves: #11565

Added a confirmation dialog for when the Reset preferences button on the Preferences dialog is clicked
All preference pages update all of their settings to match the reset preferences after resetting the preferences.

I've added some comments below (in this PR) with more info and observations.

I signed the CLA
The title of the PR describes the problem it addresses
Each commit's message describes its purpose and effects, and references the issue it resolves
If changes are extensive, there is a sequence of easily reviewable commits
The code in the PR follows the coding rules
There are no unnecessary changes
The code compiles and runs on my machine, preferably after each commit individually
I created a unit test or vtest to verify the changes I made (if applicable)

…erences" button is clicked

…rences

…ation dialog of Reset Preferences

krasko78 · 2024-11-01T19:56:25Z

Observations:

The confirmation dialog is in preferencesmodel.cpp and is pretty straight-forward. The Cancel button is the default button. Here is a screenshot:
The changes to update each page after the reset is complete are in separate commits (much easier to review I guess). The Appearance, Folders and Shortcuts page already worked as expected so no changes were needed for them.
There is inconsistency in the use of .onReceive(nullptr, vs. .onReceive(this,.
There is also inconsistent use of ValCh vs. ValNt vs. separate channel/notification.
I've added a few comments to some files to explain why I had to make a specific change that might not be obvious.

krasko78 · 2024-11-01T20:04:52Z

src/framework/languages/internal/languagesservice.cpp

        //! NOTE To change the language at the moment, a restart is required
-        m_needRestartToApplyLanguageChange = true;
+        m_needRestartToApplyLanguageChange = newLanguageCode != m_activeLanguageCode;


This is needed to make the "Restart required" text go away if the user restores the language explicitly or via Reset preferences to the currently active language. Currently, once this text appears, it won't go away.

Doesn't the new m_activeLanguageCode field duplicate info from m_currentLanguage.code?

Not exactly. If the language code is "system", m_currentLanguageCode will hold the effective language code, e.g. "en-us" so choosing another language and going back to "system" will not make "Restart required" go away if we use m_currentLanguageCode here. I am changing m_activeLanguageCode to be a local variable in this method since it is not used anywhere else.

krasko78 · 2024-11-01T20:13:18Z

src/framework/global/settings.cpp

@@ -95,6 +95,13 @@ void Settings::reset(bool keepDefaultSettings, bool notifyAboutChanges)
    m_settings->clear();

    m_isTransactionStarted = false;
+
+    std::vector<Settings::Key> locallyAddedKeys;


I made these changes because it turned out that some settings without a value are not present in m_items. If the user opens the Preferences dialog, all settings are switched to m_localSettings. If the user sets a value of a missing setting, it will be created in m_localSettings. Then, if the user resets the preferences without closing the dialog, change notifications will be sent out for the settings in m_items, i.e. any settings added in m_localSettings will be missed and thefore not updated on the dialog. Two examples of such settings are: Score -> Style for part (PART_STYLE_FILE_PATH in the code) and Import -> Style file (STYLE_FILE_IMPORT_PATH_KEY in the code).

krasko78 · 2024-11-01T20:20:33Z

src/appshell/qml/Preferences/internal/ImportStyleSection.qml

@@ -41,7 +41,12 @@ BaseSection {
    QtObject {
        id: prv

-        property bool useStyleFile: root.styleFileImportPath !== ""
+        property bool useStyleFileExplicitlySet: root.styleFileImportPath !== ""


These changes were needed to properly update the Styles used for import controls on the Import page and fix these two bugs:
1. Open the Preferences dialog and navigate to the Import page.
2. Change Style used for import from Built-in style to Use style file.
3. Type a file if you want or skip this step.
4. Click Reset Preferences without closing the dialog.
Result: the Use style file radiobutton remains selected.

AND:
1. Open the Preferences dialog and navigate to the Import page.
2. Change Style used for import from Built-in style to Use style file.
3. Type something in the file textbox.
4. Delete the text in the file textbox.
Result: the Built-in style radiobutton becomes selected.

…nux build and unit tests)

src/appshell/internal/appshellconfiguration.cpp

cbjeukendrup · 2024-11-15T13:02:43Z

src/appshell/view/preferences/advancedpreferencesmodel.cpp

@@ -96,28 +96,46 @@ void AdvancedPreferencesModel::load()
    for (auto it = items.cbegin(); it != items.cend(); ++it) {
        if (it->second.canBeManuallyEdited) {
            m_items << it->second;
+
+            muse::Settings::Key key = it->second.key;
+            settings()->valueChanged(key).onReceive(this, [this, key](const Val& val) {


I believe each subsequent call to settings()->valueChanged(key).onReceive(this (with emphasis on the latter this) either overrides the previously registered callback or does nothing

Can you elaborate? Each subsequent call is for a different setting (key) => different notification channel.

Ah sorry, you're definitely right!

No worries. Comments are always welcome. :)

…sAndFixes

mathesoncalum

Thanks for this @krasko78, Ieft some comments.

Also looks like quite a hefty rebase is required, apologies for not reviewing this sooner (we're all very busy at the moment). Before updating this though, I think it might be wise to wait for @avvvvve to get back to us with their design considerations so you don't end up doing double-work.

Cheers!

mathesoncalum · 2024-12-12T11:27:00Z

src/framework/audio/internal/platform/win/wasapiaudiodriver.cpp

@@ -34,7 +34,7 @@ using namespace winrt;
 using namespace muse;
 using namespace muse::audio;

-static constexpr char DEFAULT_DEVICE_ID[] = "default";
+static constexpr char DEFAULT_DEVICE_ID[] = "default"; // Must match DEFAULT_DEVICE_ID in audioconfiguration.cpp and possibly others!


Perhaps we should define DEFAULT_DEVICE_ID in audiotypes.h to avoid duplication.

This of course means we'll need to tweak the other audio driver classes (jackaudiodriver.cpp, linuxaudiodriver.cpp, osxaudiodriver.mm)

Yep, I'll see what I can do, thanks!

mathesoncalum · 2024-12-12T11:36:54Z

src/appshell/view/preferences/preferencesmodel.cpp

+{
+    std::string title = muse::trc("appshell", "Are you sure you want to reset preferences?");
+    std::string question = muse::trc("appshell", "This action will reset all your app preferences and delete all custom palettes and custom shortcuts.\n\n"
+                                                 "This action will not delete any of your scores.\n\n"
+                                                 "This action cannot be undone.");


Probably need a quick copy pass here.

@avvvvve pressing the reset preferences button in the preferences dialog will show a dialog with this text (screenshot here). Any thoughts on this (or the new dialog flow generally)?

mathesoncalum · 2024-12-12T11:47:54Z

src/framework/stubs/midi/midiconfigurationstub.cpp

+    static async::Notification ch;
+    return ch;


Extreme nitpick (sorry) - why the change? It's still a Notification (n).

Most likely my intension was to rename the other one indeed. :)

mathesoncalum · 2024-12-12T11:50:24Z

src/framework/stubs/midi/midiconfigurationstub.cpp

+    static async::Channel<bool> n;
+    return n;


Ah, perhaps you intended to rename this to ch?

I believe so, thanks! I have fixed both.

avvvvve · 2024-12-12T21:14:02Z

@krasko78 Thanks for taking this one on!

I understand the thought to make "Cancel" the default action here, but reordering the buttons tripped me up. I would imagine most users (including me) are used to "Cancel" being the left button in a group of two buttons in MuseScore. So if you accidentally click "Reset preferences" and instinctively move your mouse to the left button to "Cancel" the dialog without reading, you might inadvertently reset your preferences.

I would leave the order/coloration of buttons as they are in the revert to factory settings dialog (Cancel in grey on left, Reset in accent color on right).

However, we should make "Cancel" the first button that is focused if you open this dialog via keyboard navigation (rather than "Reset"). I also think we can delete the extra line breaks before "This action will note delete any of your scores", like so:

Just curious—while you're working on this, are you willing to tackle this issue that would make it so the startup wizard doesn't appear after you reset preferences and reopen the app? #13299

krasko78 · 2024-12-12T21:52:44Z

Thanks for the feedback, @mathesoncalum and @avvvvve! I'll look at your comments in detail shortly.
The Cancel button is on the right because my screenshot is from Windows (I am a Windows user). On Windows the Cancel button is on the right otherwise it would trip Windows users up :) So I believe on MacOS and Linux it will actually appear on the left. The code itself takes care of this automatically. Or is it that when we have a destructive action it is supposed to be to the right of the Cancel button even on Windows?

avvvvve · 2024-12-12T22:03:06Z

On Mac, the Cancel is showing up on the right (see screenshot), so something must be up with the auto-handling of that ordering. Fine to leave it like that for Windows then, but on Mac it should stick to the Mac defaults. I think "Reset" should still be the accented button on both OSs though.

cbjeukendrup · 2024-12-13T00:15:48Z

The code added by this PR looks fine in itself, but the order needs to be fixed in buttonboxmodel.h. Looks like we should move RejectRole almost to the front of the list, for macOS (and I guess Linux too), like this:

        // MacLayout
        std::vector <ButtonRole> { CustomRole, HelpRole, RejectRole, ResetRole, RetryRole, DestructiveRole, BackRole, AcceptRole,
                                   ApplyRole, ContinueRole },

Would be good to make sure that this has no undesired effects on other dialogs.

(FWIW, this is what Apple says about the topic: https://developer.apple.com/design/human-interface-guidelines/alerts#Buttons
So, Cancel should indeed always be on the left; they advise against making Cancel the default button, and suggest having no default at all for destructive dialogs. But we don't necessarily need to follow that of course.)

krasko78 · 2024-12-14T01:12:05Z

@avvvvve @cbjeukendrup Ah, yeah, I didn't check the button layouts in the code and automatically assumed they would be correct. I do not feel like moving RejectRole that much to the left of the list because as Casper said this would require review of the other dialogs which goes out of the scope of this PR. Instead I could give the Reset button either the ApplyRole (in fact this is what "Revert to factory settings" uses) or the ContinueRole. Another possibility is to see where the ResetRole is used and if it can be moved to the right of RejectRole.

krasko78 · 2024-12-17T21:26:30Z

I am closing this PR in favor of PR 25872 due to inability to rebase. apologies for any inconvenience.

krasko78 added 19 commits October 26, 2024 00:40

musescore#11565: Added a confirmation dialog for when the "Reset pref…

d755e82

…erences" button is clicked

musescore#11565: Made the General page update upon Reset Preferences

f35ac21

musescore#11565: Made the Canvas page update upon Reset Preferences

ef4f7bd

musescore#11565: Made the Save & Publish page update upon Reset Prefe…

7093590

…rences

musescore#11565: Made the Note Input page update on Reset Preferences

a57fe6f

musescore#11565: Made the Braille page update upon Reset Preferences

97fcf55

musescore#11565: Made the Advanced page update upon Reset Preferences

de14305

musescore#11565: Made the Update page update on Reset Preferences

3efe3c3

musescore#11565: Made the Import page update upon Reset Preferences

0e162bb

musescore#11565: Made the MIDI Mappings page update on Reset Preferences

0354323

musescore#11565: Made the Audio & MIDI page update on Reset Preferences

6505a2c

Made the Score page update on Reset Preferences

85e4df8

musescore#11565: Added "This action cannot be undone." to the confirm…

65dfd57

…ation dialog of Reset Preferences

musescore#11565: Refinements to the resetting of the General page

5644455

musescore#11565: Refinements to the resetting of the Canvas page

761d0b5

musescore#11565: Refinements to the resetting of the Note input page

831cac7

musescore#11565: Refinements to the resetting of the Advanced page

2ba60a9

musescore#11565: Refinements to the resetting of the Import page

3d9a024

musescore#11565: Refinements to the resetting of the Score page

9550b82

krasko78 marked this pull request as draft November 1, 2024 19:58

krasko78 commented Nov 1, 2024

View reviewed changes

krasko78 added 3 commits November 1, 2024 21:24

musescore#11565: Fixing the coding style

9182f7c

musescore#11565: Fixing a conflict with DEFAULT_DEVICE_ID (failing Li…

9e4d37e

…nux build and unit tests)

musescore#11565: Fixing the tests

8072952

krasko78 force-pushed the 11565-ConfirmationDialogForResetPreferencesAndFixes branch 2 times, most recently from 8f244fd to a23448e Compare November 1, 2024 23:14

musescore#11565: Fixing the tests

628dfa7

krasko78 force-pushed the 11565-ConfirmationDialogForResetPreferencesAndFixes branch from a23448e to 628dfa7 Compare November 1, 2024 23:18

krasko78 marked this pull request as ready for review November 1, 2024 23:53

cbjeukendrup reviewed Nov 15, 2024

View reviewed changes

src/appshell/internal/appshellconfiguration.cpp Outdated Show resolved Hide resolved

cbjeukendrup reviewed Nov 15, 2024

View reviewed changes

krasko78 added 2 commits November 18, 2024 02:35

Merge branch 'master' into 11565-ConfirmationDialogForResetPreference…

ebea4ba

…sAndFixes

11565: Minor improvements

0194b2d

cbjeukendrup requested a review from mathesoncalum December 1, 2024 17:36

mathesoncalum requested changes Dec 12, 2024

View reviewed changes

mathesoncalum assigned avvvvve Dec 12, 2024

krasko78 mentioned this pull request Dec 17, 2024

11565 confirmation dialog for reset preferences and make sure all preferences reset correctly #25872

Open

8 tasks

krasko78 closed this Dec 17, 2024

krasko78 deleted the 11565-ConfirmationDialogForResetPreferencesAndFixes branch December 19, 2024 19:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

11565 confirmation dialog for reset preferences and fixes #25400

11565 confirmation dialog for reset preferences and fixes #25400

krasko78 commented Nov 1, 2024

krasko78 commented Nov 1, 2024

krasko78 Nov 1, 2024

cbjeukendrup Nov 15, 2024

krasko78 Nov 18, 2024

krasko78 Nov 1, 2024

krasko78 Nov 1, 2024 •

edited

Loading

cbjeukendrup Nov 15, 2024

krasko78 Nov 18, 2024

cbjeukendrup Nov 18, 2024

krasko78 Nov 19, 2024

mathesoncalum left a comment

mathesoncalum Dec 12, 2024

krasko78 Dec 14, 2024

mathesoncalum Dec 12, 2024

mathesoncalum Dec 12, 2024

krasko78 Dec 14, 2024

mathesoncalum Dec 12, 2024

krasko78 Dec 14, 2024

avvvvve commented Dec 12, 2024

krasko78 commented Dec 12, 2024

avvvvve commented Dec 12, 2024

cbjeukendrup commented Dec 13, 2024 •

edited

Loading

krasko78 commented Dec 14, 2024

krasko78 commented Dec 17, 2024

11565 confirmation dialog for reset preferences and fixes #25400

11565 confirmation dialog for reset preferences and fixes #25400

Conversation

krasko78 commented Nov 1, 2024

krasko78 commented Nov 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krasko78 Nov 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mathesoncalum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avvvvve commented Dec 12, 2024

krasko78 commented Dec 12, 2024

avvvvve commented Dec 12, 2024

cbjeukendrup commented Dec 13, 2024 • edited Loading

krasko78 commented Dec 14, 2024

krasko78 commented Dec 17, 2024

krasko78 Nov 1, 2024 •

edited

Loading

cbjeukendrup commented Dec 13, 2024 •

edited

Loading