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

Osara move to chord actions generating MIDI editor: Unselect all events undo points #1088

Open
ns-studios opened this issue May 25, 2024 · 18 comments

Comments

@ns-studios
Copy link

I don't know if this has been talked about before, or if there's even anything Osara can do about it, but using actions such as
OSARA: Move to next chord
OSARA: Move to previous chord
OSARA: Move to previous note in chord
OSARA: Move to next note in chord
in the midi editor seems to generate
MIDI editor: Unselect all events
undo point every time they're used which then get in the way when trying to undo an actual action like move notes up one semitone, because you have to undo the unselect all events actions first.
Ways to reproduce:
open midi editor, and select piano role
Item: Open in built-in MIDI editor (set default behavior in preferences)
Mode: Piano Roll
left/right/up/down arrow around the notes
OSARA: Move to previous chord
OSARA: Move to next chord
OSARA: Move to previous note in chord
OSARA: Move to next note in chord
then undo, and you should get at least one
MIDI editor: Unselect all events
point wen undoing.
Sometimes only one unselect undo point is made after first using left/right arrow, and then another after editing one of the notes, but sometimes every use of arrows seems to generate an undo point, which can get really annoying to undo.
Would it be possible for Osara not to generate undo points for unselect all unless the action is performed manually?

@ScottChesworth
Copy link
Collaborator

I'm adding some undo blocks now, giving those points friendlier names that should also work well in other languages (think it'll happen automatically as the action names are already translated).
IMO removing undo points around navigation would need some discussion of the tradeoffs, seeing as navigation is tied to selection. Also... er... I don't know how that would be done yet.

@ns-studios
Copy link
Author

@ScottChesworth
I've just tried the build with the changes.
I don't think this is much better. I now get an undo point for every osara chord action.
The previous behavior with the unselect all events did exactly that at worst, but sometimes I would only get the unselect all point when using the chord osara action for the first time when opening the midi editor, and then the following actions wouldn't generate any more undo points until I edited the note and then another one would be generated.
I'm even confused why the unselecting would be happening as I'm not manually selecting events, just navigating through them.

@pitermach
Copy link

pitermach commented May 25, 2024 via email

@ScottChesworth
Copy link
Collaborator

Hmm I think something can be done here, but not sure what yet. Agreed that the approach I took earlier isn't it. Will keep experimenting.

@jcsteh
Copy link
Owner

jcsteh commented May 26, 2024

I'm even confused why the unselecting would be happening as I'm not manually selecting events, just navigating through them.

Navigating through chords or notes selects them, so we have to unselect them when navigating again unless you're doing multiple selection.

You can compress multiple undo points into a single, custom undo point, but i don't know of a way from the API to completely prevent one of REAPER's own undo points. The only thing I can't recall whether I've tried is passing an empty string to Undo_EndBlock, but I very much doubt that'll do it.

@XHG20033
Copy link

I think the "Unselect all events" undo point is unnecessary most of the time, because when we press the cursor keys, their function is to browse the contents of the MIDI editor, not to change it. Since the event has not changed there is no need to create an undo point.
However, OSARA may need to Unselect all events in the process of browsing notes, but I personally think that at the user level, this step does not need to be presented in most cases. As other friends said, undoing previously made changes requires undoing several "Unselect all events", which is obviously not a good experience.
Or similar to REAPER, provide an option in the settings for users to choose whether to create undo points.

@ScottChesworth
Copy link
Collaborator

I can't recall whether I've tried passing an empty string to Undo_EndBlock, but I very much doubt that'll do it.

Tried that just now and nope.

@jcsteh
Copy link
Owner

jcsteh commented May 26, 2024

I think the "Unselect all events" undo point is unnecessary most of the time, because when we press the cursor keys, their function is to browse the contents of the MIDI editor, not to change it.

That's understood. The key point is that we aren't intentionally generating these undo points. REAPER is and we can't prevent that.

We could try unselecting each selected event ourselves using API calls, rather than executing a REAPER action to do this. My concern is that this might be slow if there are a lot of events selected, whereas REAPER can probably do this faster internally, but I guess it's worth a shot.

@ScottChesworth
Copy link
Collaborator

Shall I poke Justin before we refactor things, just in case there's some trick we don't know about using UndoBlock?

@jcsteh
Copy link
Owner

jcsteh commented May 26, 2024

UP to you. I'd say there's a 99.9% chance we're not missing something, but I guess it can't hurt to ask if you haven't used up your Justin credits this month. :)

@ScottChesworth
Copy link
Collaborator

Done. Will report back when I hear back.

@ScottChesworth
Copy link
Collaborator

Yeah we didn't miss anything. Justin says he'll have a think about a way to prevent selection state being included in undo, says he might be able to add an option.

@ns-studios
Copy link
Author

ns-studios commented May 26, 2024

That would be cool if there could be an option to not add midi editor selection actions to undo just like Pitermach mentioned there are options for item/track/envelope selection.
Thank you so much for considering this and even forwarding it to the Reaper devs.

Yeah we didn't miss anything. Justin says he'll have a think about a way to prevent selection state being included in undo, says he might be able to add an option.

@ScottChesworth
Copy link
Collaborator

So in today's dev build of REAPER, there's a new Undo category in Prefs, and MIDI events is one of the things that can be unchecked. However, right now it doesn't prevent those unselect undo points from showing up. Hmmm. Not sure where to go from here.

@jcsteh
Copy link
Owner

jcsteh commented May 28, 2024

That feels like a bug. That undo point is generated by REAPER's Unselect all action, so it would seem that undo option isn't applying there?

I wouldn't want this to prevent us from undoing insertion, length changes, etc. We just want to be able to avoid undo points for selection changes in the MIDI editor.

@ScottChesworth
Copy link
Collaborator

I've followed up and asked for "Edit: Unselect all" to not generate an undo point. Suppose Justin wants to keep this behaviour though, I'm thinking a new "Edit: Unselect notes" action would meet our needs so long as it was included in the new option, right?

@jcsteh
Copy link
Owner

jcsteh commented May 29, 2024

I'm slightly concerned that inserting events and lengthening notes might be part of the new undo setting. Are they?

Edit: Unselect notes would be a change in behaviour I think. If you select all, then press right arrow, you kinda expect that pressing delete will only delete the note you just selected. With unselect notes, I think that will end up deleting any CCs that got selected. Though I'm not entirely sure how select all and unselect all behave with respect to CCs, so maybe I'm wrong.

@ScottChesworth
Copy link
Collaborator

I'm slightly concerned that inserting events and lengthening notes might be part of the new undo setting. Are they?

No. I was clear that we only wanted it effecting selection. Have just verified that inserting and lengthening are both still fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants