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

Add adjusted bar numbers to the status bar, if they differ #5071

Closed
wants to merge 1 commit into from

Conversation

joshwd36
Copy link
Contributor

This implements some of the suggestions in https://musescore.org/en/node/288968. I haven't yet changed the find function to implement searching for these adjusted bar numbers, as I think there would have to be a significant rewrite of the parsing of that, in order to allow the user to specify bar numbers in a specific section, and I think that should have its own pull request. What this does do, though, is change the status bar text to include the adjusted bar number, if it differs from the absolute bar number. It does this in the format adjusted bar number (absolute bar number).

Copy link
Contributor

@jthistle jthistle left a comment

Choose a reason for hiding this comment

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

Great job! This all looks like it should do the trick. Just a few points, mainly code style.

libmscore/measure.cpp Outdated Show resolved Hide resolved
mscore/scoreaccessibility.cpp Outdated Show resolved Hide resolved
mscore/scoreaccessibility.cpp Outdated Show resolved Hide resolved
mscore/scoreaccessibility.cpp Outdated Show resolved Hide resolved
mscore/scoreaccessibility.h Outdated Show resolved Hide resolved
@Jojo-Schmitz
Copy link
Contributor

a) for my taste there a a couple too many curly braces, not really needed for single statement if or else
b) we may be able to make the translators' work easier by constructing the strings in a better way, only translating "Measure:" and "Beat:" and only once (or at least fewer times as needed with the current PR) rather than including those potentially confusing "%1 (%2)".

@jthistle
Copy link
Contributor

  1. Eh, I'm not bothered about more curly braces than technically necessary. It hardly detracts from code readability.

  2. Good point. That would involve translating just "Measure" and "Beat" but also "Start measure" and "Start beat" as separate strings?

@Jojo-Schmitz
Copy link
Contributor

Jojo-Schmitz commented May 27, 2019

  1. Agreed, this is just a minor and personal taste thing
  2. Yes, I was just giving an example

@IsaacWeiss
Copy link
Contributor

MuseScore coding style: "Use curly braces when the body of a conditional statement contains more than one line, and also if a single line statement is somewhat complex. Otherwise, omit them."
https://musescore.org/en/handbook/developers-handbook/finding-your-way-around/musescore-coding-rules#Braces

@Jojo-Schmitz
Copy link
Contributor

and IMHO none of them here quaify as "somewhat complex"

@jthistle
Copy link
Contributor

Well, sure, but all I'm saying is that if I were merging PRs, I wouldn't prevent a PR from being merged because the braces weren't perfect.

@jthistle
Copy link
Contributor

Thanks for changing it Josh! Just one last thing, if you could squash the two commits into a single commit and change the commit message to include the issue number, that'd be great. For example:

fix #288968: add adjusted bar numbers to the status bar, if they differ

@Jojo-Schmitz
Copy link
Contributor

and work on the 'simplify translations' issue I mentioned?

@Jojo-Schmitz
Copy link
Contributor

Jojo-Schmitz commented Jan 26, 2020

rebase needed (also needs the Strings label)

Copy link
Contributor

@MarcSabatella MarcSabatella left a comment

Choose a reason for hiding this comment

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

We would also need to discuss how to best present this in the screen reader info.

@dmitrio95 dmitrio95 added the strings Affects translatable strings label Feb 10, 2020
@vpereverzev
Copy link
Member

Looking for rebase (probably on 3.x)

@vpereverzev
Copy link
Member

Archived due to inactivity (we can return it to active PRs when the author returns)

@vpereverzev vpereverzev added the archived PRs that have gone stale but could potentially be revived in the future label Nov 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
archived PRs that have gone stale but could potentially be revived in the future strings Affects translatable strings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants