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

[MU4] fix 280260: added an option to set the default font for all text types #5816

Closed
wants to merge 1 commit into from

Conversation

SKefalidis
Copy link
Contributor

@SKefalidis SKefalidis commented Mar 14, 2020

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

Added the ability to change all the text fonts as requested. The option is at Settings > Style > Score below Musical Text Font.

Edit: Improved the UI, excluded RNA and added a way for the program to give some reasonable default values to the UI elements. Right now the change happens when you change the 2nd box, but I will probably add a button because this is not very intuitive.

image

  • 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

@Jojo-Schmitz
Copy link
Contributor

For some styles it is pretty vital that they don't change. Like Figured Bass, Roman Numeral Analysis and Nashville Number System for example. Hmm, maybe only for Roman Numeral Analysis...

@MarcSabatella
Copy link
Contributor

RNA for sure, but indeed maybe only that among the actual "text styles". The other elements that require special fonts don't use the text style mechanism - figured bass, tablature, multimeasure rests, time signatures, etc - and so wouldn't be affected by the change as proposed.

</property>
</widget>
</item>
<item row="2" column="1">
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a clever implementation, but I think the UI needs work. Right now it's just a combo box, meaning it isn't meaningful if - as may often be the case - there are a mix of fonts used. Also, after using the combo box to change the fonts, then saving the score and reloading, it will still show FreeSerif, will it not? Really, the control should probably be hidden behind an explicit Change button to make clear the combo box isn't showing you anything in particular relevant to the score. Also, it might be nice if the code to initialize the combo box somehow did present something relevant, like maybe the current value of staff text, or maybe title.

Copy link
Contributor Author

@SKefalidis SKefalidis Mar 14, 2020

Choose a reason for hiding this comment

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

@MarcSabatella

It's a clever implementation, but I think the UI needs work. Right now it's just a combo box, meaning it isn't meaningful if - as may often be the case - there are a mix of fonts used.

My original idea was to create something like this (see image). In the first box you select the font that you want to change (you can only select one of the fonts that are in use in the current style), in the second box you select the font that you want to replace the first font with. Then you click the 'Change' button. This way you can replace all occurrences of one font with another which is probably better than the original request.

Also, it might be nice if the code to initialize the combo box somehow did present something relevant, like maybe the current value of staff text, or maybe title.

I could initialize the combo box to the most used font in the current style.

changeText

@Spire42
Copy link
Contributor

Spire42 commented Mar 14, 2020

  • [x ] I made sure the code in the PR follows [the coding rules]

These checkboxes are all formatted incorrectly. Please remove the space after each x.

@SKefalidis SKefalidis force-pushed the 280260-new-feature branch 2 times, most recently from c956c51 to 17c30fd Compare April 17, 2020 22:52
@shoogle
Copy link
Contributor

shoogle commented Dec 16, 2020

I think this PR is good, but it tries to do too much. Judging by the screenshot, it's is basically a glorified find-and-replace feature to find instances of one font in text styles and replace it with another (i.e. find FreeSerif and replace with Ubuntu font). This is a clever way to solve the problem of styles like RNA having a different default to the others, but it is very unintuitive.

Instead, I would suggest having only one dropdown for a single default text font. If a new default is selected then any text styles that were using the old value are updated to use the new value, while styles using other values (e.g. RNA) are left unchanged.

In terms of file format, the default text font would be saved in the <Style> section of MSCX like any other style:

<Style>
  <defaultFontFace>Arial</defaultFontFace>
</Style>

Note: if the default font face was equal to "Edwin" (the new global default for 3.6) then it would not be written to the file.

The font face for individual text styles like Expression text would only be written to the file if it is different to the value in <defaultFontFace>:

<Style>
  <defaultFontFace>Arial</defaultFontFace>
  <expressionFontFace>Comic Sans</expressionFontFace>
</Style>

As an exception, text styles that have a special default (e.g. Campania for RNA) would have to be written to the file if they are changed to something other than that special default, even if the new value is the same as <defaultFontFace>:

<Style>
  <defaultFontFace>Arial</defaultFontFace>
  <romanNumeralFontFace>Arial</romanNumeralFontFace>
</Style>

@Jojo-Schmitz
Copy link
Contributor

Jojo-Schmitz commented Dec 16, 2020

Rebase needed, probably to 3.x (easily done directly here on GitHub).

@shoogle
Copy link
Contributor

shoogle commented Dec 20, 2020

While I still think my first suggestion is the way to go in the long term, a simpler method for 3.6 would be to not save <defaultFontFace> as an MSCX tag but instead just use the most common font from all the other text styles.

There would still only be a single dropdown for "Text font" in Format > Style. The initial value of the dropdown would be given by the most common font used across all text styles for the current score, so if most styles are using FreeSerif then the dropdown would initially show FreeSerif. If the dropdown is subsequently changed to Arial then all text styles that were previously using FreeSerif are changed to Arial. No need to alter code for reading or writing MSCX files.

If there is a tie for most common font then you can use the one that comes first in whatever data structure you are using to count them (probably a map). You could come up with tie-breaking criteria such as "which font is used in more styles that follow stave size?" but that's probably overkill for a situation that will be pretty rare anyway.

Note that even if there was a <defaultFontFace> tag in MSCX as outlined in my first proposal, you would still have had to implement this font "voting" procedure for pre-3.6 scores that lack the <defaultFontFace> tag.

@SKefalidis
Copy link
Contributor Author

SKefalidis commented Dec 20, 2020

While I still think my first suggestion is the way to go in the long term, a simpler method for 3.6 would be to not save <defaultFontFace> as an MSCX tag but instead just use the most common font from all the other text styles.

There would still only be a single dropdown for "Text font" in Format > Style. The initial value of the dropdown would be given by the most common font used across all text styles for the current score, so if most styles are using FreeSerif then the dropdown would initially show FreeSerif. If the dropdown is subsequently changed to Arial then all text styles that were previously using FreeSerif are changed to Arial. No need to alter code for reading or writing MSCX files.

If there is a tie for most common font then you can use the one that comes first in whatever data structure you are using to count them (probably a map). You could come up with tie-breaking criteria such as "which font is used in more styles that follow stave size?" but that's probably overkill for a situation that will be pretty rare anyway.

Note that even if there was a <defaultFontFace> tag in MSCX as outlined in my first proposal, you would still have had to implement this font "voting" procedure for pre-3.6 scores that lack the <defaultFontFace> tag.

That is indeed pretty simple (I am doing exactly that if I understand what you are saying correctly), but I am still not convinced that this behavior is preferable to the existing one. Checkout the 3.x PR, I added a button which I believe makes this pretty intuitive.

Edit: It would be great if we could get a couple more opinions on this matter.

@Tantacrul
Copy link
Contributor

Tantacrul commented Dec 23, 2020

I would suggest we do this instead. I feel all these fonts named 'text' is utterly confusing for any casual user.

Can I remind everyone that Vasily has already announced feature lockdown for 3.6, so this can't go in because it will most likely result in a few more unexpected bugs.

It is likely we will need to release a 3.6.1 (for a MuseScore.com fix) and we will consider this change then. But as a rule, from now on, all new features should be proposed and built for V4.

image

@igorkorsukov igorkorsukov changed the title fix 280260: added an option to set the default font for all text types [MU4] fix 280260: added an option to set the default font for all text types Feb 5, 2021
@SKefalidis
Copy link
Contributor Author

Closing.

@SKefalidis SKefalidis closed this Sep 5, 2021
@Jojo-Schmitz
Copy link
Contributor

Jojo-Schmitz commented Sep 6, 2021

Might still be something for inclusion in #9000
Edit: probably not this but #7127

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.

6 participants