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

shortcut refactor for songeditor #3649

Merged
merged 4 commits into from
Aug 27, 2017
Merged

Conversation

BaraMGB
Copy link
Contributor

@BaraMGB BaraMGB commented Jun 20, 2017

This cleans up the keyboard short cuts for the song editor to being consist with the Piano Roll editor. I want to have the same behavior as in the Piano Roll editor.

What is implemented so far:

hold Ctrl - toggle to selection mode
you can select your Tcos and by release the Ctrl key it turns back into draw mode.

deselect all selections by:
click on empty space (new Tco)
click on unselected Tco
[Ctrl] + [shift] + A
[Esc]

select all Tcos
[ctrl] + A
EDIT:
[Ctrl] + drag
copies a selection or an unselected Tco
no matter the mode (draw or selection)

click + drag
moves a selection or an unselected Tco
no matter the mode (draw or selection)

[DEL]
deletes all selected Tcos

@tresf
Copy link
Member

tresf commented Jun 20, 2017

Ctrl-dragging is proper copying in my opinion, not shift. Isn't this how we copy full tracks?

@BaraMGB
Copy link
Contributor Author

BaraMGB commented Jun 20, 2017

Then we should change this in the piano roll. But I fear the short cuts of the piano roll are more burned to the brain of the users than the cuts of the song editor. Piano roll should be our basis. There the people work at most.

@grejppi
Copy link
Contributor

grejppi commented Jun 20, 2017

Ctrl+dragging sounds more like copying to me too. I still get really confused by Shift+dragging in the Piano Roll 😥

@tresf
Copy link
Member

tresf commented Jun 20, 2017

Ctrl+dragging sounds more like copying to me too.

Ctrl+drag is the default for Windows and Ubuntu to make a copy of a file. (Mac uses Shift + Alt).

@Umcaruje
Copy link
Member

I vote for ctrl+drag as well, it's consistent with the other software I use 👍

@BaraMGB
Copy link
Contributor Author

BaraMGB commented Jun 20, 2017

I'll try my best. 😑

@BaraMGB
Copy link
Contributor Author

BaraMGB commented Jun 20, 2017

I have updated this PR. [Ctrl] is now for copy. For Piano Roll Editor though I'll make a new pull request. This will be a deeper dive into the code.

This PR should be tested.

@PhysSong
Copy link
Member

The Qt4 builds fails, because of QAction::triggered(). I think it should be replaced with QAction::trigger() or QAction::activate(QAction::Trigger).
@BaraMGB @tresf @Umcaruje Please test this suggestion for both Qt4 and Qt5.

@BaraMGB
Copy link
Contributor Author

BaraMGB commented Jun 21, 2017

@PhysSong thank you.

@zonkmachine
Copy link
Member

Tested. ❤️

When you delete a whole lot at once and go Ctrl+z to undo, the undo action is done in steps. I think it should all be done in one step.

@BaraMGB
Copy link
Contributor Author

BaraMGB commented Jul 9, 2017

@zonkmachine

When you delete a whole lot at once and go Ctrl+z to undo, the undo action is done in steps. I think it should all be done in one step.

that sounds good. Can you give me a hint how to do so?

@tresf
Copy link
Member

tresf commented Jul 9, 2017

Agreed. The journaling should record everything between mouse press and mouse release when deleting.

@zonkmachine
Copy link
Member

that sounds good. Can you give me a hint how to do so?

Sorry, no.

@PhysSong
Copy link
Member

PhysSong commented Jul 12, 2017

Well, there are several methods to do so. An example is modifying ProjectJournal class to enable squashing of journaling points.
Another example, modifying TrackContainerObjectView::remove() to TrackContainerObjectView::remove( bool createJournalingPoint = true ) and add journaling point of whole song(I think it is simple but inefficient).

@BaraMGB
Copy link
Contributor Author

BaraMGB commented Jul 20, 2017

An example is modifying ProjectJournal class to enable squashing of journaling points.

For this one I need help. I don't get how to do this. @PhysSong

@PhysSong
Copy link
Member

@BaraMGB I think it will take some time to do such things. I'll give it a try soon.

@PhysSong
Copy link
Member

const int ProjectJournal::MAX_UNDO_STATES = 100;

It brings another problem: If I delete more than 100 TCOs, I can't undo it completely.

@zonkmachine
Copy link
Member

const int ProjectJournal::MAX_UNDO_STATES = 100;

Just increase this? Can you count the number of objects and override this MAX when deleting many TCOs at once?

@PhysSong
Copy link
Member

PhysSong commented Jul 21, 2017

@zonkmachine Then it shouldn't be const anymore. For some projects, 1000 isn't enough.
I tried increasing the max number more and more, but I can't completely undo delete for some project because of too much time and memory usage.

@zonkmachine
Copy link
Member

I don't know. Maybe it's leaner and faster to copy the complete project at a certain point?
How strong is your machine?

@PhysSong
Copy link
Member

CPU: Intel® Core™ i5-4210M
RAM: 4GB

For demo project BuzzerBeater, It takes memory more than 1GB.

@BaraMGB
Copy link
Contributor Author

BaraMGB commented Aug 26, 2017

@zonkmachine wrote:

When you delete a whole lot at once and go Ctrl+z to undo, the undo action is done in steps. I think it should all be done in one step.

By investigate some PR and issues and test a little bit in lmms I came to the conclusion that the UNDO-System of lmms needs a lot of work. But I don't think, this is in the scope of this PR. (For example: if you copy a bunch of TCOs in master build, you got the same undo issue)

I have tested this PR and (excepts the undo issue) it works like a charm. I don't wanna work without this changes in the songeditor. If there are no other issues with this PR I vote for merging this and file an issue for reworking the UNDO system. Perhaps @curlymorphic can help here. He introduced the undo for the songeditor here: #1699

@zonkmachine
Copy link
Member

I agree on this and vote merge too.

@BaraMGB
Copy link
Contributor Author

BaraMGB commented Aug 26, 2017

@zonkmachine Thank you. If there are no more objections I will merge it tomorrow. 👍

@tresf
Copy link
Member

tresf commented Aug 26, 2017

The description should be updated as to not cause confusion. Shift + drag seems to have been replaced with Ctrl + drag.

Sorry, was reading email and not PR. Description is good. :)

@BaraMGB BaraMGB merged commit 9b5d379 into LMMS:master Aug 27, 2017
sdasda7777 pushed a commit to sdasda7777/lmms that referenced this pull request Jun 28, 2022
* shortcut refactor for songeditor

* Ctrl-modifier for copy Tcos

* QAction()::trigger methode qt4 compatible
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.

6 participants