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 #9873: MU does not taken file name from title when saving score #17690

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

HemantAntony
Copy link
Contributor

Resolves: #9873

  • I signed the CLA
  • The title of the PR describes the problem it addresses
  • Each commit's message describes its purpose and effects, and references the issue it resolves
  • If changes are extensive, there is a sequence of easily reviewable commits
  • The code in the PR follows the coding rules
  • There are no unnecessary changes
  • The code compiles and runs on my machine, preferably after each commit individually
  • I created a unit test or vtest to verify the changes I made (if applicable)

@DmitryArefiev
Copy link
Contributor

I tested #9873 (comment) briefly on Win/Mac and it looks fine, but it also should be reviewed.

@cbjeukendrup Can you help with CR please?

@HemantAntony HemantAntony force-pushed the 9873-file_name_not_taken_from_title branch from 5ee621d to 94bcd6d Compare July 22, 2023 11:52
@HemantAntony HemantAntony force-pushed the 9873-file_name_not_taken_from_title branch from 94bcd6d to b84dc56 Compare July 22, 2023 11:54
@HemantAntony
Copy link
Contributor Author

@cbjeukendrup Can you review this? :)

Comment on lines 223 to +224
fn = fn.replace('\n', '_');
fn = fn.simplified();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's fine to call simplified first, and to remove fn = fn.replace('\n', '_');. Otherwise, newlines will be converted into _ even if removeSpace is false, while it would be much more logical to convert newlines to spaces in that case.

@@ -285,6 +285,9 @@ io::path_t ProjectConfiguration::defaultSavingFilePath(INotationProjectPtr proje
}

filename = io::filename(projectPath, false);
if (filename == NotationProject::scoreDefaultTitle()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

To be honest, I'm not a great fan of comparing strings that may have been translated (we've seen this causing trouble a few times in the past). But in this case I don't see a much better solution, and it seems harmless enough.

But I wondered one thing: shouldn't we look at the title specified in score properties first, before we resort to looking in the notation itself?

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 Issue] MU doesn't take file name from a Title when saving a new score
4 participants