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

Pattern editor refactor (and NotePropertiesRuler selection support) #964

Merged
merged 52 commits into from
Jan 6, 2021

Conversation

cme
Copy link
Contributor

@cme cme commented Oct 22, 2020

Refactoring and cleanup of pattern editors to share commonalities between drum and piano editors

@cme cme changed the title Dev pattern editor refactor Pattern editor refactor Dec 6, 2020
@theGreatWhiteShark
Copy link
Contributor

Ah, and another thing: rectangle selection using the cursor + Shift works fine in the PatternEditor but unfortunately not in the SongEditor

@cme
Copy link
Contributor Author

cme commented Dec 31, 2020

Ah, and another thing: rectangle selection using the cursor + Shift works fine in the PatternEditor but unfortunately not in the SongEditor

Yep, the SongEditor hasn't been refactored to use the same Selection model yet. Still working on that, it'll be a different PR.

As discussed in hydrogen-music#1050, this gives a visual hint to the user
of the area that can actually be interacted with, since the
indicators themselves don't hint at the area.
@theGreatWhiteShark
Copy link
Contributor

Some more notes (well, they are somewhat further things that could be done on top of your PR. Not something that needs implement right now (especially not before the release. Just putting them here because of a lack of another place. Maybe a Github project?)

I just found some time to use Hydrogen again and I'm amazed how much the interaction improved by selection and the keyboard input. However, there are still quite a number of low hanging fruits like

  • Supporting [page-up], [page-down], [home], and [end]
  • Allow for scrolling during the drag selection
  • Allow for a drag selection when pressing e.g. [ctrl]+mouse even during the drawning mode (I'm not quite sure if we still have to have two separate modes at all)

and there are some parts that are still a huge pain to interact with and could benefit from the refactoring too, like the automation path.

I would suggest we could dwell on the UX including consistent cursor changes, highlighting the current focus, providing a configurable keyboard layout etc. for the 1.2 release.

@cme
Copy link
Contributor Author

cme commented Jan 3, 2021

  • Supporting [page-up], [page-down], [home], and [end]

Home and End should be covered by the standard Qt mappings of QKeySequence::MoveToStartOfLine and QKeySequence::MoveToEndOfLine, ie. move to the time beginning and ends of the current pattern / instrument line.

But yes, page-up/page-down should be added too. Thanks!

  • Allow for scrolling during the drag selection

Excellent idea! Yes.

  • Allow for a drag selection when pressing e.g. [ctrl]+mouse even during the drawning mode (I'm not quite sure if we still have to have two separate modes at all)

Having already folded most of the functionality of the drawing mode into the select mode (indeed all the functionality of it that existed before 1.0), I think it makes sense to change the default mode to be the 'select' mode, but I think there's enough use for the continuous-draw mode that it's worth keeping Draw mode. If it's not the default mode, I don't think interacting with selections in it seems necessary?

I would suggest we could dwell on the UX including consistent cursor changes, highlighting the current focus, providing a configurable keyboard layout etc. for the 1.2 release.

Sounds good. I'll probably merge this in the next few days (as the Song Editor refactor work I'm doing is on a branch off of this, so would make a very messy PR until this is merged).

Thanks for the feedback and encouragement! :)

@theGreatWhiteShark
Copy link
Contributor

Home and End should be covered by the standard Qt mappings of QKeySequence::MoveToStartOfLine and QKeySequence::MoveToEndOfLine, ie. move to the time beginning and ends of the current pattern / instrument line.

Ah, I see. Haven't got accustomed to the cursor yet. I was more referring to the transport position. So, how about a keyboard shortcut to start to relocate to the position the cursor is located and start playback there. Also while at it: how about a keyboard shortcut to switch to the pattern in the PatternEditor the cursor is located in the SongEditor at. More or less using it's vertical and horizontal position for selection/navigation.

When pressing the former shortcut while the cursor is focused on the PatternEditor or NotePR the playback mode could change to Pattern mode and start playback at that particular Note.

Having already folded most of the functionality of the drawing mode into the select mode (indeed all the functionality of it that existed before 1.0)

I might be missing something here. But when making the continuous drawing the default interaction of the select mode and binding the drag selection to Ctrl+left mouse key both modes would have been merged (given that the behavior of mouse press and Ctrl+mouse press differs whether a selected (group of) note(s) as clicked or not).

@cme
Copy link
Contributor Author

cme commented Jan 6, 2021

Also while at it: how about a keyboard shortcut to switch to the pattern in the PatternEditor the cursor is located in the SongEditor at. More or less using it's vertical and horizontal position for selection/navigation.

When pressing the former shortcut while the cursor is focused on the PatternEditor or NotePR the playback mode could change to Pattern mode and start playback at that particular Note.

Interesting, that would be handy! Any ideas on what would seem like an appropriate key combination to use?

Having already folded most of the functionality of the drawing mode into the select mode (indeed all the functionality of it that existed before 1.0)

I might be missing something here. But when making the continuous drawing the default interaction of the select mode and binding the drag selection to Ctrl+left mouse key both modes would have been merged (given that the behavior of mouse press and Ctrl+mouse press differs whether a selected (group of) note(s) as clicked or not).

Sorry, I don't mean that the exact behaviour is the same, just that pre-1.0 the 'draw' mode would only set/clear active cells, and that this can now be done from the select mode as well.

@cme cme merged commit a99089f into hydrogen-music:master Jan 6, 2021
@theGreatWhiteShark
Copy link
Contributor

Any ideas on what would seem like an appropriate key combination to use?

How about ctrl+space?

@theGreatWhiteShark
Copy link
Contributor

Another note (should I put them here or in a separate issue?)

In the PatternEditor I can delete a selected group via del but not an individual note using the cursor + del.

@cme
Copy link
Contributor Author

cme commented Jan 9, 2021

In the PatternEditor I can delete a selected group via del but not an individual note using the cursor + del.

Oh, nice! Yes, delete should definitely delete a note under the keyboard cursor. Will add!

Another note (should I put them here or in a separate issue?)

Here seems as good a place as any, I still have it on notifications and it feels wrong to pollute the issue tracker with these.

Thanks a ton for trialing this and reporting! :)

@theGreatWhiteShark
Copy link
Contributor

Some more:

  • I found the copy-cut-paste behavior using keyboard unexpected. What would expect is that when pasting the current cursor position or the top left corner of the mouse cursor would become the top left corner of the selected rectangle. But instead it's the bottom right corner. Is this the standard way of doing it (come to think of it, I might already live in my tiling window manager world for too long)
  • paste/cut/copy does not work in the SongEditor yet (but I guess you are fully aware of this)
  • When editing in the NotePropertyRuler/PatternEditor using keyboard and selecting a different Instrument/Pattern in the PatternEditor/SongEditor the focus stays in the latter. While this is totally plausible from the perspective of the code I was a bit confused at first when using Hydrogen. After all, clicking on a different pattern has no impact on the SongEditor at all. How about not changing focus when clicking on the side pannels?

Thanks a ton for trialing this and reporting! :)

No problem. Thanks for writing all that code 😉

@cme
Copy link
Contributor Author

cme commented Jan 11, 2021

  • I found the copy-cut-paste behavior using keyboard unexpected. What would expect is that when pasting the current cursor position or the top left corner of the mouse cursor would become the top left corner of the selected rectangle. But instead it's the bottom right corner. Is this the standard way of doing it (come to think of it, I might already live in my tiling window manager world for too long)

Hmm, this also sounds like a reasonable expectation to me, and I think it's the way spreadsheets typically behave. (I'm assuming by 'top left' and 'bottom right' you mean the locations of where the selection was started and where the selection was finished?).

I think the difference is made by the way the selection is defined and expanded. In the common selection manager (at the moment) the cursor is moved when expanding the selection, which is defined between the start point, and the current cursor location. In a quick survey of a few spreadsheets, the cursor stays where it is, and the opposite corner of the selection box is moved.

Does that sound like the sort of behaviour you'd expect?

Would need a little bit of restructuring in the selection manager and the keypress handlers since all the movement is currently based on the cursor position, but not too drastic.

  • paste/cut/copy does not work in the SongEditor yet (but I guess you are fully aware of this)

Yep, see #1079 :)

  • When editing in the NotePropertyRuler/PatternEditor using keyboard and selecting a different Instrument/Pattern in the PatternEditor/SongEditor the focus stays in the latter. While this is totally plausible from the perspective of the code I was a bit confused at first when using Hydrogen. After all, clicking on a different pattern has no impact on the SongEditor at all. How about not changing focus when clicking on the side pannels?

Easy enough to do, I'll try it!

Hmm, increasingly thinking of creating a project board now. :D

Thanks!

@theGreatWhiteShark
Copy link
Contributor

I'm assuming by 'top left' and 'bottom right' you mean the locations of where the selection was started and where the selection was finished?

Exactly. Which are of course not the correct terms since they assume right-handedness as default.

But for the point of insertion I do refer to the top left pixel - or the tip - of the cursor.

In the common selection manager (at the moment) the cursor is moved when expanding the selection, which is defined between the start point, and the current cursor location. In a quick survey of a few spreadsheets, the cursor stays where it is, and the opposite corner of the selection box is moved.

I do like the current way of selection via keyboard. Moving the cursor while expanding/shrinking the selection does feel like text editing and very familiar. It's just the pasting where I expected the first grid cell the selection was started at being insert at the cursor when pressing Crtl+v.

Hmm, increasingly thinking of creating a project board now. :D

Yeah. Maybe even an associated Discussion thread to have more user feedback. Come to think of it it's not particular low-threshold to comment a PR for people who haven't contributed yet :D

@theGreatWhiteShark
Copy link
Contributor

Also I wonder whether hiding the cursor should be the default behavior. People already familiar with Hydrogen might miss out on that feature because they don't feel the need for keyboard interaction. On the other hand, turning it off if it's annoying is a rather simple thing to do.

@cme
Copy link
Contributor Author

cme commented Jan 14, 2021

In the common selection manager (at the moment) the cursor is moved when expanding the selection, which is defined between the start point, and the current cursor location. In a quick survey of a few spreadsheets, the cursor stays where it is, and the opposite corner of the selection box is moved.

I do like the current way of selection via keyboard. Moving the cursor while expanding/shrinking the selection does feel like text editing and very familiar. It's just the pasting where I expected the first grid cell the selection was started at being insert at the cursor when pressing Crtl+v.

I think I've got a little confused reading your description (because now we have "cursor" that can be keyboard cursor or mouse cursor) :D Currently, copying and pasting with keyboard movement:

  • Start selection at, say, top left.
  • Final keyboard cursor position is bottom right.
  • Copy
  • Move keyboard cursor elsewhere
  • Paste: pasted selection appears above and to the left of current keyboard cursor
    • should it? Or should the selection be pasted to the right and below the keyboard cursor?

With mouse movements:

  • Start selection drag from top left
  • Release mouse at bottom right
  • Ctrl+C (or Right-click + copy)
  • Move elsewhere
  • Right click + paste: pasted selection appears above and to the left of click point.
    • Should it? Or should the selection be pasted to the right and below the mouse cursor click point?

Having dug a bit more into spreadsheet behaviour, they seem to always paste such that the top left (not even the 'start point' of the selection as I'd assumed, just always 'top left').

This doesn't make it clear what should be done in cases where a selection has been made in multiple stages by using Drag and then Ctrl+Drag. When I tried this with LibreOffice and Excel, both refused to copy cells that were selected this way :D I think a sensible behaviour in this circumstance, consistent with the spreadsheet model, would be to use the top left point of the most recent selection action as the 'origin' point of the selection.

@cme
Copy link
Contributor Author

cme commented Jan 14, 2021

Also I wonder whether hiding the cursor should be the default behavior. People already familiar with Hydrogen might miss out on that feature because they don't feel the need for keyboard interaction. On the other hand, turning it off if it's annoying is a rather simple thing to do.

@mauser and @trebmuh were concerned the appearance of the keyboard cursor might throw off existing users, and hiding it unless keyboard navigation keys (which new users will probably instinctively press at some point!) are pressed seemed a good compromise between discoverability and avoiding confusing people. But I think you're right that that leaves old users who already believe the keyboard doesn't do anything without a way to discover it!

I think this is probably mitigated enough by mentioning it prominently in the release notes.

I'm not invested either way, but perhaps this is another potential point I should mention in a Discussion topic :)

@theGreatWhiteShark
Copy link
Contributor

Or should the selection be pasted to the right and below the keyboard cursor?
Or should the selection be pasted to the right and below the mouse cursor click point?

Those would be the things I would expect.

This doesn't make it clear what should be done in cases where a selection has been made in multiple stages by using Drag and then Ctrl+Drag. When I tried this with LibreOffice and Excel, both refused to copy cells that were selected this way :D I think a sensible behaviour in this circumstance, consistent with the spreadsheet model, would be to use the top left point of the most recent selection action as the 'origin' point of the selection.

Hmm. But this does makes the pasting depending on the order of selection. I think I would go for the most top left point of the whole selection.

For example imagine one has such a sophisticated pattern as the one below and is used to copy the whole thing over and over again. Occasionally, the note at bottom right is missed during selection and the user decides to include it with Ctrl+Drag

first_selection

Using the top left point of the most recent selection action as the 'origin' point of the selection would alter the horizontal and vertical offset of the pasted result significantly.

@theGreatWhiteShark
Copy link
Contributor

I think this is probably mitigated enough by mentioning it prominently in the release notes.

+1

@cme
Copy link
Contributor Author

cme commented Jan 17, 2021

Or should the selection be pasted to the right and below the keyboard cursor?
Or should the selection be pasted to the right and below the mouse cursor click point?

Those would be the things I would expect.

Great. Implemented in #1079 , which I'll merge soon.

This doesn't make it clear what should be done in cases where a selection has been made in multiple stages by using Drag and then Ctrl+Drag. When I tried this with LibreOffice and Excel, both refused to copy cells that were selected this way :D I think a sensible behaviour in this circumstance, consistent with the spreadsheet model, would be to use the top left point of the most recent selection action as the 'origin' point of the selection.
[...]
Using the top left point of the most recent selection action as the 'origin' point of the selection would alter the horizontal and vertical offset of the pasted result significantly.

Thanks, this is a great example and really helps make it clear. Definitely feels "top right" is the right move. It also removes any dependency on where the drag rectangle was actually created, which helps if you drag a larger area that doesn't have any notes.

@theGreatWhiteShark
Copy link
Contributor

In the common selection manager (at the moment) the cursor is moved when expanding the selection, which is defined between the start point, and the current cursor location. In a quick survey of a few spreadsheets, the cursor stays where it is, and the opposite corner of the selection box is moved.

I do like the current way of selection via keyboard. Moving the cursor while expanding/shrinking the selection does feel like text editing and very familiar. It's just the pasting where I expected the first grid cell the selection was started at being insert at the cursor when pressing Crtl+v.

After using the new keyboard selection for a couple of days I think I was wrong on this one. One get used to it very quickly but it still feels a little bit weird. Turns out in text editing the cursor does not move either. But pressing [left arrow] or [right arrow] moves the cursor to the left or to the right of the selection. The latter gave me the impression of a moving cursor while selecting.

@theGreatWhiteShark
Copy link
Contributor

So, how about a keyboard shortcut to start to relocate to the position the cursor is located and start playback there. Also while at it: how about a keyboard shortcut to switch to the pattern in the PatternEditor the cursor is located in the SongEditor at. More or less using it's vertical and horizontal position for selection/navigation.

BTW, I'm currently working on this one.

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

3 participants