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.

Sign up for GitHub

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

Change universal key shortcut to insert bars #4526

Closed
wants to merge 4 commits into from
Closed

Change universal key shortcut to insert bars #4526

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
21eb6d0
Update SongEditor.cpp
Aug 6, 2018
cf55c45
Update SongEditor.cpp
Aug 6, 2018
81770a9
Update SongEditor.cpp
Aug 6, 2018
b72c781
Update SongEditor.cpp
Aug 6, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion src/gui/editors/SongEditor.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -318,7 +318,14 @@ void SongEditor::keyPressEvent( QKeyEvent * ke )
ke->key() == Qt::Key_Insert )
{
m_song->insertBar();
}
#ifdef Q_OS_MACOS
if( /*_ke->modifiers() & Qt::ShiftModifier*/
gui->mainWindow()->isShiftPressed() == true &&
ke->key() == Qt::Key_Enter || ke->key() == Qt::Key_Return )
{
m_song->insertBar();
}
#endif
Copy link
Member

@tresf tresf Aug 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I think you accidentally removed a curly bracket here.
  2. We use LMMS_BUILD_APPLE as our precompiler directive for MacOS.
  3. You should group the || logic with parens to be explicit.
  4. @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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@lookitsnicholas lookitsnicholas Aug 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which key combos are we moving forward with? Alpha keys or function commands?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we proceed with integrating this change into the master branch, then?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. At this point this will be going into master. @tresf

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@tresf tresf Dec 31, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@lookitsnicholas lookitsnicholas Dec 31, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

else if(/* _ke->modifiers() & Qt::ShiftModifier &&*/
gui->mainWindow()->isShiftPressed() == true &&
ke->key() == Qt::Key_Delete )
Expand Down