Fix #16519: L/R Arrow During Playback Moves From Playhead Current Location #19038

22justinl · 2023-08-12T20:46:15Z

Left/Right Arrow: Move between beats
Left/Right Arrow + Ctrl/Command: Move between measures

Resolves: #16519
Resolves: #23795

Changed so that left and right arrows during playback move the playback cursor instead of the selection (which causes the playback cursor to move back to the selection)

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)

src/notation/internal/notationactioncontroller.cpp

RomanPudashkin · 2023-12-12T13:28:48Z

@22justinl please rebase this PR. Thanks!

zacjansheski · 2024-01-09T14:03:11Z

I think this change is problematic with repeats

Here is a video from the PR

video1399164880.mp4

zacjansheski · 2024-01-09T14:03:39Z

Here is a video from current 4.2

video1231836572.mp4

cbjeukendrup · 2024-03-31T15:51:17Z

@22justinl Kind reminder about the comments above :)
If you don't have time anymore to fix them, that's alright; in that case we'll close this PR, so that a different contributor can continue your work.
Please let us know!

src/notation/internal/notationactioncontroller.cpp

src/playback/internal/playbackcontroller.cpp

cbjeukendrup

Thanks very much! Besides fixing the issue, this PR helped uncover quite some problems with PlaybackController!

zacjansheski · 2024-08-06T22:23:40Z

Playhead gets stuck on time signature change

I don't think this is a "show-stopper" but in master, pressing left will take the playhead past the time signature change and in the PR you need to use Cmd+Left

Sorry this video is longer than necessary

video1423174087.mp4

…from playback cursor location - left/right arrow: move between beats - left/right arrow + ctrl/command: move between measures

…troller

22justinl · 2024-08-14T15:35:52Z

src/notation/internal/notationactioncontroller.cpp

+                    // Set target beat to max beat of previous bar
+                    engraving::TimeSigMap* timeSigMap = currentMasterNotation()->masterScore()->sigmap();
+                    int targetBarStartTick = timeSigMap->bar2tick(targetMeasureIdx, 0);
+                    targetBeatIdx = timeSigMap->timesig(Fraction::fromTicks(targetBarStartTick)).timesig().numerator() - 1;


These are the changes I made + #include "engraving/dom/sig.h" at the top (I thought it might be unclear because I rebased the branch)

@RomanPudashkin do you think we should try to extract these lines into a method that can be reused here and in NotationPlayback::beat? Or is that not worth the effort?
(The question would also be, where to put such a method.)

Cosmetic improvements can be made later

zacjansheski · 2024-08-14T18:56:45Z

Tested on MacOS 14, Windows 11, Ubuntu 22.04.3. Approved
#16519 FIXED
#23795 FIXED

cbjeukendrup

Thanks for the fix! I have one last stylistic doubt, that I'll ask Roman about:

cbjeukendrup · 2024-08-14T19:26:30Z

src/notation/internal/notationactioncontroller.cpp

+                    // Set target beat to max beat of previous bar
+                    engraving::TimeSigMap* timeSigMap = currentMasterNotation()->masterScore()->sigmap();
+                    int targetBarStartTick = timeSigMap->bar2tick(targetMeasureIdx, 0);
+                    targetBeatIdx = timeSigMap->timesig(Fraction::fromTicks(targetBarStartTick)).timesig().numerator() - 1;


@RomanPudashkin do you think we should try to extract these lines into a method that can be reused here and in NotationPlayback::beat? Or is that not worth the effort?
(The question would also be, where to put such a method.)

…r_lr Fix musescore#16519: L/R Arrow During Playback Moves From Playhead Current Location

22justinl force-pushed the move_playback_cursor_lr branch from 5caad1b to dd35901 Compare August 12, 2023 20:50

cbjeukendrup self-requested a review August 13, 2023 21:47

cbjeukendrup reviewed Aug 24, 2023

View reviewed changes

src/notation/internal/notationactioncontroller.cpp Outdated Show resolved Hide resolved

src/notation/internal/notationactioncontroller.cpp Outdated Show resolved Hide resolved

22justinl force-pushed the move_playback_cursor_lr branch from dd35901 to cc61b0a Compare August 24, 2023 08:19

cbjeukendrup requested a review from RomanPudashkin August 24, 2023 12:47

RomanPudashkin approved these changes Dec 12, 2023

View reviewed changes

RomanPudashkin assigned zacjansheski Dec 12, 2023

22justinl force-pushed the move_playback_cursor_lr branch from cc61b0a to 2a12b95 Compare December 16, 2023 23:43

igorkorsukov force-pushed the master branch 6 times, most recently from fa1f8d3 to 525a11a Compare February 14, 2024 09:08

22justinl force-pushed the move_playback_cursor_lr branch 3 times, most recently from c75a8b5 to 591151f Compare July 8, 2024 10:48

22justinl marked this pull request as draft July 8, 2024 11:04

22justinl force-pushed the move_playback_cursor_lr branch 2 times, most recently from d1bb114 to 2fadc4a Compare July 8, 2024 12:52

22justinl marked this pull request as ready for review July 8, 2024 13:41

zacjansheski requested review from RomanPudashkin and cbjeukendrup July 8, 2024 18:32

cbjeukendrup reviewed Jul 19, 2024

View reviewed changes

src/notation/internal/notationactioncontroller.cpp Outdated Show resolved Hide resolved

22justinl force-pushed the move_playback_cursor_lr branch 2 times, most recently from d1f5e88 to ba0a45e Compare July 20, 2024 19:31

22justinl force-pushed the move_playback_cursor_lr branch from ba0a45e to f3733e1 Compare July 20, 2024 19:56

cbjeukendrup reviewed Jul 25, 2024

View reviewed changes

src/notation/internal/notationactioncontroller.cpp Outdated Show resolved Hide resolved

cbjeukendrup reviewed Jul 25, 2024

View reviewed changes

src/playback/internal/playbackcontroller.cpp Outdated Show resolved Hide resolved

cbjeukendrup reviewed Jul 25, 2024

View reviewed changes

src/playback/internal/playbackcontroller.cpp Outdated Show resolved Hide resolved

22justinl force-pushed the move_playback_cursor_lr branch from e9060ee to 5fd330f Compare July 27, 2024 09:00

This was referenced Jul 27, 2024

When entering a time code in the playback toolbar, it jumps to a different time than what you enter #23793

Closed

Consistently use seconds instead of milliseconds throughout playback module #23794

Merged

22justinl force-pushed the move_playback_cursor_lr branch from 5fd330f to 4ecde07 Compare July 27, 2024 17:48

22justinl force-pushed the move_playback_cursor_lr branch 2 times, most recently from 17b2211 to 37cb03f Compare August 5, 2024 02:03

cbjeukendrup approved these changes Aug 5, 2024

View reviewed changes

RomanPudashkin approved these changes Aug 5, 2024

View reviewed changes

22justinl force-pushed the move_playback_cursor_lr branch from 37cb03f to 1de3ae8 Compare August 14, 2024 15:23

22justinl added 3 commits August 14, 2024 23:24

left/right arrow with and without ctrl/command during playback moves …

7e3cbfd

…from playback cursor location - left/right arrow: move between beats - left/right arrow + ctrl/command: move between measures

renamed tick conversion functions in NotationPlayback and PlaybackCon…

f425514

…troller

Changed seek(tick) function name for clarity + resolves musescore#23795

a1ca193

22justinl force-pushed the move_playback_cursor_lr branch from 1de3ae8 to a1ca193 Compare August 14, 2024 15:24

22justinl commented Aug 14, 2024

View reviewed changes

zacjansheski requested review from cbjeukendrup and RomanPudashkin August 14, 2024 18:56

cbjeukendrup approved these changes Aug 14, 2024

View reviewed changes

RomanPudashkin merged commit 08e50e2 into musescore:master Aug 15, 2024
11 checks passed

RomanPudashkin added a commit to RomanPudashkin/MuseScore that referenced this pull request Aug 20, 2024

Merge pull request musescore#19038 from 22justinl/move_playback_curso…

729b1f6

…r_lr Fix musescore#16519: L/R Arrow During Playback Moves From Playhead Current Location

RomanPudashkin mentioned this pull request Aug 20, 2024

440_port_fixes #24115

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix #16519: L/R Arrow During Playback Moves From Playhead Current Location #19038

Fix #16519: L/R Arrow During Playback Moves From Playhead Current Location #19038

22justinl commented Aug 12, 2023 •

edited by RomanPudashkin

Loading

RomanPudashkin commented Dec 12, 2023

zacjansheski commented Jan 9, 2024

zacjansheski commented Jan 9, 2024

cbjeukendrup commented Mar 31, 2024 •

edited

Loading

cbjeukendrup left a comment

zacjansheski commented Aug 6, 2024 •

edited

Loading

22justinl Aug 14, 2024

cbjeukendrup Aug 14, 2024

RomanPudashkin Aug 15, 2024

zacjansheski commented Aug 14, 2024

cbjeukendrup left a comment

cbjeukendrup Aug 14, 2024

Fix #16519: L/R Arrow During Playback Moves From Playhead Current Location #19038

Fix #16519: L/R Arrow During Playback Moves From Playhead Current Location #19038

Conversation

22justinl commented Aug 12, 2023 • edited by RomanPudashkin Loading

RomanPudashkin commented Dec 12, 2023

zacjansheski commented Jan 9, 2024

zacjansheski commented Jan 9, 2024

cbjeukendrup commented Mar 31, 2024 • edited Loading

cbjeukendrup left a comment

Choose a reason for hiding this comment

zacjansheski commented Aug 6, 2024 • edited Loading

22justinl Aug 14, 2024

Choose a reason for hiding this comment

cbjeukendrup Aug 14, 2024

Choose a reason for hiding this comment

RomanPudashkin Aug 15, 2024

Choose a reason for hiding this comment

zacjansheski commented Aug 14, 2024

cbjeukendrup left a comment

Choose a reason for hiding this comment

cbjeukendrup Aug 14, 2024

Choose a reason for hiding this comment

22justinl commented Aug 12, 2023 •

edited by RomanPudashkin

Loading

cbjeukendrup commented Mar 31, 2024 •

edited

Loading

zacjansheski commented Aug 6, 2024 •

edited

Loading