#17012: Add option to disable "Play notes when editing" if input comes from MIDI keyboard #24352

JohnodonCode · 2024-09-01T03:09:05Z

Resolves: #17012

This introduces a toggle in the Note input configuration to disable playing notes with MIDI input. This will allow users to keep note sounds playing when manually adding a note, but disable the sound if the note is added via MIDI input.

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)

RomanPudashkin · 2024-09-03T07:30:59Z

Looks good, however, someone should check this out from the design team. So I'm assigning @bkunda (he is on vacation, so it will take some time) and @avvvvve

avvvvve · 2024-09-04T00:41:40Z

I think we shouldn't nest this under "Play notes when editing" because that checkbox is the parent of the others below it: if you turn it off, they all turn off. You might not want to hear notes while editing but continue to hear them while entering MIDI. Also, you can hear MIDI input without actually entering notes if you have something in a staff selected, so it doesn't really fit under the category "...when editing".

I'd still like @bkunda's to sign off on when he's back in next week, but here's what I would suggest:

Give the section the title "Note preview"
Un-bold "Play notes when editing"
Indent the two chord options (left-aligned with the text next to the checkbox above) so that it's obvious that they're sub-settings
Next, leave the MIDI setting un-indented and title it "Play MIDI input"
Move "Default duration" to the bottom

So now, if you uncheck "Play notes when editing", only the indented settings become disabled. "Default duration" stays enabled since the MIDI setting is still on.

Turning off only the MIDI checkbox affects nothing else:

Turning off both disables "Default duration":

Vertical spacing notes:

From the divider line to "Note preview" is 24px
From "Note preview" to the first checkbox is 20px
Each checkbox and "Default duration" is 12 px apart

JohnodonCode · 2024-09-04T02:42:56Z

@avvvvve ,
The reason that I placed it under the while editing toggle is because, with the current behavior, MIDI input will not play if "Play notes while editing" is disabled, even if you are not in note input mode. While we can change this behavior, I was originally basing the change off the original feature request.

If I do implement this new feature, should I update this PR or create a new one, and should I wait for @bkunda to sign off on this change?

wizofaus · 2024-09-04T08:30:08Z

It seems like the two orthogonal options are
"Play notes while editing (except via MIDI)"
"Play notes entered via MIDI"

The latter applies even when not "editing" as such.

avvvvve · 2024-09-04T13:01:44Z

Right @wizofaus - the latter "Play notes entered via MIDI" accounts for when you're not in note input mode and just want to hear playback from your MIDI device without actually editing anything in the score.

My thought was that if you have "Play notes when editing" OFF and "Play MIDI input" ON,

MIDI input entered in note input mode would play back
Selecting a note or moving a note up or down with arrow keys would not play it back (play notes when editing off)
Outside of note input mode, selecting something on a staff and then playing some notes on the MIDI device WOULD play back using that staff's sound, but would not affect anything in the score.

I know this is a step beyond the original feature request so thanks for bearing with the changes @JohnodonCode! No need to make a new PR, you can update this one.

wizofaus · 2024-09-04T19:29:57Z

But what do you expect to happen if "play notes while editing" is ON and "play midi input" if OFf and you use the midi keyboard in note input mode? Based on the initial request I'm assuming MuseScore shouldn't play the notes. I'm not sure if that's what's been implemented here...

avvvvve · 2024-09-04T19:51:00Z

But what do you expect to happen if "play notes while editing" is ON and "play midi input" if OFf and you use the midi keyboard in note input mode? Based on the initial request I'm assuming MuseScore shouldn't play the notes. I'm not sure if that's what's been implemented here...

Yes, that was the request and that's what was implemented. The design I showed above would not change the behavior implemented for this scenario. It would just allow the opposite too (hear MIDI input, but don't hear anything when clicking on notes or changing their pitch).

wizofaus · 2024-09-04T19:52:57Z

But you would hear something if MIDI output was configured and connected to a device that played back the sounds? Get Outlook for Android<https://aka.ms/AAb9ysg>

…

________________________________ From: Ben Averill ***@***.***> Sent: Thursday, September 5, 2024 5:51:21 AM To: musescore/MuseScore ***@***.***> Cc: wizofaus ***@***.***>; Mention ***@***.***> Subject: Re: [musescore/MuseScore] #17012: Add option to disable "Play notes when editing" if input comes from MIDI keyboard (PR #24352) But what do you expect to happen if "play notes while editing" is ON and "play midi input" if OFf and you use the midi keyboard in note input mode? Based on the initial request I'm assuming MuseScore shouldn't play the notes. I'm not sure if that's what's been implemented here... Yes, that was the request and that's what was implemented. The design I showed above would not change the behavior implemented for this scenario. It would just allow the opposite too (hear MIDI input, but don't hear anything when clicking on notes or changing their pitch). — Reply to this email directly, view it on GitHub<#24352 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABI5UAJPWOIZ56TWAYA4FYLZU5QDTAVCNFSM6AAAAABNONNIIGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMRZHA2TEMJVGY>. You are receiving this because you were mentioned.Message ID: ***@***.***>

wizofaus · 2024-09-04T20:14:14Z

src/notation/internal/notationmidiinput.cpp

-
-        playbackController()->playElements(notesItems);
+        if (playbackConfiguration()->playNotesOnMidiInput()) {
+            playbackController()->playElements(notesItems);


unfortunately that function (PlaybackController::playElements) then always looks at the other preference (play notes while editing) so if you have that off, MIDI input will never play back.

wizofaus · 2024-09-04T20:15:58Z

Also, I double checked the code - the way it's written currently if "play notes while editing" is OFF then playback on MIDI input is effectively disabled too, regardless of whether you're in "edit"/note entry mode or not. That's an existing behaviour, you can verify with a current 4.4 build. It even affects the onscreen keyboard.
So I'd say that option is either poorly named or improperly implemented currently.
I also noticed that dragging notes up and down does NOT trigger playback even when that option is ON, I swear it used to (in 3.6 at least!)...it does when using arrows on the computer keyboard.

avvvvve · 2024-09-04T20:39:33Z

I didn't mean the code implementation does not need to be changed, just that the "Edit On, MIDI Off" scenario currently works as expected. I'm suggesting we un-nest the MIDI preference which would require a code change.

Good point that the MIDI preference should also affect whether you hear the piano keyboard, so we could change the copy to reflect that. (maybe "Play MIDI/piano keyboard panel input" though that's a mouthful...will think on it)

Feel free to log the note-dragging issue as a bug... though that might be by design (I don't know the history on that).

JohnodonCode · 2024-09-05T00:18:08Z

Changes:

Note: For demonstration purposes, non-MIDI note entry will be using a mouse. If you do not see the mouse input a note, it was done via MIDI keyboard.

Note Preview preferences work as suggested:

2024-09-04.20-03-20.mp4

Both "Play notes when editing" and "Play MIDI input" enabled:

2024-09-04.20-05-14.mp4

Only "Play notes when editing" enabled:

2024-09-04.20-14-34.mp4

Only "Play MIDI Input" enabled:

2024-09-04.20-15-23.mp4

Nothing will play if both are disabled.

Please review @RomanPudashkin , @avvvvve , @bkunda

src/playback/internal/playbackcontroller.cpp

JohnodonCode · 2024-09-05T00:41:45Z

I purposely named it "isMidi" because the conditional checks if midi note playing is enabled and I didn't want to make any major changes.

…

On Wed, Sep 4, 2024 at 8:39 PM wizofaus ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/playback/internal/playbackcontroller.cpp <#24352 (comment)> : > @@ -310,13 +310,13 @@ void PlaybackController::setTrackSoloMuteState(const InstrumentTrackId& trackId, m_notation->soloMuteState()->setTrackSoloMuteState(trackId, state); } -void PlaybackController::playElements(const std::vector<const notation::EngravingItem*>& elements) +void PlaybackController::playElements(const std::vector<const notation::EngravingItem*>& elements, bool isMidi) maybe call the parameter "isEditing" to match the "playNotesWhenEditing" function, and to allow for still getting playback when using on-screen piano keyboard even if playNotesWhenEditing is off? — Reply to this email directly, view it on GitHub <#24352 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ALTBUMLRCCI4UVOIPJKUYSTZU6R3DAVCNFSM6AAAAABNONNIIGVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDEOBRGY2DANRRGM> . You are receiving this because you were mentioned.Message ID: ***@***.***>

avvvvve · 2024-09-05T15:53:11Z

@JohnodonCode Looks good to me! Bradley will be back in on Monday so I'll remind him to give this a once-over before we merge it. Thanks!

avvvvve · 2024-09-12T16:37:51Z

@JohnodonCode quick update, chatted with Bradley today and I'm going to tweak the design a bit. I may not get to it this week but I'll write here as soon as we've agreed on something!

JohnodonCode · 2024-09-12T17:17:18Z

@avvvvve Okay. I do also want someone to review my code, as I did make a change to the playElements method in playbackcontroller by adding an argument. I gave the argument a default value so nothing would break, but I still want someone to review the changes.

avvvvve · 2024-09-12T17:20:03Z

@RomanPudashkin could you take a look at the above?

The design changes I mentioned will be purely UI. The playback piece has the desired behavior already.

cbjeukendrup

This looks very good! The only thing one could say is that the order of the methods is not everywhere consistent. Doesn't matter much, but if you want to strive for perfection then you could fix that. I'd follow the same order as the UI does, so

playNotesWhenEditing
playChordsWhenEditing
playHarmonyWhenEditing
playNotesOnMidiInput
(duration where applicable)

src/appshell/qml/Preferences/NoteInputPreferencesPage.qml

src/appshell/qml/Preferences/internal/NoteInputPlaySection.qml

avvvvve · 2024-11-05T22:41:56Z

@JohnodonCode Apologies for the delay, but after iterating more on this and taking into consideration other preferences that are being added in parallel, I've got a new design for you if you are still able to help out! Let me know if you have any questions.

See this PDF for the complete details: Preferences Note input MIDI.pdf

And here's a peek of the default state:

cbjeukendrup

This looks very good already! Just a few more minor comments:

src/appshell/qml/Preferences/internal/NoteInputPlaySection.qml

src/appshell/view/preferences/noteinputpreferencesmodel.cpp

- added accessibility label - added anchors in place of height & verticalAlignment - removed redundant changes - fixed navigation row order in both NoteInputPlaySelection and NoteInputSection

avvvvve · 2025-01-24T05:09:07Z

@JohnodonCode Thanks for turning this around! The UI looks great.

Good catch about 'Play chords when editing', we noticed that too but didn't think to split it out into a separate playback-related task. I've logged that here: #26219

(Hopefully) small UI thing that's not a blocker to merge this. When turning off "Enable MIDI input" which disables the "Play MIDI input" option, if the latter was on, the checkbox stays on in the disabled state, which could (incorrectly) suggest that the setting is on.

If it's relatively simple, could we do the following?

If the user checks "Enable MIDI input" off:

Disable "Play MIDI input" and hide the checkmark if it was on
When turning "Enable MIDI input" back on, enable "Play MIDI input" and return it to whichever ON/OFF state it was in before it was disabled

We could do the same for all the "child" checkboxes of the "Hear playback..." toggle when it's turned off.

JohnodonCode · 2025-01-24T19:50:33Z

@avvvvve I have updated the UI with the requested change, see below

2025-01-24.14-49-13.mp4

avvvvve · 2025-01-25T00:16:04Z

BEAUTIFUL! Thank you so much for working with all the feedback and the different iterations of the design. It looks awesome.

@cbjeukendrup mind doing one more code review?

cbjeukendrup · 2025-02-26T23:39:18Z

A rebase was not really doable here (a lot of conflicting changes to the same code were made in #26291 and other PRs), so I went for a merge. Added one additional cleanup commit as well. @avvvvve To be safe, this might now need another test...

avvvvve

Tested and approved! Thanks!

…pr/24352

…ters They are already emitted anyway because of the connections made in `NoteInputPreferencesModel::load()`.

cbjeukendrup · 2025-02-27T16:16:31Z

(That was one more push to fix a mistake I made while merging, but no retesting is needed now, the automated tests will speak for themselves :) )

JohnodonCode force-pushed the disableMidiPlayback branch from b54300d to cd29427 Compare September 1, 2024 03:17

JohnodonCode marked this pull request as draft September 1, 2024 03:41

JohnodonCode marked this pull request as ready for review September 1, 2024 19:18

JohnodonCode changed the title ~~Add option to disable "Play notes when editing" if input comes from MIDI keyboard~~ #17012: Add option to disable "Play notes when editing" if input comes from MIDI keyboard Sep 2, 2024

RomanPudashkin approved these changes Sep 3, 2024

View reviewed changes

RomanPudashkin assigned bkunda and avvvvve Sep 3, 2024

wizofaus reviewed Sep 4, 2024

View reviewed changes

wizofaus reviewed Sep 5, 2024

View reviewed changes

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

JohnodonCode requested review from RomanPudashkin and wizofaus September 5, 2024 01:59

JohnodonCode force-pushed the disableMidiPlayback branch from af86afc to 68142bb Compare September 5, 2024 15:02

cbjeukendrup reviewed Oct 23, 2024

View reviewed changes

src/appshell/qml/Preferences/NoteInputPreferencesPage.qml Show resolved Hide resolved

src/appshell/qml/Preferences/internal/NoteInputPlaySection.qml Outdated Show resolved Hide resolved

JohnodonCode added 6 commits January 22, 2025 16:29

Amend playChordSymbol to avoid confusion

a71949f

Fix general codestyle

3aa4658

Empty commit to retest vtests

c40cd38

Reflect disabling of "Play notes when editing" to "Play MIDI Input"

11fd7ff

Revert changes to playChordBox due to MuseScore's chord handling

25f5cc1

Duration should only be enabled with playNotesWhenEditing

5171eb9

JohnodonCode force-pushed the disableMidiPlayback branch from 7d76d31 to 5171eb9 Compare January 22, 2025 21:29

Fix conflict from rebase (oops)

c2dad38

cbjeukendrup reviewed Jan 23, 2025

View reviewed changes

Fixes based on review + fix nav order

d6128c3

- added accessibility label - added anchors in place of height & verticalAlignment - removed redundant changes - fixed navigation row order in both NoteInputPlaySelection and NoteInputSection

JohnodonCode requested a review from cbjeukendrup January 23, 2025 21:44

This was referenced Jan 24, 2025

Add 'Play notes on muted tracks' preference to Note input section #26224

Open

'Play chord when editing' preference changes #26219

Open

Show empty checkboxes when section is disabled

8c91ff7

avvvvve requested review from avvvvve and removed request for wizofaus January 25, 2025 00:16

avvvvve approved these changes Jan 25, 2025

View reviewed changes

cbjeukendrup force-pushed the disableMidiPlayback branch from 87cb717 to 29d112e Compare February 26, 2025 23:23

avvvvve self-requested a review February 27, 2025 15:22

avvvvve approved these changes Feb 27, 2025

View reviewed changes

cbjeukendrup added 2 commits February 27, 2025 17:10

Merge branch 'master' of https://github.com/musescore/MuseScore into …

ace637c

…pr/24352

NoteInputPreferencesModel: remove redundant signal emissions from set…

5cd86a8

…ters They are already emitted anyway because of the connections made in `NoteInputPreferencesModel::load()`.

cbjeukendrup force-pushed the disableMidiPlayback branch from 070d56c to 5cd86a8 Compare February 27, 2025 16:11

cbjeukendrup merged commit c7541a2 into musescore:master Feb 27, 2025
11 checks passed

cbjeukendrup mentioned this pull request Feb 27, 2025

Porting PRs to 4.5.0 #26805

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#17012: Add option to disable "Play notes when editing" if input comes from MIDI keyboard #24352

#17012: Add option to disable "Play notes when editing" if input comes from MIDI keyboard #24352

JohnodonCode commented Sep 1, 2024 •

edited

Loading

RomanPudashkin commented Sep 3, 2024

avvvvve commented Sep 4, 2024

JohnodonCode commented Sep 4, 2024

wizofaus commented Sep 4, 2024

avvvvve commented Sep 4, 2024

wizofaus commented Sep 4, 2024

avvvvve commented Sep 4, 2024

wizofaus commented Sep 4, 2024 via email

wizofaus Sep 4, 2024

wizofaus commented Sep 4, 2024 •

edited

Loading

avvvvve commented Sep 4, 2024

JohnodonCode commented Sep 5, 2024

JohnodonCode commented Sep 5, 2024 via email

avvvvve commented Sep 5, 2024

avvvvve commented Sep 12, 2024

JohnodonCode commented Sep 12, 2024 •

edited

Loading

avvvvve commented Sep 12, 2024

cbjeukendrup left a comment

avvvvve commented Nov 5, 2024

cbjeukendrup left a comment

avvvvve commented Jan 24, 2025

JohnodonCode commented Jan 24, 2025

avvvvve commented Jan 25, 2025

cbjeukendrup commented Feb 26, 2025

avvvvve left a comment

cbjeukendrup commented Feb 27, 2025

#17012: Add option to disable "Play notes when editing" if input comes from MIDI keyboard #24352

#17012: Add option to disable "Play notes when editing" if input comes from MIDI keyboard #24352

Conversation

JohnodonCode commented Sep 1, 2024 • edited Loading

RomanPudashkin commented Sep 3, 2024

avvvvve commented Sep 4, 2024

JohnodonCode commented Sep 4, 2024

wizofaus commented Sep 4, 2024

avvvvve commented Sep 4, 2024

wizofaus commented Sep 4, 2024

avvvvve commented Sep 4, 2024

wizofaus commented Sep 4, 2024 via email

wizofaus Sep 4, 2024

Choose a reason for hiding this comment

wizofaus commented Sep 4, 2024 • edited Loading

avvvvve commented Sep 4, 2024

JohnodonCode commented Sep 5, 2024

Changes:

Note Preview preferences work as suggested:

Both "Play notes when editing" and "Play MIDI input" enabled:

Only "Play notes when editing" enabled:

Only "Play MIDI Input" enabled:

JohnodonCode commented Sep 5, 2024 via email

avvvvve commented Sep 5, 2024

avvvvve commented Sep 12, 2024

JohnodonCode commented Sep 12, 2024 • edited Loading

avvvvve commented Sep 12, 2024

cbjeukendrup left a comment

Choose a reason for hiding this comment

avvvvve commented Nov 5, 2024

cbjeukendrup left a comment

Choose a reason for hiding this comment

avvvvve commented Jan 24, 2025

JohnodonCode commented Jan 24, 2025

avvvvve commented Jan 25, 2025

cbjeukendrup commented Feb 26, 2025

avvvvve left a comment

Choose a reason for hiding this comment

cbjeukendrup commented Feb 27, 2025

JohnodonCode commented Sep 1, 2024 •

edited

Loading

wizofaus commented Sep 4, 2024 •

edited

Loading

JohnodonCode commented Sep 12, 2024 •

edited

Loading