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 #297426: The playback cursor is not repositioned when selecting items other than notes, rests or measures #5749

Merged

Conversation

Spire42
Copy link
Contributor

@Spire42 Spire42 commented Feb 23, 2020

Resolves: #297426

Improved the functionality of the playhead (a.k.a. the playback cursor) so that whenever the user selects an element, the playhead is automatically repositioned to the element's time position. Previously, this worked only for noteheads and rests, but it now works for note stems, beams, augmentation dots, accidentals, ties, slurs, articulations, time signatures, key signatures, clefs, tempo changes, dynamics, lines, barlines, breaks, spacers, measure numbers, text, and so on.

Special cases handled:

  • Barlines. The playhead is moved to the start of the measure to the right of the barline, unless it's the last barline in either the entire score or the system, in which case the playhead is moved to the start of the measure to the left of the barline.

  • Brackets always have a time position of zero, so the playhead is moved to the start of the first measure in the system that the bracket belongs to.

  • Instrument names always have a time position of zero, so the playhead is moved to the start of the first measure in the system that the instrument name belongs to.

  • I signed CLA
  • I made sure the code in the PR follows the coding rules
  • I made sure the code compiles on my machine
  • I made sure there are no unnecessary changes in the code
  • I made sure the title of the PR reflects the core meaning of the issue you are solving
  • I made sure the commit message(s) contain a description and answer the question "Why do those changes fix that particular issue?" or "Why are those changes really necessary as improvements?"
  • I made sure the commit message title starts with "fix #424242:" if there is a related issue
  • I created the test (mtest, vtest, script test) to verify the changes I made

Copy link
Contributor

@dmitrio95 dmitrio95 left a comment

Choose a reason for hiding this comment

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

Added some minor comments, but the overall solution seems really good to me.

libmscore/barline.cpp Outdated Show resolved Hide resolved
libmscore/bracket.cpp Show resolved Hide resolved
@Spire42 Spire42 force-pushed the 297426-move-playhead-to-selected-element branch 2 times, most recently from 6c6586d to d327ab2 Compare February 25, 2020 17:23
@igorkorsukov
Copy link
Contributor

igorkorsukov commented Mar 6, 2020

I believe that determining where to play is not the responsibility of these elements, it is external logic, application level logic rather than domain model (as opposed to getting a tick, for example)

@igorkorsukov
Copy link
Contributor

Having a separate helper class

This is not a class helper, this is a class service that fulfills its responsibility :)

@igorkorsukov
Copy link
Contributor

if you think the logic from where to play is the level of the domain model (and not the application level), then your solution is excellent

@Spire42
Copy link
Contributor Author

Spire42 commented Mar 6, 2020

I believe that determining where to play is not the responsibility of these elements, it is external logic, application level logic rather than domain model (as opposed to getting a tick, for example)

We fundamentally disagree on this. If you have a look at the code for the various tick() and rtick() implementations (which in some cases reach out way beyond the scope of the individual element in question), you might change your mind.

@igorkorsukov
Copy link
Contributor

igorkorsukov commented Mar 6, 2020

Thanks. I ask you to write comments in the base class that each tick method returns (what is tick, rtick and playTick, when and for what each method needs to be used)

@Spire42
Copy link
Contributor Author

Spire42 commented Mar 9, 2020

I ask you to write comments in the base class that each tick method returns (what is tick, rtick and playTick, when and for what each method needs to be used)

I completely agree with you that these three member functions of Element need documentation. However, I also see that of the existing 114 member functions in Element, only around 20 of them have any documentation at all, and what little documentation exists is mostly cryptic and not very helpful.

There has been some talk recently in the Developers Chat of an initiative to write comprehensive Doxygen-style documentation for the entire MuseScore codebase, possibly as a GSoC project. I think that would be a great idea.

In any case, I think writing documentation should be considered an issue on its own, separate from a small fix like this.

@igorkorsukov
Copy link
Contributor

only around 20 of them have any documentation at all, and what little documentation exists is mostly cryptic and not very helpful.

these three functions especially need documentation, because they are all similar, it is not obvious what their difference is.

I personally am not a adherent of documenting everything, I believe in self-documenting code (I try to write code like this). I believe that documentation needs something that is not obvious. Although at the same time I write a lot of comments myself :) (you can see it in my PR)
But perhaps in a large project with a lot of different people, we need to document everywhere.

@Spire42
Copy link
Contributor Author

Spire42 commented Mar 9, 2020

OK. I will document the one function I added, but I have to leave the other two alone for now, firstly because they aren't related to this PR, and secondly because I didn't write those functions and I'm not confident that I fully understand them well enough to explain precisely what they're meant to be used for — especially rtick().

@Spire42 Spire42 force-pushed the 297426-move-playhead-to-selected-element branch from d327ab2 to 2025506 Compare March 9, 2020 13:26
@Spire42
Copy link
Contributor Author

Spire42 commented Mar 9, 2020

Done.

…tems other than notes, rests or measures

Improved the functionality of the playhead (a.k.a. the playback cursor) so that whenever the user selects an element, the playhead is automatically repositioned to the element's time position. Previously, this worked only for noteheads and rests, but it now works for note stems, beams, augmentation dots, accidentals, ties, slurs, articulations, time signatures, key signatures, clefs, tempo changes, dynamics, lines, barlines, breaks, spacers, measure numbers, text, and so on.

Special cases handled:

* Barlines. The playhead is moved to the start of the measure to the right of the barline, unless it's the last barline in either the entire score or the system, in which case the playhead is moved to the start of the measure to the left of the barline.

* Brackets always have a time position of zero, so the playhead is moved to the start of the first measure in the system that the bracket belongs to.

* Instrument names always have a time position of zero, so the playhead is moved to the start of the first measure in the system that the instrument name belongs to.
@Spire42 Spire42 force-pushed the 297426-move-playhead-to-selected-element branch from 7132261 to f363220 Compare April 14, 2020 12:15
@anatoly-os anatoly-os merged commit 2a1b76e into musescore:master Apr 14, 2020
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.

None yet

4 participants