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

fixed saving courtesy for time signature change (issue #281896) #4595

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@lg2de
Copy link

lg2de commented Jan 16, 2019

I noticed that hiding the change announcement for the time signature in MS 3.0.0 and 3.0.1 is not saved (or loaded) correctly.
When analyzing the file content (XML), I noticed that there are two types of tags associated with the change announcement of signature changes, "showCourtesy" in the time sig change and "showCourtesySig" for key sig change. I wondered why they were different, so I decided to make my first attempt for code contribution.

I think I identified the root cause when writing the XML. Here "showCourtesy" is used while loading is checking for "showCourtesySig".
I'm not quite sure which is the desired tag name, I've noticed that the mentioned change of this PR solves the problem: "Hiding the change of time signature is saved and restored after loading".

@ericfont

This comment has been minimized.

Copy link
Contributor

ericfont commented Jan 16, 2019

Hi. Thanks for this PR. The normal procedure for fixing bugs is to write an issue report in the issue tracker: https://musescore.org/en/project/issues/musescore

The issue tracker will assign it a number. And then you use that number in the commit message of the PR with the format: "fix #issuenum problem being fixed". Take a look at any other issue in the list of pull requests to get an idea.

@lg2de

This comment has been minimized.

Copy link
Author

lg2de commented Jan 16, 2019

@ericfont Should I create an issue now?

@ericfont

This comment has been minimized.

Copy link
Contributor

ericfont commented Jan 16, 2019

I'm not the one in charge of accepting pull requests, so I can't say if you required to. Maybe the maintainers will accept this minor PR without an issue report. But I would suggest you do make an issue report. At least it will be an exercise in how to make an issue report, and it will let the rest of the community know that that this issue exists and has been fixed. And it will increase the chances that this PR is accepted. So you might as well go ahead and make an issue report, amend the commit with the fix #num in the commit message, and repush the commit.

@ericfont

This comment has been minimized.

Copy link
Contributor

ericfont commented Jan 16, 2019

(also first search the issue tracker for this problem...the issue might already be known and have a number!)

@dmitrio95

This comment has been minimized.

Copy link
Contributor

dmitrio95 commented Jan 16, 2019

A standard question: have you signed a CLA? If not, please sign it and tell us your nickname on musescore.org.

The relevant issue seems to have already been reported: https://musescore.org/en/node/281896. So it would be good to prepend fix #281896 to your commit message in order for issue tracker to automatically close the issue.

The patch itself looks good for me.

@lg2de lg2de changed the title fixed saving courtesy for time signature change fixed saving courtesy for time signature change (issue #281973) Jan 16, 2019

@lg2de lg2de changed the title fixed saving courtesy for time signature change (issue #281973) fixed saving courtesy for time signature change (issue #281896) Jan 16, 2019

@lg2de

This comment has been minimized.

Copy link
Author

lg2de commented Jan 16, 2019

@dmitrio95 Just now I searched for existing issue, but 281896 I did not found :(
So I created a new one... This one has been closed now and I added to the header 281896 instead.
Now I will try to update the commit message...

@ericfont

This comment has been minimized.

Copy link
Contributor

ericfont commented Jan 16, 2019

@lg2de you didn't quite modify the commit message correctly, as your PR now has two commits. you didn't need to do the merge. Just needed to ammend the commit message. Could you try again, this time only have one commit?

Sorry for having to follow all these procedures :)

@ericfont

This comment has been minimized.

Copy link
Contributor

ericfont commented Jan 16, 2019

So to correct this, you could do a few different things, such as:

  • Squash all commits together into one single commit with the correct message.
  • Or reset your head back to the original commit and simply do commit --amend to change the message
  • Or start a fresh new PR entirely with only one commit.
@ericfont

This comment has been minimized.

Copy link
Contributor

ericfont commented Jan 16, 2019

Well basically commit b91f8c4 is the correctly-formatted commit...I suppose the maintainer can just cherry-pick that one.

anatoly-os added a commit that referenced this pull request Jan 29, 2019

@anatoly-os

This comment has been minimized.

Copy link
Contributor

anatoly-os commented Jan 29, 2019

@lg2de thank you for the fix! Manually merged in 1f485c9.

@anatoly-os anatoly-os closed this Jan 29, 2019

@lg2de lg2de deleted the lg2de:FixSavingTimeSigCourtesy branch Jan 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment