Fix #302714: make chord symbol playback settings available in Edit Style dialog #5889

IsaacWeiss · 2020-03-30T01:15:06Z

Resolves: https://musescore.org/en/node/302714

As suggested by @Jojo-Schmitz in his PR #5852, this reuses voicing_select.ui from the Inspector. This creates no merge conflicts, and it will work perfectly well if merged independently, but it will look a lot nicer (as in the above screenshot) if his PR is merged first.

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
I created the test (mtest, vtest, script test) to verify the changes I made

Harmoniker1 · 2020-03-30T02:02:16Z

Tests are failing. You named the voicing widget harmonyVoicingWidget in .ui file, different than what's used in .cpp file, that's why ;-)

IsaacWeiss · 2020-03-30T02:30:27Z

Huh, it compiled just fine on my machine. Weird. Update pushed.

Jojo-Schmitz · 2020-03-30T05:54:29Z

Looks great!
Adding a horizontal spacer should improve the layout further (in line with the Formatting part of the dialog)

Jojo-Schmitz · 2020-03-30T05:58:11Z

mscore/editstyle.cpp

+      voicingSelectWidget->durationBox->clear();
+      voicingSelectWidget->durationBox->addItem(tr("Until Next Chord Symbol"), int(HDuration::UNTIL_NEXT_CHORD_SYMBOL));
+      voicingSelectWidget->durationBox->addItem(tr("Until End of Measure"), int(HDuration::STOP_AT_MEASURE_END));
+      voicingSelectWidget->durationBox->addItem(tr("Chord/Rest Duration"), int(HDuration::SEGMENT_DURATION));


Can't you just take the strings from that ui file rather than having them here again, as duplicates?
Like replacing tr("...") with qApp->translate("VoicingSelect","..."). Would still need to be the 100% identical strings, but at least would prevent them from the need to get translated twice.
Maybe even refer to the index of those strings in that dialog rather than having the strings themselves, like voicingSelectWidget->interpretBox->itemText(i), then those won't ever go out of sync.
But maybe none of those are needed at all? Like no need for the lines 427-447 at all?

I think they are needed, as with all the other combobox initializations just above in the same file. Without these lines, for whatever reason, the comboboxes appear empty.

Getting the index sounds like it would be ideal, but I can't seem to get it to work. Using qApp->translate() works (I'd be interested to know exactly what it does differently), so if using the index is not an option I'll switch to that.

It refers to translations done elsewhere (defined via context, the 1st arg)

With the index method, I could conceivably do these initializations in fewer lines by making a vector for each enum (e.g. vector<HDuration> HDurations) and then use a loop (e.g. for auto hDuration : HDurations) to add all the items.

Yes. Seems better anyhow, to prevent the strings from getting out of sync

It would be like voicingSelectWidget->voicingBox->addItem(voicingSelectWidget->voicingBox->itemText(0), int(Voicing::AUTO));, no?

I would guess so, yes.
Well, no. You're deleting them first
voicingSelectWidget->durationBox->clear();
The items are (or should be) there already, just not connected?

Aha! Hold that thought.

Okay, I'm baffled. It appears I was mistaken earlier when I said "Without these lines, for whatever reason, the comboboxes appear empty." In fact, the comboboxes are filled automatically, but they appear empty because no option is selected. So I triedsetCurrentIndex(), but it has no effect.

Actually these indexes (indices?) need to get set as per the current score setting.
But I guess that is what setValues() and getValue() are doing furter down?

Jojo-Schmitz · 2020-03-30T05:58:52Z

mscore/editstyle.cpp

+      voicingSelectWidget->voicingBox->addItem(tr("Automatic"), int(Voicing::AUTO));
+      voicingSelectWidget->voicingBox->addItem(tr("Root Only"), int(Voicing::ROOT_ONLY));
+      voicingSelectWidget->voicingBox->addItem(tr("Close"), int(Voicing::CLOSE));
+      voicingSelectWidget->voicingBox->addItem(tr("Drop 2"), int(Voicing::DROP_2));


My PR changes this to "Drop Two"

Noted, thanks

Jojo-Schmitz · 2020-03-30T12:22:55Z

mscore/editstyle.cpp

+
+      voicingSelectWidget->interpretBox->clear();
+      voicingSelectWidget->interpretBox->addItem(tr("Jazz"), int(0)); // two-item combobox for boolean style variant
+      voicingSelectWidget->interpretBox->addItem(tr("Literal"), int(1)); // true = literal


Why those 2 casts to int?

I started out with false and true there and wanted to be clear.

but Jazz isn't false ;-)
0 and 1 are int already (or turned into that), so the cast seems superfluous, even more so as that method really calls for a QVariant.

According to @peterhieuvu it is ;-)

Ah yeah, Jazz and Literal should probably have been a char/boolean enum instead of true or false to improve readability

mscore/editstyle.cpp

Jojo-Schmitz · 2020-03-30T17:18:12Z

mscore/editstyle.cpp

+      { Sid::harmonyPlay,             false, harmonyPlay,             0 },
+      { Sid::harmonyVoiceLiteral,     false, voicingSelectWidget->interpretBox, 0 },
+      { Sid::harmonyVoicing,          false, voicingSelectWidget->voicingBox, 0 },
+      { Sid::harmonyDuration,         false, voicingSelectWidget->durationBox, 0 },


No reset buttons?
In Inspector at least voicingBox has a reset button
Oh, I see, that reset and set to style button applies to all 3 settings in one go

IsaacWeiss · 2020-04-24T04:20:38Z

So, to recap: this PR tries to reuse an external widget, which is more than can be said for most of the Style dialog. Trying to use the strings provided in that widget I encounter problems of one kind or another (the closest I got was having the comboboxes appear empty with no option selected, and adding setCurrentIndex(0) had no effect at all). Filling the comboboxes afresh, as in the currently committed code, works beautifully, but it seems like it should be unnecessary. I suspect there is a way to reuse the widget in its entirety, but it has escaped me so far. Review from maintainers would be appreciated.

Jojo-Schmitz · 2020-07-19T04:50:48Z

Rebase needed
Maybe rebase to 3.x branch or even separate versions for.3.x and master

Jojo-Schmitz · 2020-08-13T06:35:50Z

@IsaacWeiss this still needs a rebase and a 3.x version

…yle dialog a

IsaacWeiss · 2020-08-13T20:04:01Z

This has been rebased against 3.x and passed all tests. @anatoly-os mentioned on Telegram that he could merge changes from 3.x to master, but that might be difficult with the UI file, so I am happy to take care of that with a second PR if necessary. In resolving changes to editstyle.ui I noticed a whole section of style settings in 3.x currently missing in master, with measure number position above and below. Did this fall through the cracks or is the difference on purpose?

Jojo-Schmitz · 2020-08-14T10:32:59Z

It is because #6208 hasn't yet been merged to master I guess

IsaacWeiss force-pushed the 302714-chord-symbol-playback-style branch from 0780eb0 to c431a3c Compare March 30, 2020 02:30

Jojo-Schmitz reviewed Mar 30, 2020

View reviewed changes

mscore/editstyle.cpp Show resolved Hide resolved

Jojo-Schmitz reviewed Mar 30, 2020

View reviewed changes

njvdberg mentioned this pull request Mar 31, 2020

Fix #293785: Fix #302590: Fix #294890: Chord symbols jump when user goes into edit… #5876

Merged

8 tasks

Harmoniker1 mentioned this pull request Apr 22, 2020

MuseScore 3.5 Alpha PRs to be merged #5960

Closed

This was referenced Jul 21, 2020

MuseScore 3.5.1 Release. PRs to be merged and issues to be fixed. #6355

Closed

preferences to disable chord symbol playback when opening old scores, or creating new ones #6259

Merged

IsaacWeiss changed the base branch from master to 3.x August 13, 2020 14:51

Fix #302714: make chord symbol playback settings available in Edit St…

fec330b

…yle dialog a

IsaacWeiss force-pushed the 302714-chord-symbol-playback-style branch from c431a3c to fec330b Compare August 13, 2020 17:29

anatoly-os merged commit 29b892c into musescore:3.x Aug 25, 2020

Jojo-Schmitz mentioned this pull request Aug 25, 2020

[MU3] Partial fix for #306911 #6255

Merged

anatoly-os mentioned this pull request Aug 25, 2020

[MU4 Issue] PRs from 3.x to be merged into / ported to master #6079

Closed

29 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #302714: make chord symbol playback settings available in Edit Style dialog #5889

Fix #302714: make chord symbol playback settings available in Edit Style dialog #5889

IsaacWeiss commented Mar 30, 2020 •

edited

Loading

Harmoniker1 commented Mar 30, 2020 •

edited

Loading

IsaacWeiss commented Mar 30, 2020

Jojo-Schmitz commented Mar 30, 2020 •

edited

Loading

Jojo-Schmitz Mar 30, 2020 •

edited

Loading

IsaacWeiss Mar 30, 2020

Jojo-Schmitz Mar 30, 2020

IsaacWeiss Mar 30, 2020

Jojo-Schmitz Mar 30, 2020

IsaacWeiss Mar 30, 2020

Jojo-Schmitz Mar 30, 2020 •

edited

Loading

IsaacWeiss Mar 30, 2020

IsaacWeiss Mar 30, 2020

Jojo-Schmitz Mar 30, 2020 •

edited

Loading

Jojo-Schmitz Mar 30, 2020

IsaacWeiss Mar 30, 2020

Jojo-Schmitz Mar 30, 2020

IsaacWeiss Mar 30, 2020

Jojo-Schmitz Mar 30, 2020 •

edited

Loading

IsaacWeiss Mar 30, 2020

peterhieuvu Mar 30, 2020

Jojo-Schmitz Mar 30, 2020 •

edited

Loading

IsaacWeiss commented Apr 24, 2020

Jojo-Schmitz commented Jul 19, 2020

Jojo-Schmitz commented Aug 13, 2020 •

edited

Loading

IsaacWeiss commented Aug 13, 2020

Jojo-Schmitz commented Aug 14, 2020

Fix #302714: make chord symbol playback settings available in Edit Style dialog #5889

Fix #302714: make chord symbol playback settings available in Edit Style dialog #5889

Conversation

IsaacWeiss commented Mar 30, 2020 • edited Loading

Harmoniker1 commented Mar 30, 2020 • edited Loading

IsaacWeiss commented Mar 30, 2020

Jojo-Schmitz commented Mar 30, 2020 • edited Loading

Jojo-Schmitz Mar 30, 2020 • 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

Choose a reason for hiding this comment

Jojo-Schmitz Mar 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jojo-Schmitz Mar 30, 2020 • 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

Jojo-Schmitz Mar 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jojo-Schmitz Mar 30, 2020 • edited Loading

Choose a reason for hiding this comment

IsaacWeiss commented Apr 24, 2020

Jojo-Schmitz commented Jul 19, 2020

Jojo-Schmitz commented Aug 13, 2020 • edited Loading

IsaacWeiss commented Aug 13, 2020

Jojo-Schmitz commented Aug 14, 2020

IsaacWeiss commented Mar 30, 2020 •

edited

Loading

Harmoniker1 commented Mar 30, 2020 •

edited

Loading

Jojo-Schmitz commented Mar 30, 2020 •

edited

Loading

Jojo-Schmitz Mar 30, 2020 •

edited

Loading

Jojo-Schmitz Mar 30, 2020 •

edited

Loading

Jojo-Schmitz Mar 30, 2020 •

edited

Loading

Jojo-Schmitz Mar 30, 2020 •

edited

Loading

Jojo-Schmitz Mar 30, 2020 •

edited

Loading

Jojo-Schmitz commented Aug 13, 2020 •

edited

Loading