Skip to content

Add cmd+h for hide and ctrl+space for pause/resume#108

Merged
jpochyla merged 2 commits intojpochyla:masterfrom
JuliDi:add-more-hotkeys
Aug 18, 2021
Merged

Add cmd+h for hide and ctrl+space for pause/resume#108
jpochyla merged 2 commits intojpochyla:masterfrom
JuliDi:add-more-hotkeys

Conversation

@JuliDi
Copy link
Copy Markdown
Contributor

@JuliDi JuliDi commented Aug 18, 2021

This should fix #91 in parts, but has a little drawback: you need to press ctrl+space to pause/resume the song.
I could not get the application to react to a normal key even without a modifier (i.e. only pressing space or another key when the psst window is open).
If there is a way to achieve this, I'd be happy to make the according changes.

In case #103 should be implemented (ctrl+w sends druid's command::CLOSE_WINDOW), I could add that as well.

@jpochyla
Copy link
Copy Markdown
Owner

Hi, thanks for the contribution! While the Cmd+H shortcut is definitely Mac-specific, Ctrl+Space won't work on other platforms, as we omit the application menu on Windows and Linux. Cross-platform keyboard shortcuts would need to be implemented in a custom controller, handling them in one place, or separately in the specific controllers (i.e. pause/resume in the PlaybackController).

@JuliDi
Copy link
Copy Markdown
Contributor Author

JuliDi commented Aug 18, 2021

The Ctrl+Space shortcut should not be in the application menu, I've added it to the edit menu. Isn't that visible on all platforms?
See

if cfg!(target_os = "macos") {
Menu::empty().entry(mac_app_menu())
} else {
Menu::empty()
}
.entry(edit_menu())
.entry(view_menu())

What do you think about adding a "Playback" menu (like the spotify app) where all those controls are collected and assigned a shortcut? Maybe that'd be a better solution.

(i.e. pause/resume in the PlaybackController)

Could you point me to where that would go in the playback controller? I played around with keydown events in there yesterday but couldn't get anything to work.

@jpochyla
Copy link
Copy Markdown
Owner

Oh, sorry, that condition in menu.rs is a bit unnecessary, because of:

if cfg!(target_os = "macos") {
win.menu(menu::main_menu)
} else {
win
}

Could you point me to where that would go in the playback controller?

It would go to the Controller::event method, after the command matching stuff. This might be too cryptic and I will definitely get to it myself very soon, so feel free to omit this from the PR.

@JuliDi
Copy link
Copy Markdown
Contributor Author

JuliDi commented Aug 18, 2021

Alright, thanks for your help.

I have reverted the changes regarding pause/resume and left only the cmd+h part.

It would go to the Controller::event method, after the command matching stuff. This might be too cryptic and I will definitely get to it myself very soon, so feel free to omit this from the PR.

Okay, I will have a look and see whether I can make it work, and open a new PR if it does.

@jpochyla
Copy link
Copy Markdown
Owner

Thank you! ❤️

@jpochyla jpochyla merged commit 4c9bbed into jpochyla:master Aug 18, 2021
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.

Addition of keyboard controls

2 participants