Harp pedalling diagrams and UI #12597

miiizen · 2022-07-29T14:39:34Z

This pull request adds harp pedalling diagrams to MuseScore for GSoC 2022.

Known issues

Opening via shortcut
Stacking of diagrams around other elements
Placement of the popup on the score
The arrow at the top of the UI
Probably some spacing within the UI

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

src/notation/qml/MuseScore/NotationScene/internal/ElementPopupLoader.qml

src/engraving/libmscore/note.cpp

cbjeukendrup

Some (probably last) minor comments about everything except the changes in the notation module! (those I will review later)

src/engraving/libmscore/chordrest.cpp

src/engraving/libmscore/engravingitem.h

src/engraving/libmscore/harppedaldiagram.cpp

src/palette/internal/palettecreator.cpp

cbjeukendrup · 2022-08-25T00:10:08Z

This PR needs a rebase too; this time all conflicts are just about the #includes.

src/engraving/libmscore/select.cpp

src/engraving/libmscore/harppedaldiagram.h

src/engraving/libmscore/harppedaldiagram.cpp

src/notation/qml/MuseScore/NotationScene/internal/HarpPedalPopup.qml

src/notation/qml/MuseScore/NotationScene/internal/HarpPedalGridColumn.qml

src/notation/qml/MuseScore/NotationScene/internal/HarpPedalPopup.qml

src/notation/qml/MuseScore/NotationScene/internal/HarpPedalGridColumn.qml

src/engraving/libmscore/harppedaldiagram.cpp

cbjeukendrup · 2023-03-20T13:50:51Z

Hi @miiizen! We have started working on MuseScore 4.1, and we would like to include this PR. It would be great if you could rebase it. I've also posted a few review comments that I had written earlier but apparently not yet published.

miiizen · 2023-03-20T14:50:58Z

Hi @cbjeukendrup! That's great news thanks for letting me know. I'll have time to rebase and make these changes around the end of next week.

src/notation/view/internal/harppedalpopupmodel.cpp

cbjeukendrup

Another round of review comments! It's really very good now, so this is probably the last review round from me and then I'll ask a colleague to check it too.

src/engraving/libmscore/part.cpp

src/notation/qml/MuseScore/NotationScene/internal/HarpPedalPopup.qml

src/engraving/libmscore/harppedaldiagram.cpp

Tantacrul · 2023-04-28T22:22:44Z

Had a poke at the diagram to see how things are going with it. Found a few things and thought I'd share. Sorry if this is a bit premature and you were planning on asking us to test later.

Harp_Ped_01.mp4

cbjeukendrup · 2023-05-06T12:36:17Z

@miiizen A rebase is needed again...

RomanPudashkin · 2023-05-10T16:44:25Z

src/engraving/libmscore/harppedaldiagram.h

+    OBJECT_ALLOCATOR(engraving, HarpPedalDiagram)
+    DECLARE_CLASSOF(ElementType::HARP_DIAGRAM)
+
+    std::array<PedalPosition, 7> _pedalState;


I would suggest adding a named constant and using it instead of this magic number

RomanPudashkin · 2023-05-10T16:45:07Z

src/engraving/libmscore/harppedaldiagram.h

+#include "pitchspelling.h"
+
+using namespace mu;
+//using namespace mu::engraving;


Please remove this line

RomanPudashkin · 2023-05-10T16:55:27Z

This PR looks great (and ready to be merged), thank you so much! The only issue that it has is the broken unit tests. If you can't fix them yourself (or don't have enough time), I suggest disabling them (I'll fix them later). Or you can try rebasing this PR one more time, maybe this will help

…ges made for undo, text styles, score interaction.

…gram class and integration with the undo stack

miiizen · 2023-05-11T20:11:55Z

Thanks for the feedback!
Rebasing hasn't fixed them - afraid I won't have time to try to fix the tests til after Tuesday, feel free to have a look yourself.

cbjeukendrup · 2023-05-11T23:24:58Z

Fortunately, I think the test failures are no mystery at all. The default z index of an element is defined as its ElementType casted to int multiplied by 100. Because the HARP_DIAGRAM type got inserted, the default z-index for images was incremented (because the IMAGE element type comes after HARP_DIAGRAM).

The reason that the z index is written to the file in those specific files where the tests fail, is that it is "customised" in those places. And the reason that it is customised and gets changed while reading/writing the score, is BSymbol::add (and Image is derived from BSymbol). What happens is that when reading the score, the z index of the inner image gets set to the z index of the outer image minus one, regardless of what's written in the file. The z index of the outer image was not customised, so will use the new default value. Therefore the z index of the inner image will be changed too.

So the fix would be to just update the contents of src/engraving/tests/parts_data/part-image.mscx and src/engraving/tests/parts_data/part-image-parts.mscx according to https://github.com/musescore/MuseScore/actions/runs/4952108865/jobs/8857989773?pr=12597#step:9:3323

(Of course, this behaviour of changing z index while reading is very questionable, but that's a separate problem.)

See: musescore#12597 (comment)

miiizen commented Aug 4, 2022

View reviewed changes

src/notation/qml/MuseScore/NotationScene/internal/ElementPopupLoader.qml Outdated Show resolved Hide resolved

miiizen force-pushed the harp-pedal-ui branch 4 times, most recently from 3ffc6d6 to e55ff70 Compare August 16, 2022 18:09

miiizen commented Aug 22, 2022

View reviewed changes

src/engraving/libmscore/note.cpp Show resolved Hide resolved

cbjeukendrup mentioned this pull request Aug 24, 2022

Harp pedalling diagrams and harp notation palette #12269

Closed

16 tasks

cbjeukendrup reviewed Aug 25, 2022

View reviewed changes

miiizen force-pushed the harp-pedal-ui branch from a94e220 to a9ecb98 Compare August 31, 2022 22:11

cbjeukendrup reviewed Sep 1, 2022

View reviewed changes

src/engraving/libmscore/select.cpp Show resolved Hide resolved

miiizen force-pushed the harp-pedal-ui branch 2 times, most recently from 84f7718 to ea6326d Compare September 9, 2022 17:47

cbjeukendrup added the vtests This PR produces approved changes to vtest results label Sep 17, 2022

bkunda assigned Tantacrul Mar 14, 2023

bkunda added the needs review The issue needs review to set priority, fix version or change status etc. label Mar 14, 2023

cbjeukendrup reviewed Mar 20, 2023

View reviewed changes

miiizen force-pushed the harp-pedal-ui branch from ea6326d to b2985bf Compare April 3, 2023 09:03

miiizen commented Apr 3, 2023

View reviewed changes

src/notation/view/internal/harppedalpopupmodel.cpp Outdated Show resolved Hide resolved

cbjeukendrup reviewed Apr 26, 2023

View reviewed changes

src/engraving/libmscore/part.cpp Show resolved Hide resolved

src/notation/qml/MuseScore/NotationScene/internal/HarpPedalPopup.qml Outdated Show resolved Hide resolved

src/engraving/libmscore/harppedaldiagram.cpp Outdated Show resolved Hide resolved

miiizen force-pushed the harp-pedal-ui branch from 9ed856c to b09d84d Compare April 28, 2023 10:51

miiizen force-pushed the harp-pedal-ui branch from b09d84d to 0894cee Compare May 6, 2023 16:00

RomanPudashkin reviewed May 10, 2023

View reviewed changes

RomanPudashkin approved these changes May 10, 2023

View reviewed changes

miiizen added 20 commits May 11, 2023 20:51

Created base class for harp pedal diagram

0453bd4

Add palette for harp notation

2b5ccc9

Created pedal settings element with diagram and text variations. Chan…

f749686

…ges made for undo, text styles, score interaction.

Element popups viewable in the score

3fff74c

Created pedalling UI. This includes necessary changes to HarpPedalDia…

5c1d26f

…gram class and integration with the undo stack

Added proofreading via notehead colours

b48115a

Create utests and vtest

05f3137

Refactor into grid

598ac6f

Fix diagram overlapping top of window

7faec3f

Fix linux build fail

7cacea4

Use unicode strings in test

0ea9cfb

Move harp pedal diagrams' xml read to TRead

d755c50

Refine popup positioning

d6ba50d

update reading & writing diagrams

8140fec

Move pedal diagrams when time is added

55c62df

change accessible panel name

1f54f73

update tread/twrite

81afdf6

Prevent adding diagram to segment where one already exists

b10604b

Create elementHasPopup method in notationactioncontroller

7869073

Replace magic 7 with constant

35a41cb

miiizen force-pushed the harp-pedal-ui branch from 0894cee to 35a41cb Compare May 11, 2023 20:12

miiizen marked this pull request as ready for review May 11, 2023 20:12

RomanPudashkin merged commit dea7ce2 into musescore:master May 12, 2023

RomanPudashkin added a commit to RomanPudashkin/MuseScore that referenced this pull request May 12, 2023

fix utests

12b307d

See: musescore#12597 (comment)

RomanPudashkin added a commit to RomanPudashkin/MuseScore that referenced this pull request May 12, 2023

fix utests

a27ef72

See: musescore#12597 (comment)

RomanPudashkin mentioned this pull request May 12, 2023

utests_fix #17537

Merged

bkunda mentioned this pull request May 15, 2023

Minor fixes for harp pedalling widget #17565

Closed

miiizen deleted the harp-pedal-ui branch January 9, 2024 15:09

Harp pedalling diagrams and UI #12597

Harp pedalling diagrams and UI #12597

Uh oh!

Conversation

miiizen commented Jul 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cbjeukendrup left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cbjeukendrup commented Aug 25, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cbjeukendrup commented Mar 20, 2023

Uh oh!

miiizen commented Mar 20, 2023

Uh oh!

Uh oh!

cbjeukendrup left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Tantacrul commented Apr 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cbjeukendrup commented May 6, 2023

Uh oh!

RomanPudashkin May 10, 2023

Choose a reason for hiding this comment

Uh oh!

RomanPudashkin May 10, 2023

Choose a reason for hiding this comment

Uh oh!

RomanPudashkin commented May 10, 2023

Uh oh!

miiizen commented May 11, 2023

Uh oh!

cbjeukendrup commented May 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

miiizen commented Jul 29, 2022 •

edited

Loading

Tantacrul commented Apr 28, 2023 •

edited

Loading

cbjeukendrup commented May 11, 2023 •

edited

Loading