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

Select tuplet when calling next-element prev-element actions #23082

Merged

Conversation

efu98
Copy link
Contributor

@efu98 efu98 commented Jun 4, 2024

Resolves: #17600

Select tuplet when calling action next-element and prev-element

  • 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)

@efu98 efu98 closed this Jun 4, 2024
@efu98 efu98 reopened this Jun 4, 2024
@efu98 efu98 changed the title Select tuplet when calling next-element action DRAFT Select tuplet when calling next-element action Jun 4, 2024
@RomanPudashkin RomanPudashkin requested a review from Eism June 7, 2024 06:40
@efu98 efu98 marked this pull request as draft June 9, 2024 09:05
@efu98 efu98 changed the title DRAFT Select tuplet when calling next-element action Select tuplet when calling next-element action Jun 9, 2024
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.

Thanks for this! You've nearly solved the next-element function (but not quite), and you also need to do the same in the other direction with previous-element (Alt+Left).

From #17600 (comment):

If you go in the other direction with Alt+Left then the elements should be selected in reverse order.

src/engraving/dom/segment.cpp Outdated Show resolved Hide resolved
src/engraving/dom/tuplet.cpp Outdated Show resolved Hide resolved
@efu98 efu98 changed the title Select tuplet when calling next-element action Select tuplet when calling next-element prev-element actions Jun 16, 2024
@efu98 efu98 force-pushed the 17600-Tuplet-skipped-by-alt+left-and-alt+right branch from 2704d5b to 2a27a41 Compare June 19, 2024 22:25
@efu98 efu98 marked this pull request as ready for review June 20, 2024 05:52
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Jun 20, 2024
@Jojo-Schmitz
Copy link
Contributor

Jojo-Schmitz commented Jun 20, 2024

Going back does not select them in reverse order, both directions select the tuplet first.
May not be a great deal, at least it does get selected both ways and sort of it is the last and first element at the same time in a way.

@shoogle
Copy link
Contributor

shoogle commented Jun 20, 2024

@Jojo-Schmitz is right, when going backwards it's not in reverse order.

image

Here's what we want to see:

Forwards (Alt+Right) Backwards (Alt+Left)
1. D
2. Tuplet
3. E
4. F
5. G
6. A
6. A
5. G
4. F
3. E
2. Tuplet
1. D

@efu98 efu98 force-pushed the 17600-Tuplet-skipped-by-alt+left-and-alt+right branch from f91f1be to c684e78 Compare August 2, 2024 21:54
@efu98 efu98 requested a review from shoogle August 3, 2024 10:18
@efu98
Copy link
Contributor Author

efu98 commented Aug 3, 2024

Hii! Sorry for taking so much time making the changes, PR is ready to be reviewed now!

Copy link
Contributor Author

@efu98 efu98 left a comment

Choose a reason for hiding this comment

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

(Sorry, i was trying to remove the change request)

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.

Functionality is there, thanks! Just a couple of comments about the code.

src/engraving/dom/chord.cpp Outdated Show resolved Hide resolved
src/engraving/dom/tuplet.h Outdated Show resolved Hide resolved
@efu98
Copy link
Contributor Author

efu98 commented Aug 13, 2024

Hello! Thank you for the review ! I have edited the code according to the comments

@efu98 efu98 requested a review from shoogle August 13, 2024 21:49
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.

Thanks! Please fix the build and then squash all your commits into one.

src/engraving/dom/tuplet.cpp Show resolved Hide resolved
@efu98 efu98 force-pushed the 17600-Tuplet-skipped-by-alt+left-and-alt+right branch from 541ee6a to 3538d12 Compare August 20, 2024 20:45
@efu98 efu98 requested a review from shoogle August 20, 2024 21:18
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.

Great job, thanks!

Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Aug 28, 2024
@shoogle shoogle merged commit 77d4cc2 into musescore:master Aug 28, 2024
11 checks passed
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Aug 29, 2024
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Aug 29, 2024
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Aug 29, 2024
Jojo-Schmitz pushed a commit to Jojo-Schmitz/MuseScore that referenced this pull request Aug 29, 2024
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.

[Accessibility] Tuplets skipped by Alt+Left and Alt+Right
3 participants