Change universal key shortcut to insert bars #4526
Conversation
src/gui/editors/SongEditor.cpp
Outdated
| @@ -315,7 +315,7 @@ void SongEditor::keyPressEvent( QKeyEvent * ke ) | |||
| { | |||
| if( /*_ke->modifiers() & Qt::ShiftModifier*/ | |||
| gui->mainWindow()->isShiftPressed() == true && | |||
| ke->key() == Qt::Key_Insert ) | |||
| ke->key() == Qt::Key_Enter ) | |||
There was a problem hiding this comment.
You need to have a precompiler check for Mac here. We're not changing our default shortcut for all OSs. :)
There was a problem hiding this comment.
Or you can make two shortcuts with an ||.
| { | ||
| m_song->insertBar(); | ||
| } | ||
| #endif |
There was a problem hiding this comment.
- I think you accidentally removed a curly bracket here.
- We use LMMS_BUILD_APPLE as our precompiler directive for MacOS.
- You should group the
||logic with parens to be explicit. - @LMMS/developers are there any objections to just removing the precompiler and doing
( ke->key() == Qt::Key_Insert || ke->key() == Qt::Key_Enter || ke->key() == Qt::Key_Return)for all platforms?
There was a problem hiding this comment.
Yeah, I noticed that I accidentally removed the curly bracket - was planning on just creating a new PR.
We should probably test Return on macOS platforms before universally changing it, tho - if it doesn't work, Shift+D and Shift+I would be good options to fulfill the same purpose (as discussed previously).
There was a problem hiding this comment.
there any objections to just removing the precompiler and doing[…] for all platforms?
No objections, sounds like a good idea. It's simpler and avoids confusion for people who switch platforms.
There was a problem hiding this comment.
Which key combos are we moving forward with? Alpha keys or function commands?
There was a problem hiding this comment.
@lukas-w thanks. @lookitsnicholas I don't have an investment either way. I'll check in #general on Discord just to make sure. I'll tag you in the post.
There was a problem hiding this comment.
Should we proceed with integrating this change into the master branch, then?
There was a problem hiding this comment.
Yes. At this point this will be going into master. @tresf
There was a problem hiding this comment.
A thought just crossed my mind... has anyone actually tested these particular key combos on macOS? I'm currently running a build that @tresf graciously compiled for me, but it uses Shift+Command to insert bars instead of the designated key combos suggested here.
There was a problem hiding this comment.
I've since started using a Mac keyboard and the lack of delete shortcut is a huge hindrance. This PR is very similar. Any small shortcut changes that fix an OS-specific bug are fine in stable. It's unfortunate that the OP has abandoned the PR so if the best we can do is land it in master, oh well.
There was a problem hiding this comment.
haha, I'm still here - also, Delete is Fn+Shift+Delete (pretty sure that we talked about this in the Discord chat); this PR just needs testing, which I can probably complete later today/tomorrow (wasn't able to setup my dev environment prior to macOS Mojave's GA)
|
Please use tabs for indentations instead of spaces. This is our coding convention. |
|
tabs were used, Xcode must encode them differently or smth
|
Each editor has its own preference when you hit the |
|
@lookitsnicholas I'd love to see this in |
|
Superseded by #4851. |
the Ins key is absent/non-functional on macOS devices, a change is necessary for this functionality to work
Edit by @tresf Closes #4522