GSoC '24 - Dynamics Popup #23038

ketgg · 2024-05-31T00:24:24Z

Pull request for Dynamics Popup Project.

Current Status

Implemented the UI

Added functionality to change the dynamic from popup

Added functionality to draw hairpin when either grip is dragged

Grips are not shown if the dynamic already has a hairpin on either side or if it's tick is zero like fff

Added shortcut to open the popup

I signed the CLA
The title of the PR describes the problem it addresses
Each commit's message describes its purpose and effects, and references the issue it resolves
If changes are extensive, there is a sequence of easily reviewable commits
The code in the PR follows the coding rules
There are no unnecessary changes
The code compiles and runs on my machine, preferably after each commit individually
I created a unit test or vtest to verify the changes I made (if applicable)

cbjeukendrup

First round of code review! I see a lot of good things, which is nice. And I also have a few suggestions to make it even better.

src/notation/view/internal/dynamicpopupmodel.h

src/notation/view/internal/dynamicpopupmodel.cpp

src/notation/view/internal/dynamicpopupmodel.h

src/notation/view/internal/dynamicpopupmodel.cpp

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

src/notation/view/internal/dynamicpopupmodel.cpp

… has a hairpin

…nality to add hairpin from the popup

…e end of hairpin drag

… macOS

…mics (for SFF dynamic as it doesnot have a SymId)

cbjeukendrup

Here is a mostly complete review of the changes in src/engraving. @mike-spa May I ask you to check the remaining methods?

cbjeukendrup · 2024-08-18T14:58:28Z

src/engraving/dom/dynamic.cpp

+    // Right grip (when two grips)
+    if (int(ed.curGrip) == 1 && hasLeftGrip() && hasRightGrip()) {
+        m_rightDragOffset += ed.evtDelta.x();
+        if (rightDragOffset() < 0) {
+            m_rightDragOffset = 0;
+        }
+        return;
+    }
+
+    // Right grip (when single grip)
+    if (int(ed.curGrip) == 0 && !hasLeftGrip() && hasRightGrip()) {
+        m_rightDragOffset += ed.evtDelta.x();
+        if (rightDragOffset() < 0) {
+            m_rightDragOffset = 0;
+        }
+        return;
+    }


Looks like these can be consolidated into one if statement, like

if ((int(ed.curGrip) == 1 && hasLeftGrip() && hasRightGrip()) || (int(ed.curGrip) == 0 && !hasLeftGrip() && hasRightGrip())) {

Also, instead of calling has…Grip again and again, you might want to call them once and use a variable, like this:

const bool hasLeftGrip = this->hasLeftGrip();

Yup, will do that

cbjeukendrup · 2024-08-18T14:59:06Z

src/engraving/dom/dynamic.cpp

+//   findAdjacentHairpins
+//---------------------------------------------------------
+
+void Dynamic::findAdjacentHairpins()


@mike-spa Please check this method

cbjeukendrup · 2024-08-18T15:01:11Z

src/engraving/dom/dynamic.cpp

+        }
+    }
+
+    return;


Redundant return statement

Oh, I forgot to remove it

cbjeukendrup · 2024-08-18T15:02:41Z

src/engraving/dom/dynamic.cpp

+
+    if (hasLeftGrip() && hasRightGrip()) {
+        return 2;
+    } else if (hasLeftGrip() ^ hasRightGrip()) {


The && case has already been checked, so no need to use xor here. The use of ^ is a bit questionable because officially it's a bitwise operator and not a boolean operator.

Suggested change

} else if (hasLeftGrip() ^ hasRightGrip()) {

} else if (hasLeftGrip() || hasRightGrip()) {

Also, the same applies about not calling has…Grip again and again.

Got it, will use OR here

cbjeukendrup · 2024-08-18T15:15:31Z

src/engraving/dom/dynamic.cpp

+    PointF leftOffset(-ldata->bbox().width() / 2 - md + m_leftDragOffset, -11.408);
+    PointF rightOffset(ldata->bbox().width() / 2 + md + m_rightDragOffset, -11.408);


The number -11.408 is what we call a "magic number". When a cryptic hardcoded number is necessary, it is usually a good idea to extract it into a named constant, sometimes accompanied by a comment about how this number was found.
But in this case, I think it should not be a hardcoded number, but computed based on the height of the Dynamic, or perhaps some other parameter like the y-value of the visual center. It should namely also work well with different fonts, font sizes, spatium values.
@mike-spa Could you suggest a good way to compute the vertical grip positions?

@cbjeukendrup I calculated it by subtracting the pagePos of the dynamic from the position of Grip::START from HairpinSegment::gripsPositions and it turned out to be a constant value. I wasn't sure how this could be computed, so I hardcoded it.

cbjeukendrup · 2024-08-18T15:30:29Z

src/engraving/dom/dynamic.h

@@ -25,6 +25,8 @@

 #include "textbase.h"

+#include "draw/painter.h"


Instead of including the header here, you could include it in the cpp file, and here in the header file only write

namespace muse::draw { class Painter; }

cbjeukendrup · 2024-08-18T15:35:42Z

src/engraving/dom/edit.cpp

+        }
+        textBox = Factory::createDynamic(dummy()->segment());
+        chordRest->undoAddAnnotation(textBox);
+        textBox->setFlag(ElementFlag::IS_PREVIEW, true);


I think it would be better to set this flag in the popup code. The popup is namely the only thing that will create a preview dynamic, so it would be good to keep the code that sets the flag close to the code that creates the preview dynamic.

(Also, when splitting this PR into smaller PRs, you'll need to do that anyway, because in the PR about the popup, this code won't exist yet 🙂)

cbjeukendrup · 2024-08-18T15:45:33Z

src/engraving/dom/edit.cpp

@@ -3870,6 +3880,33 @@ void Score::addHairpinToDynamic(Hairpin* hairpin, Dynamic* dynamic)
    undoAddElement(hairpin);
 }

+void Score::addHairpinOnGripDrag(Hairpin* hairpin, Dynamic* dynamic, const PointF& pos, Dynamic::Grip grip)


@mike-spa Please check this method

cbjeukendrup · 2024-08-18T15:46:59Z

src/engraving/dom/textbase.cpp

+    // Don't draw cursor if it's dynamic or if there is a selection
+    if (!ed.element->isDynamic() && !cursor->hasSelection()) {


This is questionable; for existing scores, dynamics may also contain normal text besides the dynamic symbol. So the user should also be able to edit that normal text, so a text cursor would be required.

cbjeukendrup · 2024-08-18T15:48:13Z

src/engraving/dom/textbase.cpp

+    RectF r;
+
+    if (ed.element->isDynamic()) {
+        r = toDynamic(ed.element)->adjustedBoundingRect();
+    } else {
+        double m = spatium();
+        r = canvasBoundingRect().adjusted(-m, -m, m, m);
+    }


Here applies what I said about checking whether we really need the adjustedBoundingRect special case

cbjeukendrup

Here is everything for notation/view

src/notation/view/internal/dynamicpopupmodel.cpp

src/notation/view/internal/dynamicpopupmodel.h

src/notation/view/internal/dynamicpopupmodel.cpp

cbjeukendrup · 2024-08-19T00:36:11Z

src/notation/view/notationviewinputcontroller.cpp

@@ -138,7 +138,11 @@ void NotationViewInputController::onNotationChanged()
        m_view->hideElementPopup();

        if (AbstractElementPopupModel::supportsPopup(type)) {
-            m_view->showElementPopup(type, selectedItem->canvasBoundingRect());
+            if (selectedItem->isDynamic()) {
+                m_view->showElementPopup(type, toDynamic(selectedItem)->adjustedBoundingRect());


The same doubt again about adjustedBoundingRect. You could decide to move all changes related to the addition of this method into yet another separate PR, so that the initial popup functionality PR will be easy to merge.

cbjeukendrup · 2024-08-19T00:39:27Z

src/notation/view/notationviewinputcontroller.cpp

@@ -877,6 +881,10 @@ void NotationViewInputController::mouseReleaseEvent(QMouseEvent* event)

    if (interaction->isDragStarted()) {
        interaction->endDrag();
+        // When dragging of hairpin ends on a note or rest, open dynamic popup


on a note or rest

I don't see this reflected in the actual condition in the code. Is it a requirement at all?

Actually the check for note or rest happens here in Score::addText which is called through addTextToItem in toggleDynamicPopup

Ah okay. Still it might be better to adjust the comment, because it's a bit prone to causing confusion.

cbjeukendrup · 2024-08-19T00:42:36Z

src/notation/view/notationviewinputcontroller.cpp

@@ -877,6 +881,10 @@ void NotationViewInputController::mouseReleaseEvent(QMouseEvent* event)

    if (interaction->isDragStarted()) {
        interaction->endDrag();
+        // When dragging of hairpin ends on a note or rest, open dynamic popup
+        if (interaction->selection()->element()->isHairpinSegment()) {


On the one hand, this feels a bit like an ad-hoc special case. On the other hand, I don't quite see a better way for this, currently. @mike-spa do you have ideas?

cbjeukendrup · 2024-08-19T00:44:29Z

src/notation/view/abstractnotationpaintview.cpp

@@ -546,7 +546,11 @@ void AbstractNotationPaintView::showElementPopup(const ElementType& elementType,

    PopupModelType modelType = AbstractElementPopupModel::modelTypeFromElement(elementType);

-    emit showElementPopupRequested(modelType, fromLogical(elementRect).toQRectF());
+    if (elementType == ElementType::DYNAMIC) {
+        emit showElementPopupRequested(modelType, fromLogical(elementRect).toQRect()); // toQRect() is used in AbstractElementPopupModel::updateItemRect()


I don't quite understand this change. Is it necessary?

@cbjeukendrup Initially when we click on dynamic showElementPopupRequested is emitted which if you notice uses toQRectF() meaning that the y value will be a decimal which will be used to position the popup. But, the updateItemRect method (which is called on changing the dynamic) returns a QRect and not QRectF so the value after the decimal will be lost, meaning that it will shift the popup a little higher than initially positioned as seen in below video. (It's not very noticeable but still...)

2024-08-19.08-57-17.mp4

I was thinking if we can change updateItemRect to use QRectF (or, in general use QRect in showElementPopupRequested), so its in sync?

Ah, I see! Yes, updating updateItemRect and m_itemRect to use QRectF seems good to me.

cbjeukendrup · 2024-08-19T00:45:51Z

src/notation/view/abstractelementpopupmodel.cpp

+        rect = QRect();
+    } else {
+        if (m_item->isDynamic()) {
+            rect = fromLogical(toDynamic(m_item)->adjustedBoundingRect()).toQRect();


Sorry, same thing again...

It is used to place the popup below the adjustedBoundingRectangle of dynamic like you can see in the in the below image, even when we zoom in it is placed consistently below the rectangle.

If we use the canvasBoundingRect the popup will be inside the bounding rectangle on zooming (even if I add some value to root.y of updatePosition in Qml file it was not getting placed consistently on zooming, so thats why I used adjustedBoudingRect instead of canvasBoundingRect as the elementRect)

I see... @mike-spa do you have ideas about how this can be generalised? A bit like hitShape(), but different...

Also, since all TextBase elements have this grey box around them while editing their text, it might make sense to apply this to all of them. But only during actual text editing.

cbjeukendrup marked this pull request as draft May 31, 2024 02:38

ketgg force-pushed the gsoc_dynamics_popup branch 4 times, most recently from 5904da1 to 0565ec7 Compare June 13, 2024 08:50

cbjeukendrup added the GSoC PRs created by GSoC participants during coding period label Jun 16, 2024

cbjeukendrup reviewed Jun 16, 2024

View reviewed changes

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

ketgg force-pushed the gsoc_dynamics_popup branch from 0565ec7 to fed4c93 Compare June 17, 2024 09:51

avvvvve mentioned this pull request Jun 18, 2024

Replacing a dynamic via palettes should not overwrite its properties #23285

Closed

ketgg force-pushed the gsoc_dynamics_popup branch 6 times, most recently from 1e6a203 to 81579c2 Compare August 5, 2024 14:16

This comment was marked as resolved.

Sign in to view

ketgg force-pushed the gsoc_dynamics_popup branch 3 times, most recently from cf50bc6 to e081307 Compare August 13, 2024 05:48

ketgg added 7 commits August 14, 2024 17:46

Add basic dynamics popup

d18a656

Add UI of the popup

2e93053

Add proper width and offsets to dynamics

f555cf9

Add hairpins to popup, use FlatButton approach

1f0120b

Review changes

f1a6ee7

Add functionality to change dynamic

752bc2b

Add drag handles on dynamic

f25790b

ketgg added 18 commits August 14, 2024 17:46

Add drag functionality for right grip

3713efd

Add drag functionality for left grip, Disable grip if dynamic already…

abfc291

… has a hairpin

Add functionality to change hairpin type using shift key, and functio…

96388e7

…nality to add hairpin from the popup

Fix rebase issues and merge conflicts

2d85c7b

Add dynamic shortcut

5743125

Fix codestyle

7571fca

Modify popup positioning and bounding rectangle

5bff9d0

Fix codestyle

29ef36f

Show preview on hovering a dynamic

b222ff6

Fix codestyle

9f6d97f

Add IS_PREVIEW ElementFlag

51d4270

Show popup when hairpin drag ends on a note or rest

cdd386a

Fix anchor lines not showing up on dragging dynamic

bdc9638

Select an existing dynamic instead of opening a new empty popup at th…

33a9f4e

…e end of hairpin drag

Fix issue with dynamic markings not displaying correctly on Linux and…

0a5f284

… macOS

Use score font for popup

bf23503

Automatic switching of hairpin type

2dbd74e

Flip hairpins type on changing existing dynamic

d47c49d

ketgg force-pushed the gsoc_dynamics_popup branch from e081307 to d47c49d Compare August 14, 2024 15:26

Fix automatic flip of hairpins, Add different approach to render dyna…

7c52f6e

…mics (for SFF dynamic as it doesnot have a SymId)

cbjeukendrup reviewed Aug 18, 2024

View reviewed changes

cbjeukendrup reviewed Aug 19, 2024

View reviewed changes

This was referenced Aug 21, 2024

GSoC '24 - Dynamics Popup - Part 1 #24125

Open

GSoC '24 - Dynamics Popup - Part 2 #24147

Open

GSoC '24 - Dynamics Popup - Part 3 #24152

Open

GSoC '24 - Dynamics Popup - Part 4 #24159

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GSoC '24 - Dynamics Popup #23038

GSoC '24 - Dynamics Popup #23038

ketgg commented May 31, 2024 •

edited

Loading

cbjeukendrup left a comment

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

cbjeukendrup left a comment

cbjeukendrup Aug 18, 2024

ketgg Aug 19, 2024

cbjeukendrup Aug 18, 2024

cbjeukendrup Aug 18, 2024

ketgg Aug 19, 2024

cbjeukendrup Aug 18, 2024

ketgg Aug 19, 2024

cbjeukendrup Aug 18, 2024

ketgg Aug 19, 2024

cbjeukendrup Aug 18, 2024

cbjeukendrup Aug 18, 2024

cbjeukendrup Aug 18, 2024

cbjeukendrup Aug 18, 2024

cbjeukendrup Aug 18, 2024

cbjeukendrup left a comment

cbjeukendrup Aug 19, 2024

cbjeukendrup Aug 19, 2024

ketgg Aug 19, 2024

cbjeukendrup Aug 21, 2024

cbjeukendrup Aug 19, 2024

cbjeukendrup Aug 19, 2024

ketgg Aug 19, 2024 •

edited

Loading

cbjeukendrup Aug 19, 2024

cbjeukendrup Aug 19, 2024

ketgg Aug 19, 2024

cbjeukendrup Aug 21, 2024

+                      }
+                  }
+                  return;

	} else if (hasLeftGrip() ^ hasRightGrip()) {
	} else if (hasLeftGrip() \|\| hasRightGrip()) {

		PointF leftOffset(-ldata->bbox().width() / 2 - md + m_leftDragOffset, -11.408);
		PointF rightOffset(ldata->bbox().width() / 2 + md + m_rightDragOffset, -11.408);

		@@ -25,6 +25,8 @@

		#include "textbase.h"

		#include "draw/painter.h"

		// Don't draw cursor if it's dynamic or if there is a selection
		if (!ed.element->isDynamic() && !cursor->hasSelection()) {

GSoC '24 - Dynamics Popup #23038

Are you sure you want to change the base?

GSoC '24 - Dynamics Popup #23038

Conversation

ketgg commented May 31, 2024 • edited Loading

cbjeukendrup left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

cbjeukendrup left a 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

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

cbjeukendrup left a 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

ketgg Aug 19, 2024 • 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

ketgg commented May 31, 2024 •

edited

Loading

ketgg Aug 19, 2024 •

edited

Loading