fix #272441: Toolbar Editor: Toolbar duplication #3689

JoshuaBonn1 · 2018-05-22T00:02:38Z

Originally, the toolbar only worked for the note input toolbar and had a bug for each other one. Now it correctly handles any edit to the toolbars.
The workspace only had the note input toolbar saved into the workspace. That has now been expanded to playback controls toolbar and file operations toolbar.

Jojo-Schmitz · 2018-05-22T07:22:07Z

Please do not make revision.h being part of any commit.
See developers' handbook and the post-checkout hook, https://musescore.org/en/handbook/developers-handbook/compilation/compile-instructions-windows-mingw-git#Source-Code, the part marked optional but recommended

Jojo-Schmitz · 2018-05-22T07:23:37Z

mscore/musescore.cpp

+      connect(mag, SIGNAL(magChanged(MagIdx)), SLOT(magChanged(MagIdx)));
+      fileTools->addWidget(mag);
+      viewModeCombo = new QComboBox(this);
+      #if defined(Q_OS_MAC)


No indent for pre-processor directives, please.

Just to clarify, you want me to change it to the following?

#if defined(Q_OS_MAC) viewModeCombo->setFocusPolicy(Qt::StrongFocus); #else viewModeCombo->setFocusPolicy(Qt::TabFocus); #endif

Yes, please. Here and in the other places.

Jojo-Schmitz · 2018-05-22T15:26:40Z

mscore/musescore.cpp

+            for (auto s : _fileOperationEntries) {
+                  if (!*s)
+                        fileTools->addSeparator();
+                  fileTools->addWidget(new AccessibleToolButton(fileTools, getAction(s)));


don't we need an else here?

Jojo-Schmitz · 2018-05-22T15:26:53Z

mscore/musescore.cpp

+            for (auto s : _fileOperationEntries) {
+                  if (!*s)
+                        fileTools->addSeparator();
+                  fileTools->addWidget(new AccessibleToolButton(fileTools, getAction(s)));


JoshuaBonn1 · 2018-05-22T15:53:37Z

It seems the message about the revision.h is not found in the Ubuntu build instructions. Is it a specific windows thing? Cause, it worked for me on Ubuntu 16.04LTS

anatoly-os · 2018-05-22T16:19:33Z

It does work on any platform 'cause revision.h is not used in development, but in release process only. So, the content of this file is changed by release managers before the official release.

MarcSabatella · 2018-05-22T16:46:51Z

mscore/musescore.cpp

+
+      if (qApp->layoutDirection() == Qt::LayoutDirection::LeftToRight) {
+            for (auto s : _fileOperationEntries) {
+                  if (!*s)


Dumb question - is it really (!*s) as opposed to (*s != "")? Or does that conversion happen elsewhere?

Most of this was just copying and pasting from existing code. I personally like *s != "" better if it works as expected

Upon compilation, it says that ISO C++ forbids that. The only thing that seems to work (for compilation) is strcmp(s, "").

Well, it segfaults when I tried that. So, !*s checks the first character of s for '\0'

Good point. Like I said, dumb question :-)

MarcSabatella · 2018-05-22T16:47:26Z

mscore/musescore.cpp

+                  transportTools->addSeparator();
+            else {
+                  if (QString(s) == "midi-on") {
+                        #ifdef HAS_MIDI


As I think was mentioned elsewhere, we don't indent ifdef's

I asked Jojo Schmitz and he said I should add tabs. Or am I confused about the actual purpose (see an above outdated message for that)

if you peruse the source I don't think you'll find a single instance of the ifdef statement itself being indented. The content of the ifdef, sure, it should be inline with the rest of the code. I think that's what the comment you mention was talking about. @Jojo-Schmitz?

What @Jojo-Schmitz actually wrote was "No indent for pre-processor directives, please.". Emphasis on the "no".

Ah, I was misunderstanding. I thought it meant no indent between the ifdefs. But it meant the ifdefs themselves should have no whitespace before them. I'll fix that

Yep, that's what I meant

Jojo-Schmitz · 2018-05-29T07:06:04Z

mscore/musescore.cpp

+                  if (QString(s) == "midi-on") {
+#ifdef HAS_MIDI
+                        transportTools->addWidget(new AccessibleToolButton(transportTools, getAction(s)));
+#endif


doesn't this result in starting with a separator in case of no MIDI? Wouldn't we need to skip that separator in an #else branch? The old code certainly only added the separator in the #if branch.
So either skip it or explicitly add it here but remove the empty entry from that array?

If I add it explicitly, then it's going to have odd behavior when switching the MIDI around to different locations. If I add a skip, then what do I skip when midi is placed in different places? Or will "midi-on" even be an available action if there is no midi?

In other apps I've used with customizable toolbars, buttons are buttons, separators are separators, and there is no connection between them. Adding a button doesn't ever add a separator, or does deleting a button delete a separator, nor does moving a button move a separator. Both are under the user's independent control.

For midi not being there, maybe it should remove both the midi option and the separator on the first run and then let the user customize it from then on. Then it will work for the default workspaces and anything custom past that is the user’s choice.

Now with your latest change that whole section is no longer needed, as that string (and the separator) doesn't exist if HAS_MIDI is not #define'd.
I like that change, it kills 2 birds with one stone ;-)

The toolbar editor now behaves correctly, each toolbar may be edited Playback controls and File operations are now saved to the workspace

Jojo-Schmitz · 2018-10-29T09:22:47Z

Needs a rebase

Jojo-Schmitz · 2018-10-29T13:33:48Z

OK, I've rebased/cherry-picked it into my #4076 now, in the understand that @JoshuaBonn1 lost his build environment and won't get it back any time soon (enough)

Jojo-Schmitz · 2018-10-29T18:32:59Z

as a separate PR now in #4078

anatoly-os · 2018-10-29T19:28:30Z

Merged #4078

JoshuaBonn1 changed the title ~~Fix toolbar editor, save all toolbars to workspace~~ fix #272441: Toolbar Editor: Toolbar duplication May 22, 2018

Jojo-Schmitz reviewed May 22, 2018

View reviewed changes

JoshuaBonn1 force-pushed the 272441-toolbar-editor branch from 091f141 to f1bc277 Compare May 22, 2018 15:31

MarcSabatella reviewed May 22, 2018

View reviewed changes

JoshuaBonn1 force-pushed the 272441-toolbar-editor branch 2 times, most recently from 422be72 to da18960 Compare May 28, 2018 18:16

Jojo-Schmitz reviewed May 29, 2018

View reviewed changes

JoshuaBonn1 force-pushed the 272441-toolbar-editor branch 2 times, most recently from b635e25 to b30e7d2 Compare May 30, 2018 22:10

fix #272441 Toolbar Editor: Toolbar duplication

bdfdf6b

The toolbar editor now behaves correctly, each toolbar may be edited Playback controls and File operations are now saved to the workspace

JoshuaBonn1 force-pushed the 272441-toolbar-editor branch from b30e7d2 to bdfdf6b Compare May 30, 2018 22:11

Jojo-Schmitz mentioned this pull request Oct 29, 2018

fix #277566: Add "Count-In" to the toolbar editor "Playback Controls" #4076

Merged

This was referenced Oct 29, 2018

Expand workspace functionality, fix #272441 #3729

Closed

fix #272441 Toolbar Editor: Toolbar duplication #4078

Merged

Expand workspace functionality, fix #272441 #4079

Merged

anatoly-os closed this Oct 29, 2018

JoshuaBonn1 deleted the 272441-toolbar-editor branch January 12, 2024 03:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix #272441: Toolbar Editor: Toolbar duplication #3689

fix #272441: Toolbar Editor: Toolbar duplication #3689

JoshuaBonn1 commented May 22, 2018

Jojo-Schmitz commented May 22, 2018 •

edited

Jojo-Schmitz May 22, 2018 •

edited

JoshuaBonn1 May 22, 2018

Jojo-Schmitz May 22, 2018

Jojo-Schmitz May 22, 2018

Jojo-Schmitz May 22, 2018

JoshuaBonn1 commented May 22, 2018

anatoly-os commented May 22, 2018

MarcSabatella May 22, 2018

JoshuaBonn1 May 22, 2018

JoshuaBonn1 May 22, 2018

JoshuaBonn1 May 22, 2018

MarcSabatella May 22, 2018

MarcSabatella May 22, 2018

JoshuaBonn1 May 22, 2018

MarcSabatella May 22, 2018

MarcSabatella May 22, 2018

JoshuaBonn1 May 22, 2018

Jojo-Schmitz May 22, 2018

Jojo-Schmitz May 29, 2018 •

edited

JoshuaBonn1 May 29, 2018

MarcSabatella May 29, 2018

JoshuaBonn1 May 29, 2018

Jojo-Schmitz May 30, 2018 •

edited

Jojo-Schmitz commented Oct 29, 2018

Jojo-Schmitz commented Oct 29, 2018

Jojo-Schmitz commented Oct 29, 2018

anatoly-os commented Oct 29, 2018

fix #272441: Toolbar Editor: Toolbar duplication #3689

fix #272441: Toolbar Editor: Toolbar duplication #3689

Conversation

JoshuaBonn1 commented May 22, 2018

Jojo-Schmitz commented May 22, 2018 • edited

Jojo-Schmitz May 22, 2018 • edited

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

JoshuaBonn1 commented May 22, 2018

anatoly-os commented May 22, 2018

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jojo-Schmitz May 29, 2018 • edited

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

Jojo-Schmitz May 30, 2018 • edited

Choose a reason for hiding this comment

Jojo-Schmitz commented Oct 29, 2018

Jojo-Schmitz commented Oct 29, 2018

Jojo-Schmitz commented Oct 29, 2018

anatoly-os commented Oct 29, 2018

Jojo-Schmitz commented May 22, 2018 •

edited

Jojo-Schmitz May 22, 2018 •

edited

Jojo-Schmitz May 29, 2018 •

edited

Jojo-Schmitz May 30, 2018 •

edited