Fix #289190 - Toolbar customization refinements & fixes #5019

Obliquely · 2019-05-15T13:41:17Z

made dialogue show the proper names for customisable actions / item
previously it was showing the internal tags/ids (which won't translate)
allow for the view mode / view zoom to be customised like other items
fixed title of dialogue box
removed text box showing workspace name from dialogue box - it's an
edit box showing information that can never be edited - unhelpful UI
added name of workspace being edited to the dialogue box title
re-ordered the available and chosen lists in the dialogue box to make
more sense - most specific is at the right and not in the middle
added tooltips to various items in dialogue box
menu is greyed out / disabled when toolbars are not customisable,
i.e. because we're in advanced or basic mode
available actions now maintain STANDARD order when items are removed
from the current toolbar

Obliquely · 2019-05-15T13:42:17Z

Jojo-Schmitz · 2019-05-15T14:15:13Z

mscore/musescore.cpp


-      viewModeCombo = new QComboBox(this);
+        for (auto s : _fileOperationEntries) {


wrong indentation, we use 6 spaces per level, here it is 8

Jojo-Schmitz · 2019-05-15T14:16:04Z

mscore/musescore.cpp

+
+    // view-options is treated as a special case as it's not a QToolButton
+    // but, rather, made up of two QComboBox objects (one sub-classed).
+    void MuseScore::addViewOptionsDoubleWidget()


here it is 4

Jojo-Schmitz · 2019-05-15T14:16:50Z

mscore/musescore.h

@@ -882,6 +883,7 @@ MasterSynthesizer* synthesizerFactory();
 Driver* driverFactory(Seq*, QString driver);

 extern QAction* getAction(const char*);
+    extern QString getShortcutText(const char*);


wrong indent again

Jojo-Schmitz · 2019-05-15T14:17:36Z

mscore/shortcut.cpp

+         QT_TRANSLATE_NOOP("action","View Options"),
+         0,
+         0,
+          Icons::viewOptions_ICON,


different indeting here, but one space too much ;-)

Jojo-Schmitz · 2019-05-15T14:18:02Z

mscore/shortcut.cpp

+
+QString getShortcutText(const char* id)
+{
+    Shortcut* shortcut = Shortcut::getShortcut(id);


6 spaces here please

Jojo-Schmitz · 2019-05-15T14:23:06Z

mscore/toolbarEditor.ui

+   <item row="1" column="0" rowspan="2" colspan="2">
+    <widget class="QListWidget" name="toolbarList">
+     <property name="toolTip">
+      <string>Toolbar to customise</string>


customize please, US spelling. And I promise to translate it to customise for the Brits ;-)

Jojo-Schmitz · 2019-05-15T15:06:48Z

mscore/musescore.cpp

-              }
+            else
+                  fileTools->addWidget(new AccessibleToolButton(fileTools, getAction(s)));
+      }


Wrong indent (we use banner style, braces indented)

Jojo-Schmitz · 2019-05-15T15:08:48Z

mscore/musescore.cpp

+      bool leftToRightLayout = qApp->layoutDirection() == Qt::LayoutDirection::LeftToRight;
+
+      if (!leftToRightLayout)
+          _fileOperationEntries.reverse();


6 spaces here

Jojo-Schmitz · 2019-05-15T15:09:06Z

mscore/musescore.cpp

+      }
+
+      if (!leftToRightLayout)
+         _fileOperationEntries.reverse();


Jojo-Schmitz · 2019-05-15T15:09:19Z

mscore/musescore.cpp

      mag = new MagBox;
      connect(mag, SIGNAL(magChanged(MagIdx)), SLOT(magChanged(MagIdx)));
      fileTools->addWidget(mag);
-
+      


no spaces here

Jojo-Schmitz · 2019-05-15T15:09:26Z

mscore/musescore.cpp

      viewModeCombo = new QComboBox(this);
 #if defined(Q_OS_MAC)
      viewModeCombo->setFocusPolicy(Qt::StrongFocus);
 #else
      viewModeCombo->setFocusPolicy(Qt::TabFocus);
 #endif
      viewModeCombo->setFixedHeight(preferences.getInt(PREF_UI_THEME_ICONHEIGHT) + 8);  // hack
-
+      


Jojo-Schmitz · 2019-05-15T15:09:36Z

mscore/musescore.cpp

      viewModeCombo->setAccessibleName(tr("View Mode"));
      viewModeCombo->addItem(tr("Page View"), int(LayoutMode::PAGE));
      viewModeCombo->addItem(tr("Continuous View"), int(LayoutMode::LINE));
      viewModeCombo->addItem(tr("Single Page"), int(LayoutMode::SYSTEM));
-
+      


Jojo-Schmitz · 2019-05-15T15:10:08Z

mscore/musescore.cpp


 //---------------------------------------------------------
 //   populatePlaybackControls
 //---------------------------------------------------------

 void MuseScore::populatePlaybackControls()
-      {
+   {


Either no spaces (for methods/functions) or 6

Jojo-Schmitz · 2019-05-15T15:10:19Z

mscore/musescore.cpp

-            if (!*s)
-                  transportTools->addSeparator();
+         if (!*s) {
+            transportTools->addSeparator();


indent... here and below

Jojo-Schmitz · 2019-05-15T15:11:29Z

mscore/toolbarEditor.cpp

-            editToolbars = new ToolbarEditor(this);
-            }
+         editToolbars = new ToolbarEditor(this);
+      }


banner style indent. Or no braces here at all for a single statement

Jojo-Schmitz · 2019-05-15T15:11:53Z

mscore/toolbarEditor.cpp

@@ -111,49 +117,63 @@ void ToolbarEditor::accepted()
 //---------------------------------------------------------

 void ToolbarEditor::populateLists(const std::list<const char*>& all, std::list<const char*>* current)
-      {
+   {


no or 6 spaces...

Jojo-Schmitz · 2019-05-15T15:12:12Z

mscore/toolbarEditor.cpp

      actionList->clear();
+      for (auto currentId : *current) {
+         QString shortcutText = getShortcutText(currentId);


indent... here and below

Jojo-Schmitz · 2019-05-15T15:12:27Z

mscore/toolbarEditor.cpp

-            actionList->addItem(item);
+
+      for (auto id : all) {
+         bool found = false;


Jojo-Schmitz · 2019-05-15T15:12:56Z

mscore/toolbarEditor.cpp

+
+
+
+


excess white space

Jojo-Schmitz · 2019-05-15T15:13:43Z

mscore/workspace.cpp

@@ -169,17 +169,20 @@ void MuseScore::changeWorkspace(QAction* a)
 //---------------------------------------------------------

 void MuseScore::changeWorkspace(Workspace* p, bool first)
-      {


Jojo-Schmitz · 2019-05-15T15:14:00Z

mscore/workspace.cpp

-            Workspace::currentWorkspace->save();
-
+         Workspace::currentWorkspace->save();
+      


spaces for nothing

Jojo-Schmitz · 2019-05-15T15:14:11Z

mscore/workspace.cpp

-            updateIcons();
-            preferencesChanged(true);
-            }
+         updateIcons();


Jojo-Schmitz · 2019-05-15T15:15:25Z

Please check out coding style guide, reg. indenting, see https://musescore.org/en/handbook/developers-handbook/finding-your-way-around/musescore-coding-rules#Indentation-%26-whitespaces
If using QtCreator much of this can get automated, see https://musescore.org/en/handbook/developers-handbook/finding-your-way-around/musescore-coding-rules#Coding-style and the xml file provided there

Reg. language see https://musescore.org/en/handbook/developers-handbook/text-style-guide#Language

Also please squash the commits

anatoly-os · 2019-05-15T15:29:42Z

Please don't squash :) It will be useful during the review process of such non trivial changes

Obliquely · 2019-05-17T18:42:41Z

@Jojo-Schmitz I very much hope I've caught all the indenting errors now. I've squashed. Thanks for the help and apologies for using up your time on those kinds of error.

Jojo-Schmitz · 2019-05-19T11:53:12Z

mscore/shortcut.cpp

+         STATE_NORMAL,
+         "view-options",
+         QT_TRANSLATE_NOOP("action","View Options"),
+         0,


Please change that 0, into QT_TRANSLATE_NOOP("action","View options"),, needed for the shortcuts dialog (and as a tooltip) and there in sentence case

This comment got me thinking. The zoom box and view mode box don't really belong in the shortcuts list at all. There are already shortcuts for various zoom and page mode actions. So I've slightly changed tack. (See below.)

Obliquely · 2019-05-20T23:06:39Z

@Jojo-Schmitz 's comment about shortcuts (above) made me realise that it doesn't make sense to use shortcuts to represent the zoom and view mode combo boxes. There are already shortcut actions related to what the dropdown boxes do. This slightly reduces scope of changes and avoids the need for a new icon. Also added option to customise the zoom box and the view mode independently and fixed some spacing issues related to those toolbar items.

Obliquely · 2019-05-20T23:20:17Z

mscore/toolbarEditor.cpp

+            return new QListWidgetItem(action->icon(), itemName);
+      }
+
+      qDebug()<<"ToolbarEditor does not recognize id for toolbar item: "<<QString(id);


Is it OK to leave the qDebug() in or is it better to do something with ASSERT (the syntax of which I'd need to look up :)). This should never be sprung, but it's pretty benign if it is.

IIRC qDebug()s vanish when build in RELEASE mode

Jojo-Schmitz · 2019-05-21T06:00:37Z

Now Travis CI's mtests fail, check https://travis-ci.org/musescore/MuseScore/jobs/535052660#L4293-L4321

Obliquely · 2019-05-21T07:38:34Z

mscore/musescore.cpp

@@ -4140,7 +4140,10 @@ void MuseScore::changeState(ScoreState val)
      if (getAction("split-measure")->isEnabled())
            getAction("split-measure")->setEnabled(cs && cs->masterScore()->excerpts().size() == 0);

-      getAction("edit-toolbars")->setEnabled(!Workspace::currentWorkspace->readOnly());
+      Workspace* currentWorkspace = Workspace::currentWorkspace;
+      if (currentWorkspace) {


When running tst_runscripts test the currentWorkspace returns nullptr and MuseScore crashes before it even gets to run any of the tests. I've put in an explicit check to avoid this. It now passes 43/44 tests, which is exactly what master is currently passing. Is currentWorkspace unexpectedly being null because the tests are running MuseScore in "headless" mode (if that's the right terminology)? Might this be an issue elsewhere?

Jojo-Schmitz · 2020-02-10T09:06:10Z

rebase needed

Jojo-Schmitz · 2020-08-29T06:38:56Z

Another rebase needed

RobFog · 2020-08-29T07:55:00Z

Is this another one for the design department and @Tantacrul to consider?

Jojo-Schmitz · 2020-08-30T13:14:03Z

Code style issues. The Travis fails seems to be unrelated to this PR

- make ToolbarEditor dialog show the proper names for customisable actions / item as previously it was showing the internal tags/ids - allow for the view mode & page zoom dropdowns to be customised like all other toolbar items - re-ordered the available and chosen lists in the dialogue box to make more sense - most specific is now at the right and not in the middle - Customize Toolbars menu is greyed out / disabled when toolbars are not customisable, i.e. because we're in advanced or basic mode or because although workspace is editable the toolbars are not - available actions now maintain STANDARD order when items are removed from the current toolbar - improved the spacing of view mode and page zoom dropdowns on the toolbar

Obliquely · 2020-08-30T14:11:18Z

Code style issues. The Travis fails seems to be unrelated to this PR

Code style fixed (via uncrustify - thanks :) ). Not sure i can do anything wrt TRAVIS fail.

Jojo-Schmitz · 2020-08-31T07:17:05Z

No idea what to do about Travis, https://travis-ci.org/github/musescore/MuseScore/jobs/722473721#L484-L653
Seems entirely unrelated to the changer in this PR here

vpereverzev · 2020-12-30T23:54:48Z

Automatically archived due to long inactivity. Could be re-opened later once an author will come back