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 typo, follow up to #23473 #24036

Merged
merged 2 commits into from
Oct 9, 2024
Merged

Conversation

Jojo-Schmitz
Copy link
Contributor

@Jojo-Schmitz Jojo-Schmitz commented Aug 14, 2024

We changed the style tag "Spatium" into "spatium" in #23473, but still write "Spatium" hard coded into the style files, this PR fixes that. On reading we already read both, so no extra action needed.

@Jojo-Schmitz
Copy link
Contributor Author

Jojo-Schmitz commented Aug 14, 2024

Unfortunately now 545 occurences in unit test files would need to get amended, any clever idea how to not have to do it manually?

Sledghammer approach:
find . -name '*-ref.mscx' -exec sed -i s/Spatium/spatium/ {} \;
changed 363 of them

@cbjeukendrup
Copy link
Contributor

Maybe something with a regular expression, to replace <Spatium>([0-9]+[.]?[0-9]*)</Spatium> with <spatium>$1</spatium>. I think you can do that in Visual Studio Code. There you can also specify that it should only look in *.mscz,*.mscx files.

@Jojo-Schmitz
Copy link
Contributor Author

Jojo-Schmitz commented Aug 14, 2024

Hmm, let's see whether find . -name '*ref.mscx' -exec sed -i 's/Spatium>/spatium>/g' {} \; is any better...
(changed 434 files)

@Jojo-Schmitz Jojo-Schmitz force-pushed the spatium branch 2 times, most recently from 446cf53 to 51ffd39 Compare August 14, 2024 16:23
@cbjeukendrup cbjeukendrup requested a review from mike-spa August 14, 2024 16:32
@Jojo-Schmitz
Copy link
Contributor Author

Jojo-Schmitz commented Aug 14, 2024

Better: for f in $(find . -name '*.mscx' | xargs grep -l 'museScore version="4.40"'); do sed -i 's/Spatium>/spatium>/g' $f; done, i.e. replacing "Spatium>" with "spatium>" in all 4.4 mscx files, 546 files, 674 occurences, yes, more than immediatelly needed, but should be on the futur-proof side.

@Jojo-Schmitz
Copy link
Contributor Author

Jojo-Schmitz commented Aug 21, 2024

Any news about this? Should it go into 4.4?
Or into 4.5, and if so along with bumping the mscVersion?

I suspect a lot of bit rot on this one the longer it sits unmerged, i.e. more failing unit tests...
(Let's see what a rebase shows. Edit: indeed one more score to fix)

@RomanPudashkin
Copy link
Contributor

No, it shouldn't go into 4.4. We'll have to wait for @mike-spa to review it first

Copy link
Contributor

@mike-spa mike-spa left a comment

Choose a reason for hiding this comment

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

Looks ok to me.

@Jojo-Schmitz Jojo-Schmitz force-pushed the spatium branch 2 times, most recently from deb14c1 to 7c45ddb Compare September 5, 2024 10:09
@Jojo-Schmitz
Copy link
Contributor Author

Rebased (to fix merge conflict) and refreshed (to include scores added since)
Please consider a timely merge...

by running `for f in $(find . -name '*.mscx'  | xargs grep -l 'museScore version="4.40"'); do sed -i 's/Spatium>/spatium>/g' $f; done`, i.e. replacing "Spatium>" with "spatium>" in all 4.4 mscx files
@cbjeukendrup cbjeukendrup merged commit ba0d4d2 into musescore:master Oct 9, 2024
11 checks passed
@Jojo-Schmitz Jojo-Schmitz deleted the spatium branch October 9, 2024 15:47
@rpatters1
Copy link
Contributor

rpatters1 commented Oct 10, 2024

How far back is MuseScore accepting <spatium> (not capitalized) on input? (Roughly)

I only ask because I've written a .mss exporter for Finale, and I will change it to emit <spatium> if it is recognized a reasonable number of versions back.

@Jojo-Schmitz
Copy link
Contributor Author

Jojo-Schmitz commented Oct 10, 2024

Since 4.4.0, some 3 months ago
But <Spatium> is still accepted, so no need to change your exporter, it is now just not written anymore

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.

5 participants