Tempo Popup #8524

sidharth-anand · 2021-07-05T09:05:22Z

Features

The popup is fully functional and supports user input through all 3 tabs.
The tempo marking is also modified to prevent user input inside the equation.

tempo.popup.mp4

Note: This PR depends on #8308

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

src/engraving/libmscore/tempotext.cpp

Tantacrul · 2021-07-25T09:51:48Z

I have a few thoughts for improvements:

Insertion point

The first thing is the insertion point (for text editing). It looks weird as a rectangle that doesn't flash on an off. For that reason, it looks like an unintended artefact, like an uppercase 'i'. It would make a lot more sense if it would flash on and off.
When the user clicks on a text field in the popup, the insertion point in the tempo marking should disappear. Then if the user single-clicks back on the tempo marking again (because we're still in an 'edit' mode) the insertion point should re-appear there, and be removed from the popup. This would help the user understand what is in focus. Having two visible insertion points is quite strange.

Tantacrul · 2021-07-25T10:05:36Z

Placement of Popup

If you trigger the popup and then move the score around a little, then open the popup again, you'll find that it gets misaligned pretty easily. It would be great if it would always be centred to the position of the tempo marking.
The vertical positioning is a bit weird too. If the tempo marking appears in the middle of the screen, then the popup looks OK. However, if the tempo marking is towards the top of my screen, the popup appears to be too far away. If I scroll so the tempo marking appears to be at the bottom of my screen, then the popup appears to the right of it, which looks weirder.

Top of my screen:

Bottom of screen:

I would suggest the popup is a good bit closer, like this:

And if there is not enough visible room under the tempo marking, then I think the idea of keeping it to the right makes sense and I would remove the upwards pointing arrow and keep it closer to the marking itself.

Tantacrul · 2021-07-25T10:15:16Z

Spacing of popup

Just a few little things here and there:

There is too little padding between the tabs and the content underneath. Generally, there is a 12px gap between sections, like this:
This is particularly bad with the 'Code' tab, where the title directly under it is overlapping the same space. Here's a visual correction I've made to give you an idea of how it should look

Tantacrul · 2021-07-25T10:20:01Z

Other visual issues with Popup

I've noticed that whole wholenote and double-wholenote symbols are weird looking in the context menu.

I've created two new symbols in the MuseScoreIcon font (not Leland), which you should use instead for the context menu items instead. These are specifically for vertical display. The unicodes are F3D3 & F3D4 respectively.

Tantacrul · 2021-07-25T10:24:13Z

Wrong icons used for Whole Note and Double Whole note in metronome marking.

These look to be too small and are pointing at Bravura. The correct symbols are found in LelandText: ECA2 and ECA0.

@oktophonie can confirm

Tantacrul · 2021-07-25T10:30:51Z

Wrong visuals for selected tab state

Currently, the text in a selected / unselected tabs are inverted. A selected tab should be bolder, less opaque. Basically, you just need to swap the stylings around.

This is how it looks now:

This is how it should look (see Figma for reference)

Tantacrul · 2021-07-25T10:34:59Z

Bug

'Visibility' sometimes requires being toggled twice for it to work.

Create tempo marking
Double click to get the popup
Type something in the main tempo text field (like Andante, etc.)
Press the 'visibility' toggle in the popup (to off)
Observe it does not work
Turn it on and off again
Now it works

Tantacrul · 2021-07-25T10:36:02Z

Last comment: I don't think a user should be able to press the backspace button to delete portions of the text that are controlled by the popup.

sidharth-anand · 2021-08-04T08:57:02Z

1. If you trigger the popup and then move the score around a little, then open the popup again, you'll find that it gets misaligned pretty easily. It would be great if it would always be centred to the position of the tempo marking.

I'm not able to reproduce this. The popup seems to be centered to the element irrespective of whether i move the score, or scroll

I've fixed most of the other issues and will be updating the PR soon

Jojo-Schmitz · 2021-08-04T09:05:43Z

A rebase is needed too

Tantacrul · 2021-08-15T14:45:04Z

Wow. This is a big improvement!

One bug I noticed about 'visibility': If you put text before and after a tempo marking (see image) and then turn the visibility off and back on again, it forgets the position of the tempo marking and puts it at the end.

Before:

After:

Tantacrul · 2021-08-15T14:50:05Z

I'm also curious about what problems you are facing with the whole / double whole note symbols.

Just in case, here's the latest version of the font (make sure to uninstall any versions on your machine, btw)
https://www.dropbox.com/s/ip59ren10u69hr7/MusescoreIcon.ttf?dl=0

Unicodes are F3D3 & F3F4

If you continue to have issues with them, let me know on Discord.

Tantacrul · 2021-08-15T14:56:16Z

One last thing:

Some of the metronome marks look wrong (often far too small).

For example, compare this 16th, which is correct:

With this 32nd, which is far too small (and defaulting to Bravura, which is the backup, implying it is pointing at the wrong font):

The same goes for the whole note and double whole note:

I'm going to guess that you are telling it to look in 'Leland', whereas I believe they should be looking at 'Leland Text'.

Incidentally, make sure to use the latest version of both Leland and Leland text.
https://github.com/MuseScoreFonts/Leland

The best person to speak to about all of the icon stuff is definitely @oktophonie

Thanks a lot again!

Jojo-Schmitz · 2021-08-17T10:06:36Z

src/importexport/ove/internal/importove.cpp

@@ -1044,11 +1093,8 @@ TDuration OveNoteType_To_Duration(ovebase::NoteType noteType)
 //      break;
 //  }


these 8 lines should get removed, they apparently have been errnously duplicated.
Or (probably better) change to match the comments above, // return "w";

Jojo-Schmitz · 2021-08-17T10:08:59Z

src/palette/internal/palettecreator.cpp

-                     QT_TRANSLATE_NOOP("palette", "Dotted eighth note = quarter note metric modulation"),
-                     2.0 / 3.0, true, false, true, false, false),
+    static const TempoPattern tempoPatterns[] = {
+        TempoPattern("", "h = 80", "Half notes = 80 BPM"),


We're loosing their translatability (of tooltips IIRC)

Jojo-Schmitz · 2021-08-19T10:17:25Z

src/framework/uicomponents/qml/MuseScore/UiComponents/StyledPopup.qml


    property alias navigation: navPanel
    property bool isDoActiveParentOnClose: true

+


excessive whitespace

bkunda · 2021-09-06T11:37:29Z

1. If you trigger the popup and then move the score around a little, then open the popup again, you'll find that it gets misaligned pretty easily. It would be great if it would always be centred to the position of the tempo marking.
I'm not able to reproduce this. The popup seems to be centered to the element irrespective of whether i move the score, or scroll

I've fixed most of the other issues and will be updating the PR soon

I was able to reproduce @Tantacrul's issue here. See demo video:

popup-visibility.mov

In my case, the popup appears spatially offset from the tempo mark in different ways, depending on the zoom level. At some zoom levels, the popup completely masks the tempo text, and it is impossible to move it away.

I also note that the triangular element on the top border of the popup's frame is not rendering in MacOS.

bkunda · 2021-09-06T11:42:42Z

And apologies - did not mean to close this issue! 🤦‍♂️

Jojo-Schmitz · 2021-09-06T12:08:26Z

rebase needed

bkunda · 2021-09-06T12:10:49Z

Tempo text without a bpm value
An interesting issue here!
This observation relates to tempo text containing no bpm value (e.g. "Allegretto" or "Vivace", as opposed to "Allegro♩=120").

See this video for a full explainer: https://www.dropbox.com/s/2regyildnhthwxm/no-bpm-text-issue.mov?dl=0

In summary:

If making any adjustments to the bpm value of the tempo text is to trigger visibility of the bpm value itself (as it currently does), then the visibility checkbox should automatically be selected (which currently doesn't happen)
I did not find the relationship between the visibility checkbox and the tempo ("bpm") section to be sufficiently clear, and might suggest a slight tweaking of the checkbox's placement:

(Note: I've also suggested a slightly more descriptive label (image on the right), which might just make the function that bit clearer).

@Tantacrul let me know your thoughts on this. I realise it's your design baby! 😬

shoogle · 2021-09-06T12:35:35Z

src/engraving/tests/keysig_data/keysig.mscx

+            <equation>q = 100</equation>
+            <equationVisible>1</equationVisible>
+            <followText>1</followText>
            <text>𝅘𝅥 = 100</text>


Is there a reason for keeping the metronome marking in the <text> element now that you have the <equation> element?

Also, I originally suggested this for the MSCX syntax:

<text>Some text <equation>q = 100</equation> some more text</text>

This allows text after the equation. Is there a reason you chose not to do it this way?

Tantacrul · 2021-09-06T12:49:37Z

@Tantacrul let me know your thoughts on this. I realise it's your design baby! 😬

Please! Go to town and don't worry about me. I agree with your suggestions

bkunda · 2021-09-06T12:53:09Z

@Tantacrul let me know your thoughts on this. I realise it's your design baby! 😬

Please! Go to town and don't worry about me. I agree with your suggestions

All good then! 😌

bkunda · 2021-09-06T13:08:56Z

A (minor) spacing issue AND Backspace key functionality

By default, a space should be inserted between the descriptive text ("Allegro") and the tempo/bpm text.

Current default spacing:

Desired default spacing:

Incidentally, I was able to create the second image above using the spacebar. Noting @Tantacrul's comment above re: the backspace key, I might suggest that, for consistency's sake, the spacebar also be rendered unusable....

...OR...

They both be rendered usable. The problem here is that, I can currently add spaces between elements, but I cannot remove them. If I accidentally, or even intentionally, add more spaces than I end up wanting, I can't remove them without deleting the tempo text and starting again.

A further argument in favour of keeping keyboard keys usable: I currently can enter a line space using the return key, in order to generate this (often desirable) result:

It is a problem for me that some keys work while other keys don't.

My vote would be to reinstate the backspace key functionality, particularly as it allows the user to more liberally modify the text imported from the palette (For example, adding the word "Molto" before something like "Vivace"; It feels intuitive to be able to make modifications like this directly in the tempo text itself, and having the standard keyboard keys like backspace and spacebar at your disposal is an important part of this experience).

bkunda · 2021-09-06T13:11:23Z

A (minor) spacing issue

By default, a space should be inserted between the descriptive text ("Allegro") and the tempo/bpm text.

Current default spacing:

Desired default spacing:

Incidentally, I was able to create the second image above using the spacebar. Noting @Tantacrul's comment above re: the backspace key, I might suggest that, for consistency's sake, the spacebar also be rendered unusable....

...OR...

They both be rendered usable. The problem here is that, I can currently add spaces between elements, but I cannot remove them. If I accidentally, or even intentionally, add more spaces than I end up wanting, I can't remove them without deleting the tempo text and starting again.

A further argument in favour of keeping keyboard keys usable: I currently can enter a line space using the return key, in order to generate this (often desirable) result:

It is a problem for me that some keys work while other keys don't.

My vote would be to reinstate the backspace key functionality, particularly as it allows the user to more liberally modify the text imported from the palette (For example, adding the word "Molto" before something like "Vivace"; It feels intuitive to be able to make modifications like this directly in the tempo text itself, and having the standard keyboard keys like backspace and spacebar at your disposal is an important part of this experience).

Please also note a bug to fix: tempo text font is changing unexpectedly (as in the third image of this comment) after a few goes at making modifications in the popup.

bkunda · 2021-09-07T08:22:03Z

Hi again 😀
Just some further (minor) visual fixes to do.
Alignment is currently off-centre for most elements (in MacOS build), and there are some incorrect sizes set for the text box and equal sign:

I believe the Figma file should have the correct spacing, and @jessjwilliamson can also help if there are any questions about exactly how many pixels should be where.

Jojo-Schmitz · 2021-09-15T06:10:12Z

rebase needed

sidharth-anand marked this pull request as draft July 5, 2021 09:06

Jojo-Schmitz reviewed Jul 5, 2021

View reviewed changes

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

Jojo-Schmitz reviewed Jul 5, 2021

View reviewed changes

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

sidharth-anand force-pushed the tempo-popup branch 6 times, most recently from e5c1238 to cf98f55 Compare July 18, 2021 15:38

sidharth-anand marked this pull request as ready for review July 19, 2021 04:21

sidharth-anand force-pushed the tempo-popup branch 4 times, most recently from 805eedd to 3d5d29f Compare July 19, 2021 11:32

sidharth-anand force-pushed the tempo-popup branch 2 times, most recently from 0b73123 to 0a428db Compare August 15, 2021 13:03

sidharth-anand force-pushed the tempo-popup branch from 0a428db to 7909795 Compare August 15, 2021 17:03

Element Popups

ac272b0

sidharth-anand force-pushed the tempo-popup branch 4 times, most recently from 2e310a9 to 21b5144 Compare August 17, 2021 08:46

Tempo Popup

1092b62

sidharth-anand force-pushed the tempo-popup branch from 21b5144 to 1092b62 Compare August 17, 2021 09:11

Jojo-Schmitz reviewed Aug 17, 2021

View reviewed changes

Jojo-Schmitz reviewed Aug 19, 2021

View reviewed changes

bkunda closed this Sep 6, 2021

bkunda reopened this Sep 6, 2021

shoogle reviewed Sep 6, 2021

View reviewed changes

cbjeukendrup added the GSoC PRs created by GSoC participants during coding period label Jul 17, 2023

igorkorsukov force-pushed the master branch 6 times, most recently from fa1f8d3 to 525a11a Compare February 14, 2024 09:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tempo Popup #8524

Tempo Popup #8524

sidharth-anand commented Jul 5, 2021 •

edited

Loading

Tantacrul commented Jul 25, 2021 •

edited

Loading

Tantacrul commented Jul 25, 2021 •

edited

Loading

Tantacrul commented Jul 25, 2021

Tantacrul commented Jul 25, 2021 •

edited

Loading

Tantacrul commented Jul 25, 2021

Tantacrul commented Jul 25, 2021 •

edited

Loading

Tantacrul commented Jul 25, 2021

Tantacrul commented Jul 25, 2021

sidharth-anand commented Aug 4, 2021

Jojo-Schmitz commented Aug 4, 2021

Tantacrul commented Aug 15, 2021

Tantacrul commented Aug 15, 2021 •

edited

Loading

Tantacrul commented Aug 15, 2021 •

edited

Loading

Jojo-Schmitz Aug 17, 2021 •

edited

Loading

Jojo-Schmitz Aug 17, 2021 •

edited

Loading

Jojo-Schmitz Aug 19, 2021

bkunda commented Sep 6, 2021 •

edited

Loading

bkunda commented Sep 6, 2021

Jojo-Schmitz commented Sep 6, 2021

bkunda commented Sep 6, 2021

shoogle Sep 6, 2021

Tantacrul commented Sep 6, 2021

bkunda commented Sep 6, 2021

bkunda commented Sep 6, 2021 •

edited

Loading

bkunda commented Sep 6, 2021

bkunda commented Sep 7, 2021

Jojo-Schmitz commented Sep 15, 2021

		@@ -1044,11 +1093,8 @@ TDuration OveNoteType_To_Duration(ovebase::NoteType noteType)
		// break;
		// }


		property alias navigation: navPanel
		property bool isDoActiveParentOnClose: true

Tempo Popup #8524

Are you sure you want to change the base?

Tempo Popup #8524

Conversation

sidharth-anand commented Jul 5, 2021 • edited Loading

Features

Tantacrul commented Jul 25, 2021 • edited Loading

Insertion point

Tantacrul commented Jul 25, 2021 • edited Loading

Placement of Popup

Tantacrul commented Jul 25, 2021

Spacing of popup

Tantacrul commented Jul 25, 2021 • edited Loading

Other visual issues with Popup

Tantacrul commented Jul 25, 2021

Wrong icons used for Whole Note and Double Whole note in metronome marking.

Tantacrul commented Jul 25, 2021 • edited Loading

Wrong visuals for selected tab state

Tantacrul commented Jul 25, 2021

Bug

Tantacrul commented Jul 25, 2021

sidharth-anand commented Aug 4, 2021

Jojo-Schmitz commented Aug 4, 2021

Tantacrul commented Aug 15, 2021

Tantacrul commented Aug 15, 2021 • edited Loading

Tantacrul commented Aug 15, 2021 • edited Loading

Jojo-Schmitz Aug 17, 2021 • edited Loading

Choose a reason for hiding this comment

Jojo-Schmitz Aug 17, 2021 • edited Loading

Choose a reason for hiding this comment

Jojo-Schmitz Aug 19, 2021

Choose a reason for hiding this comment

bkunda commented Sep 6, 2021 • edited Loading

bkunda commented Sep 6, 2021

Jojo-Schmitz commented Sep 6, 2021

bkunda commented Sep 6, 2021

shoogle Sep 6, 2021

Choose a reason for hiding this comment

Tantacrul commented Sep 6, 2021

bkunda commented Sep 6, 2021

bkunda commented Sep 6, 2021 • edited Loading

bkunda commented Sep 6, 2021

bkunda commented Sep 7, 2021

Jojo-Schmitz commented Sep 15, 2021

sidharth-anand commented Jul 5, 2021 •

edited

Loading

Tantacrul commented Jul 25, 2021 •

edited

Loading

Tantacrul commented Jul 25, 2021 •

edited

Loading

Tantacrul commented Jul 25, 2021 •

edited

Loading

Tantacrul commented Jul 25, 2021 •

edited

Loading

Tantacrul commented Aug 15, 2021 •

edited

Loading

Tantacrul commented Aug 15, 2021 •

edited

Loading

Jojo-Schmitz Aug 17, 2021 •

edited

Loading

Jojo-Schmitz Aug 17, 2021 •

edited

Loading

bkunda commented Sep 6, 2021 •

edited

Loading

bkunda commented Sep 6, 2021 •

edited

Loading