Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #10307: Rearranged Palettes #10363

Merged
merged 1 commit into from
Feb 10, 2022

Conversation

HemantAntony
Copy link
Contributor

Resolves: #10307

  • 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

@HemantAntony HemantAntony marked this pull request as draft January 24, 2022 06:41
@HemantAntony HemantAntony force-pushed the 10307-changepalettes branch 2 times, most recently from 900fffe to 505b75a Compare January 24, 2022 06:44
@HemantAntony
Copy link
Contributor Author

@bkunda, I could not add the icon for Staff Type Change. Also, in the issue, you had asked me to change description of a cell to "Chant Caesura". I changed it to "Chant caesura" to make it consistent with other descriptions. If you want to me to change it to the former, please tell me.

@bkunda
Copy link

bkunda commented Jan 24, 2022

@bkunda, I could not add the icon for Staff Type Change. Also, in the issue, you had asked me to change description of a cell to "Chant Caesura". I changed it to "Chant caesura" to make it consistent with other descriptions. If you want to me to change it to the former, please tell me.

Sentence case is correct so you're spot on about this! 🙌🏻

I'll follow up separately with our dev team re: that extra icon. There are a few other questions I have about that palette so we will resolve it at some point soon I'm sure.

Many thanks again! 🙏🏻

@HemantAntony
Copy link
Contributor Author

@bkunda, can you please check Bagpipe Embellishments palette. Its a bit too long in my opinion

@HemantAntony HemantAntony marked this pull request as ready for review January 27, 2022 17:01
@HemantAntony HemantAntony marked this pull request as draft January 27, 2022 17:02
@HemantAntony HemantAntony force-pushed the 10307-changepalettes branch 7 times, most recently from a67bfd6 to 40f3145 Compare January 29, 2022 09:04
@HemantAntony HemantAntony marked this pull request as ready for review January 29, 2022 09:05
@bkunda
Copy link

bkunda commented Jan 31, 2022

From a UX perspective, this is working well. Great work @HemantAntony.
I have a list of small tweaks I'd like to make (some are corrections, others can now be specified since the bug about the & has been fixed up, others still are micro adjustments I have observed after a second viewing of this whole issue). I'll list these below....

Meanwhile @RomanPudashkin, could you or one of your colleagues please just have a look at this code and give @HemantAntony any feedback before we finalise everything?


Final adjustments (For now... 😉 ):

Tempo

  • "Swing" seems to have disappeared. It should appear alongside "Straight".

Fingering

  • “Fingering 0” and “LH guitar fingering 0” should be in main palette (not in “More”)

Arpeggios & glissandi:

  • “Straight glissando” and “Wavy glissando” are duplicated. Remove them from “more” popup.
  • Now that the “Palette properties” are working, there is an issue with the alignment of specifically these two elements:

Arpeggios   glissandi

They appear too high up in the upper right corner of their respective palette tile. It would be great if they could be centred somehow. If this could be achieved, I’d like to see the palette properties for this as follows:
Width=42
Height=44
Elements offset=0sp
Scale=1
Show grid=yes

Grace notes

  • “Scale” doesn’t seem to be working (a separate bug might be needed for this)
  • Change properties to: Width=45, Height=40, Element offset=0sp, Scale=1, Show grid=yes

Breaths and pauses

  • Change width to 40 (Width=40, Height=40, Element offset=0, Scale=1, Show grid=yes)

Repeats and jumps

  • Change properties to: Width=75, Height=28, Element offset=0, Scale=0.85, Show grid=yes

Dynamics

  • Change scale to 1

Hope this makes sense. @HemantAntony let me know if you have any questions, and thank you so much for your work on this! 🙏🏻

@bkunda
Copy link

bkunda commented Feb 9, 2022

@HemantAntony this is almost ready from my end!
Can we please just have the default properties for "arpeggios & glissandi" set to:

Width=42
Height=44
Elements offset=0sp
Scale=1
Show grid=yes

Many thanks,
Bradley

@HemantAntony
Copy link
Contributor Author

I updated the PR. Also @RomanPudashkin, was I supposed to have created this PR with different commits instead of a single one

@bkunda
Copy link

bkunda commented Feb 9, 2022

@Eism @RomanPudashkin @igorkorsukov this looks good to me from a UX perspective. If one of you could please have a look at it, that would be great.

I've only tested on MacOS (the scaling issue in Windows means I can't accurately see the scaling of palette objects, I can't test it yet in this environment).

Many thanks @HemantAntony!

@RomanPudashkin
Copy link
Contributor

Overall looks very good! I left several small comments for you. This PR can be merged after resolving them. Thank you!

@HemantAntony
Copy link
Contributor Author

Please don't merge yet. I'm looking at another related issue

@HemantAntony HemantAntony marked this pull request as draft February 10, 2022 12:58
@HemantAntony HemantAntony marked this pull request as ready for review February 10, 2022 13:17
@RomanPudashkin RomanPudashkin merged commit 37f1382 into musescore:master Feb 10, 2022
@HemantAntony HemantAntony deleted the 10307-changepalettes branch February 10, 2022 16:01
@@ -3805,7 +3805,7 @@ constexpr const std::array<const char*, size_t(SymId::lastSym) + 1> SymNames::s_
"Punctum auctum, ascending",
"Punctum auctum, descending",
"Augmentum (mora)",
QT_TRANSLATE_NOOP("symUserNames", "Caesura"),
QT_TRANSLATE_NOOP("symUserNames", "Chant caesura"),
Copy link
Contributor

@Jojo-Schmitz Jojo-Schmitz Feb 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not the name used in SMuFL!
Yes, the glyph is chantCaesura, but the clear text name is "Caesura" (unfortunatly, complain to SMuFL).
See fonts/smufl/glyphnames.json:

	"chantCaesura": {
		"codepoint": "U+E8F8",
		"description": "Caesura"
	},

Whenever ...tools/fonttools/smufl2sym.{sh,bat} is run again, it'd revert this change.

See #6152, when this got added to 3.x and 7d70696 for master, esp. the commented out code in the mtests checking for duplicates, as in 3.x this was only in the master palette.

See #10531

Jojo-Schmitz added a commit to Jojo-Schmitz/MuseScore that referenced this pull request Feb 11, 2022
see musescore#10363 and the discussion there
Jojo-Schmitz added a commit to Jojo-Schmitz/MuseScore that referenced this pull request Feb 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[MU4 Task] Tidy up default palette visibility settings
4 participants