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.

Sign up for GitHub

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

Jump to bottom

fix #301584: test for duplicates names in palette #5760

Merged
merged 2 commits into from
May 2, 2020
Merged

fix #301584: test for duplicates names in palette #5760

merged 2 commits into from
May 2, 2020

Conversation

krkartikay
Copy link
Contributor

@krkartikay krkartikay commented Feb 26, 2020 •
edited
Loading

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

This commit adds a test for checking if there are duplicate names in the palettes in the default workspaces and manually removes the duplicate items from Advanced.xml and Basic.xml by making the names more descriptive.

@Jojo-Schmitz
Copy link
Contributor

Please use a commit title starting with "fix #301584"

@Jojo-Schmitz
Copy link
Contributor

Well, tst_pallette failed (https://travis-ci.org/musescore/MuseScore/jobs/655368046#L4398), but without telling where and why

Harmoniker1
Harmoniker1 reviewed Feb 26, 2020
View reviewed changes
Copy link
Contributor

@Harmoniker1 Harmoniker1 left a comment

Choose a reason for hiding this comment

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

Below are formatting suggestions. Welcome!

mtest/mscore/palette/CMakeLists.txt Outdated Show resolved Hide resolved
mtest/mscore/palette/tst_palette.cpp Outdated Show resolved Hide resolved
mtest/mscore/palette/tst_palette.cpp Outdated Show resolved Hide resolved
mtest/mscore/palette/tst_palette.cpp Outdated Show resolved Hide resolved
mtest/mscore/palette/tst_palette.cpp Show resolved Hide resolved
mtest/mscore/palette/tst_palette.cpp Outdated Show resolved Hide resolved
mtest/mscore/palette/tst_palette.cpp Outdated Show resolved Hide resolved
mtest/mscore/palette/tst_palette.cpp Outdated Show resolved Hide resolved
mtest/mscore/palette/tst_palette.cpp Outdated Show resolved Hide resolved
mtest/mscore/palette/tst_palette.cpp Outdated Show resolved Hide resolved
shoogle
shoogle reviewed Feb 26, 2020
View reviewed changes
Copy link
Contributor

@shoogle shoogle left a comment

Choose a reason for hiding this comment

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

Looks very good overall! I can't tell why it catches some duplicates but not others, but it might have something to do with the conversion from QString to std::string, so I'll let you fix that before I take a look in more detail.

mtest/mscore/palette/tst_palette.cpp Outdated Show resolved Hide resolved
mtest/mscore/palette/tst_palette.cpp Show resolved Hide resolved
mtest/mscore/palette/tst_palette.cpp Outdated Show resolved Hide resolved
Jojo-Schmitz
Jojo-Schmitz reviewed Mar 2, 2020
View reviewed changes
mscore/menus.cpp Outdated Show resolved Hide resolved
@krkartikay krkartikay force-pushed the 301584-palette-duplicates branch 2 times, most recently from 5ffb5f5 to a004b3a Compare March 2, 2020 18:49
@krkartikay krkartikay changed the title [WIP] fix #301584: test for duplicates names in palette fix #301584: test for duplicates names in palette Mar 2, 2020
@krkartikay krkartikay force-pushed the 301584-palette-duplicates branch from a004b3a to 923b66d Compare March 2, 2020 19:22
shoogle
shoogle reviewed Mar 2, 2020
View reviewed changes
mtest/mscore/palette/tst_palette.cpp Outdated Show resolved Hide resolved
mtest/mscore/palette/tst_palette.cpp Outdated Show resolved Hide resolved
share/workspaces/Advanced.xml Outdated Show resolved Hide resolved
share/workspaces/Advanced.xml Outdated Show resolved Hide resolved
share/workspaces/Advanced.xml Outdated Show resolved Hide resolved
share/workspaces/Advanced.xml Outdated Show resolved Hide resolved
share/workspaces/Advanced.xml Outdated Show resolved Hide resolved
share/workspaces/Basic.xml Outdated Show resolved Hide resolved
@krkartikay krkartikay force-pushed the 301584-palette-duplicates branch 3 times, most recently from 4da405e to f333c53 Compare March 4, 2020 11:41
Jojo-Schmitz
Jojo-Schmitz reviewed Apr 21, 2020
View reviewed changes
libmscore/arpeggio.h Outdated Show resolved Hide resolved
Jojo-Schmitz
Jojo-Schmitz reviewed Apr 21, 2020
View reviewed changes
libmscore/glissando.h Outdated Show resolved Hide resolved
Jojo-Schmitz
Jojo-Schmitz reviewed Apr 21, 2020
View reviewed changes
mscore/menus.cpp Outdated Show resolved Hide resolved
Jojo-Schmitz
Jojo-Schmitz reviewed Apr 21, 2020
View reviewed changes
mscore/menus.cpp Outdated Show resolved Hide resolved
Jojo-Schmitz
Jojo-Schmitz reviewed Apr 21, 2020
View reviewed changes
mscore/menus.cpp Outdated Show resolved Hide resolved
@krkartikay krkartikay force-pushed the 301584-palette-duplicates branch from a34f681 to 2bdd3f5 Compare April 21, 2020 15:11
@krkartikay
fix #301584: test for duplicates names in palette
6286d64 
adds a test for checking if there are duplicate
names in the palettes in the default workspaces
and manually removed the duplicate items by making
the names more descriptive.
@krkartikay krkartikay force-pushed the 301584-palette-duplicates branch from 43b7f12 to ca092fd Compare April 21, 2020 16:44
Jojo-Schmitz
Jojo-Schmitz reviewed Apr 21, 2020
View reviewed changes
mscore/menus.cpp Outdated Show resolved Hide resolved
Jojo-Schmitz
Jojo-Schmitz reviewed Apr 21, 2020
View reviewed changes
mscore/menus.cpp Outdated Show resolved Hide resolved
@krkartikay krkartikay force-pushed the 301584-palette-duplicates branch 2 times, most recently from 1317064 to fd51679 Compare April 22, 2020 15:47
@anatoly-os
Copy link
Contributor

@Kartikay26 @shoogle Could you please make an overview of the changes in this PR after all reviews and remarks?

  1. What this PR is intended to fix / make better?
  2. What are the risks of current solutions?
  3. What will change for users after merging this PR?

@shoogle
Copy link
Contributor

shoogle commented Apr 28, 2020

@anatoly-os

  1. Some palette elements currently have the same name. This PR gives all elements a unique name and adds a test to ensure all names are unique in the future.

  2. By "current solution" do you mean what is done in this PR or what is already in master?

  3. The tooltips for palette elements will change as well as the accessible name spoken by screen readers.

For example, everything in the Tempo palette is currently called "Tempo Text".

image

This PR changes the tooltip to read "Adagio".

@Jojo-Schmitz
Copy link
Contributor

So useful (and actually necessary) for accessibility/screenreaders, right?

shoogle
shoogle requested changes Apr 28, 2020
View reviewed changes
mscore/menus.cpp Outdated Show resolved Hide resolved
share/workspaces/Basic.xml Outdated Show resolved Hide resolved
@krkartikay krkartikay force-pushed the 301584-palette-duplicates branch from fd51679 to f124760 Compare April 29, 2020 03:23
@krkartikay krkartikay force-pushed the 301584-palette-duplicates branch from f124760 to 0cca701 Compare April 29, 2020 09:03
@anatoly-os
Copy link
Contributor

@shoogle I meant the risks of the solution implemented in current PR.

@krkartikay
Copy link
Contributor Author

krkartikay commented May 1, 2020 •
edited
Loading

@shoogle I meant the risks of the solution implemented in current PR.

This PR contains a test and changes to the strings in the Palette text. I have tested it and it seems to be working correctly at least for English. However, as I am not familiar with the translation process so I don't know whether the translation will work correctly. That is the main risk, I guess, that the translation of some strings may become out of sync if I have messed up something in this PR.

But I have tried to do everything as @Jojo-Schmitz and @dmitrio95 said, so I hope it's mostly correct.

@anatoly-os anatoly-os merged commit d0a70e6 into musescore:master May 2, 2020
njvdberg added a commit to njvdberg/MuseScore that referenced this pull request May 3, 2020
@njvdberg
PR musescore#6019 removed "using namespace std" but PR musescore#5760 …
0513a5e 
…introduced tst_palette.cpp which

was created before the merge of PR musescore#6019 and used "vector" without the "std" prefix.
@njvdberg njvdberg mentioned this pull request May 3, 2020
Merged
6 tasks
njvdberg added a commit to njvdberg/MuseScore that referenced this pull request May 3, 2020
@njvdberg
PR musescore#6019 removed "using namespace std" but PR musescore#5760 …
53dbea0 
…introduced tst_palette.cpp which

was created before the merge of PR musescore#6019 and used "vector" without the "std" prefix.
@krkartikay krkartikay deleted the 301584-palette-duplicates branch May 10, 2020 07:18
@Jojo-Schmitz Jojo-Schmitz mentioned this pull request May 19, 2020
Merged
Jojo-Schmitz
Jojo-Schmitz reviewed May 19, 2020
View reviewed changes
libmscore/barline.cpp
{ BarLineType::END_REPEAT, QT_TRANSLATE_NOOP("Palette", "End repeat barline"), "end-repeat" },
{ BarLineType::BROKEN, QT_TRANSLATE_NOOP("Palette", "Dashed barline"), "dashed" },
{ BarLineType::END, QT_TRANSLATE_NOOP("Palette", "Final barline"), "end" },
{ BarLineType::END_START_REPEAT, QT_TRANSLATE_NOOP("Palette", "End-start repeat barline"), "end-start-repeat" },
Copy link
Contributor

@Jojo-Schmitz Jojo-Schmitz May 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

These changes now actually cause https://musescore.org/en/node/305676 and #6096 (comment)
Not doing it here though would change the issue into the opposite: instead of the repeat barline tooltips in the Repeats & Jumps palette not being translated, it that would be those in the Barlines palette

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

@Jojo-Schmitz Jojo-Schmitz Jojo-Schmitz left review comments

@dmitrio95 dmitrio95 dmitrio95 left review comments

@Harmoniker1 Harmoniker1 Harmoniker1 left review comments

@shoogle shoogle shoogle requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@krkartikay @Jojo-Schmitz @dmitrio95 @anatoly-os @shoogle @Harmoniker1