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 #269672: Show selected notes on the Piano Keyboard #3488

Closed
wants to merge 1 commit into from
Closed

Fix #269672: Show selected notes on the Piano Keyboard #3488

wants to merge 1 commit into from

Conversation

yukisaw
Copy link

@yukisaw yukisaw commented Feb 22, 2018

@lasconic lasconic changed the title (issue 269672) To show selected notes on the "piano" control Fix #269672: Show selected notes on the Piano Keyboard Feb 22, 2018
@lasconic
Copy link
Contributor

Good start but

  1. It doesn't work in the case of a range selection (Shift + Click)
  2. It doesn't work in the case of multiple notehead selection (with Ctrl + Click)
  3. What does "chordpart" means exactly ? it needs a better name
  4. If I understand the use of -n->pitch() correctly, it's an ugly hack... it needs at least a comment, but it would be best to avoid the hack altogether.

@yukisaw
Copy link
Author

yukisaw commented Feb 22, 2018

  1. I don`t think this has to work on range selection.
  2. Same as 1st. But probably I can add this like several active and inactive pitches at one time. That's not hard, but need a lot of "if" conditions.
  3. Part of chord =) I will rename it when found a better name.
  4. Need to change logic to avoid this.

And all these things will take much more than 15 minutes I spent on this code. So don`t know when the request will be done.

@anatoly-os
Copy link
Contributor

@yukisaw so you did 80% of work for 20% of time. Since it is not must have functionality, it is better to spend more (free) time to finish it later, isn't it?

@yukisaw
Copy link
Author

yukisaw commented Feb 22, 2018

1 Skipped
2, 3 Resolved
4 Another hack.

Chord* chord = static_cast<Note*>(elements.front())->chord();
for (auto e : elements)
{
if (e->type() != Element::Type::NOTE || static_cast<Note*>(e)->chord() != chord)
Copy link
Contributor

Choose a reason for hiding this comment

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

in master we're using isNote() and toNote() instead, here and in other places of this PR

Copy link
Author

Choose a reason for hiding this comment

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

There is no isNote() and toNote() methods at MuseScore 2.2

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, didn't notice this was o(only) for 2.2

Copy link
Contributor

@anatoly-os anatoly-os left a comment

Choose a reason for hiding this comment

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

I'm sure now it is worse than it was. I mean bit shift.

Copy link
Contributor

@anatoly-os anatoly-os left a comment

Choose a reason for hiding this comment

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

Shifting bits is even worse than using 'minus'. What is going on?

@yukisaw
Copy link
Author

yukisaw commented Feb 22, 2018

Last one. That`s totally stupid. Was very easy solution on 22 lines.

@anatoly-os
Copy link
Contributor

In 3.0 piano code was fully reworked. So, it will be impossible to merge it to 3.0. And 3.0 has good methods like pressPitch() and releasePitch() without operations on ints. So, maybe implement it in 3.0?

@yukisaw
Copy link
Author

yukisaw commented Feb 22, 2018

You may make it by you own.

@anatoly-os
Copy link
Contributor

@yukisaw I assume the last comment was addressed to me. I meant, implement it ONLY in 3.0 and avoid such unclear code in 2.2. I still don't understand the reason why we're implementing it.

@mattmcclinch
Copy link
Contributor

I am sensing that there are still issues with this PR. So I would like to offer #3489 as an alternative.

@lasconic
Copy link
Contributor

#3489 looks better.

@lasconic lasconic closed this Feb 26, 2018
@yukisaw yukisaw deleted the pianotool branch February 26, 2018 14:20
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.

5 participants