Restructure Palettes buttons and add "Expand/Collapse All" buttons #6593

cbjeukendrup · 2020-10-01T21:33:42Z

This is what I did:

Moved the "Add Palettes" button to the right of the search field;
Replaced the button text with a '+' icon;
Made the search field a bit wider;
Added an extra '...' button at the right;
Created a popup for the new button;
Filled the popup with "Expand/Collapse All" buttons;
Moved the "Single Palette" option to the new popup;
Removed the context menu formerly holding "Single Palette";
Tested thoroughly on my Mac.

A MuseScore user and friend of mine requested this feature, so I started looking if I could implement it. I also had some correspondence about this with Tantacrul in the MuseScore Developer Chat, and he agreed with the changes.
By the way: this is the very first time that I'm using Qml (and sort of my first real contribution for MuseScore), so I'll be welcoming any feedback!

Maybe this PR is something for MuseScore 3.6, because it's not really a breaking change, but it involves some string translations.

I signed CLA
I made sure the code in the PR follows the coding rules
I made sure the code compiles on my machine
I made sure there are no unnecessary changes in the code
I made sure the title of the PR reflects the core meaning of the issue you are solving
I made sure the commit message(s) contain a description and answer the question "Why do those changes fix that particular issue?" or "Why are those changes really necessary as improvements?"
I made sure the commit message title starts with "fix #424242:" if there is a related issue
[N/A] I created the test (mtest, vtest, script test) to verify the changes I made

ecstrema · 2020-10-02T01:43:24Z

Great! Would you mind posting a screenshot?

cbjeukendrup · 2020-10-02T09:27:36Z

@Marr11317 Done that, in the original post!

adriaandegroot

[[ Not a MuseScore contributor or reviewer by any means, but I was asked to look at this PR externally, with some kind of C++ and QML hats on ]]

Looks good overall
The screencast is a good touch in the PR
Copyright statements are a mess; even with the CLA in place, it seems weird to claim a 2019 date on some new QML, and some code copyrighted by the BV and some by an individual who's not you.

adriaandegroot · 2020-10-02T11:58:44Z

mscore/palette/palettedialog.cpp

@@ -60,7 +60,7 @@ PalettePropertiesDialog::~PalettePropertiesDialog()
      }

 //---------------------------------------------------------
-//   PalettePropertiesDialog::setData
+//   PalettePropertiesDialog::fillControlsWithData


This is an unrelated change, although it is correct. Might want to split that to a separate commit / PR.

True, but I thought that it's neither convenient to have a PR for such a small, unimportant change. Do you think it's OK to leave it like this?

mscore/palette/paletteworkspace.cpp

adriaandegroot · 2020-10-02T12:03:11Z

mscore/palette/paletteworkspace.cpp

+
+      preferencesListenerID = preferences.addOnSetListener([this](const QString& key, const QVariant& value) {
+            if (key == PREF_APP_USESINGLEPALETTE)
+                  emit singlePaletteChanged();


For QML purposes, it's sometimes better to pass the changed value in the signal, e.g. singlePaletteChanged(value.toBool()), but existing signals in the MuseScore codebase are inconsistent in that regard.

I read different opinions about this. Some people say that you should pass the value in the signal, others say that you definitely shouldn't. Anyway, I'm not sure how to tell Qml that it should take the value from the signal. Or does it automatically, when the signal has a parameter?

mscore/palette/paletteworkspace.h

mscore/qml/palettes/PaletteOptionsPopup.qml

adriaandegroot · 2020-10-02T12:29:40Z

mscore/qml/palettes/PaletteOptionsPopup.qml

+    }
+
+    onAboutToShow: {
+        // TODO: change these properties automatically whenever a palette is expanded/collapsed


These properties could be added to the model itself, though that's an opinion on what belongs in the model and what doesn't. You'd either get a model signal, or bind to a property in paletteTree that is bound to the model property.

I put them here, because they are only updated when the Popup opens. So when something else would read these properties, chances are that they are not up-to-date. Ideally, there would be a signal to update the properties automatically when a Palette is collapsed or expanded. In that case, it would be okay to move them to the model. It could be a next step to implement this.

mscore/qml/palettes/PaletteTree.qml

mscore/qml/palettes/StyledPopup.qml

cbjeukendrup · 2020-10-02T17:27:03Z

@adriaandegroot Thanks for your review! I answered and resolved the points you mentioned.
I agree about the copyright statements; I believe they are not quite consistent in the whole project. I suggest that somebody who knows a little more about copyrights than I do, could clean them up.

mscore/qml/palettes/PaletteOptionsPopup.qml

mscore/qml/palettes/StyledCheckBox.qml

Jojo-Schmitz · 2020-10-03T11:27:10Z

I agree about the copyright statements; I believe they are not quite consistent in the whole project. I suggest that somebody who knows a little more about copyrights than I do, could clean them up.

Just leave the existing Copyright statements as they are (this is indeed a cleanup job that should eventually get done, but surely not as part of this PR), but fix those for your new files.

- Moved the "Add Palettes" button to the right of the search field; - Replaced the button text with a '+' icon; - Made the search field a bit wider; - Added an extra '...' button at the right; - Created a popup for the new button; - Filled the popup with "Expand/Collapse All" buttons; - Moved the "Single Palette" option to the new popup; - Removed the context menu formerly holding "Single Palette"; - Tested thoroughly.

... to make it look better in a separate window.

cbjeukendrup · 2020-10-10T14:25:01Z

Another small change: I added some margin above the PalettesWidgetHeader, so that it looks better when the Palettes panel is moved to a separate window. Tested on Windows and macOS.

vpereverzev · 2020-10-21T09:32:25Z

I'll merge it in 3.x, but it shouldn't be ported to master because we have the whole new implementation

adriaandegroot reviewed Oct 2, 2020

View reviewed changes

cbjeukendrup force-pushed the 3.x-palettes-collapse-all branch from 61f5267 to 9a056ff Compare October 2, 2020 16:44

cbjeukendrup mentioned this pull request Oct 2, 2020

MuseScore 3.6 Alpha. PRs to be merged and issues to be fixed. #6505

Closed

74 tasks

cbjeukendrup force-pushed the 3.x-palettes-collapse-all branch from 9a056ff to 2f3687f Compare October 3, 2020 11:04

Jojo-Schmitz reviewed Oct 3, 2020

View reviewed changes

mscore/qml/palettes/PaletteOptionsPopup.qml Outdated Show resolved Hide resolved

Jojo-Schmitz reviewed Oct 3, 2020

View reviewed changes

mscore/qml/palettes/StyledCheckBox.qml Outdated Show resolved Hide resolved

cbjeukendrup force-pushed the 3.x-palettes-collapse-all branch from 2f3687f to de61423 Compare October 3, 2020 11:45

cbjeukendrup force-pushed the 3.x-palettes-collapse-all branch from de61423 to 73b3ce0 Compare October 9, 2020 21:12

Add margin above PalettesWidgetHeader...

bdb11e4

... to make it look better in a separate window.

IsaacWeiss mentioned this pull request Oct 12, 2020

[MU4] Palettes panel redesign #6659

Merged

vpereverzev merged commit 77da716 into musescore:3.x Oct 21, 2020

cbjeukendrup deleted the 3.x-palettes-collapse-all branch October 21, 2020 09:52

cbjeukendrup mentioned this pull request Nov 26, 2020

Make 'Add Palettes' button more prominent #6944

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restructure Palettes buttons and add "Expand/Collapse All" buttons #6593

Restructure Palettes buttons and add "Expand/Collapse All" buttons #6593

cbjeukendrup commented Oct 1, 2020 •

edited

Loading

ecstrema commented Oct 2, 2020

cbjeukendrup commented Oct 2, 2020

adriaandegroot left a comment

adriaandegroot Oct 2, 2020

cbjeukendrup Oct 2, 2020

adriaandegroot Oct 2, 2020

cbjeukendrup Oct 2, 2020

adriaandegroot Oct 2, 2020

cbjeukendrup Oct 2, 2020

cbjeukendrup commented Oct 2, 2020

Jojo-Schmitz commented Oct 3, 2020 •

edited

Loading

cbjeukendrup commented Oct 10, 2020

vpereverzev commented Oct 21, 2020

Restructure Palettes buttons and add "Expand/Collapse All" buttons #6593

Restructure Palettes buttons and add "Expand/Collapse All" buttons #6593

Conversation

cbjeukendrup commented Oct 1, 2020 • edited Loading

ecstrema commented Oct 2, 2020

cbjeukendrup commented Oct 2, 2020

adriaandegroot left a comment

Choose a reason for hiding this comment

adriaandegroot Oct 2, 2020

Choose a reason for hiding this comment

cbjeukendrup Oct 2, 2020

Choose a reason for hiding this comment

adriaandegroot Oct 2, 2020

Choose a reason for hiding this comment

cbjeukendrup Oct 2, 2020

Choose a reason for hiding this comment

adriaandegroot Oct 2, 2020

Choose a reason for hiding this comment

cbjeukendrup Oct 2, 2020

Choose a reason for hiding this comment

cbjeukendrup commented Oct 2, 2020

Jojo-Schmitz commented Oct 3, 2020 • edited Loading

cbjeukendrup commented Oct 10, 2020

vpereverzev commented Oct 21, 2020

cbjeukendrup commented Oct 1, 2020 •

edited

Loading

Jojo-Schmitz commented Oct 3, 2020 •

edited

Loading