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

Add/fix missing keyboard shortcuts #6

Merged
merged 3 commits into from May 23, 2018
Merged

Conversation

brs17
Copy link
Contributor

@brs17 brs17 commented May 21, 2018

Should fix pop-os/pop#171
Fixed Super+m (Show application menu)
Note: Removed Super+m from toggling message tray (since Super+v already does that)
Fixed Super+d (Show desktop)
Fixed Super+f (Open files)

All of these should work now: http://pop.system76.com/docs/keyboard-shortcuts/

jackpot51
jackpot51 previously approved these changes May 21, 2018
cassidyjames
cassidyjames previously approved these changes May 21, 2018
Copy link
Contributor

@cassidyjames cassidyjames left a comment

Choose a reason for hiding this comment

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

Looks correct, but I don't know how to test this offhand.

@jackpot51 what does the :pop do in this file? Does that only apply the override to the pop session or something?

@jackpot51
Copy link
Member

Yep. The override is only added in the pop session.

@jackpot51
Copy link
Member

One potential issue - home is by default set to 'Home', as some keyboards has an actual key. This would probably break that key, but I don't have a keyboard to test. @brs17 can you check that, and @cassidyjames do you think this is acceptable?

@cassidyjames
Copy link
Contributor

@jackpot51 Ah yeah, I remember our ideal was being able to add both keys. If that's not supported, then I'm not sure. We should probably leave it working for the dedicated key…

@brs17
Copy link
Contributor Author

brs17 commented May 21, 2018

Good catch @jackpot51. I did write over the 'Home' key. Oddly the values for media keys are strings and not string arrays so I can't add a second option. Going to play with the custom shortcuts for media keys to enable both to work.

@brs17
Copy link
Contributor Author

brs17 commented May 21, 2018

So it looks like I can make a custom shortcuts by running:

gsettings set org.gnome.settings-daemon.plugins.media-keys.custom-keybinding:/org/gnome/settings-daemon/plugins/media-keys/custom-keybindings/Files/ name "Files"

gsettings set org.gnome.settings-daemon.plugins.media-keys.custom-keybinding:/org/gnome/settings-daemon/plugins/media-keys/custom-keybindings/Files/ command "nautilus -w"

gsettings set org.gnome.settings-daemon.plugins.media-keys.custom-keybinding:/org/gnome/settings-daemon/plugins/media-keys/custom-keybindings/Files/ binding "<Super>f"

gsettings set org.gnome.settings-daemon.plugins.media-keys.custom-keybindings "['/org/gnome/settings-daemon/plugins/media-keys/custom-keybindings/Files/']"

So I would do that and remove the changes I made to pop-session.gsettings-override for this shortcut.

Now I just need to figure out where to put that.

@brs17
Copy link
Contributor Author

brs17 commented May 21, 2018

Ah, I probably should update the changelog as well, shouldn't I?!

@brs17 brs17 dismissed stale reviews from cassidyjames and jackpot51 via 977a1c3 May 22, 2018 21:05
@brs17 brs17 changed the title Add/fix missing keyboard shortcuts WIP, DON'T MERGE: Add/fix missing keyboard shortcuts May 22, 2018
@jackpot51
Copy link
Member

I don't think the custom shortcut is going to work

@brs17
Copy link
Contributor Author

brs17 commented May 22, 2018

It doesn't :(

@brs17
Copy link
Contributor Author

brs17 commented May 22, 2018

Alright so I've reverted all of the Files shortcut code.

Discussing offline with @jackpot51 and @cassidyjames , we are going to remove Super+f (expected) functionality to let the file browser button work on certain external keyboards.

I am planning to file a bug upstream that the current gsetting for opening the file browser(and all media keys) should have a key value type of string array not string so that multiple values could be provided.

jackpot51
jackpot51 previously approved these changes May 22, 2018
@WatchMkr
Copy link

Something came to mind reading this. Is it more common to have a Files/Home button on a keyboard or not? It's probably more common not to have those keys and that's certainly the case out of the percentage of keyboards we ship. If that's the case, is it better to set Super+F for files and a customer can use Keyboards shortcuts if they have the key and want to set it? It's a pretty obvious setting.

@brs17
Copy link
Contributor Author

brs17 commented May 23, 2018

That's a good point.

My first thought that comes to mind is that by doing this we would be actively breaking functionality for people who do have a keyboard with it (the Amazon Basics keyboard does have the key). Seeing that this functionality has not worked in the lifetime of Pop!_OS my thought is we shouldn't now rush to do something that breaks functionality.

I think if we can get upstream to agree to accept multiple shortcuts for multimedia keys, that'd be the best way to implement both. Otherwise if we really want this feature, doing it ourselves would also be an option.

@brs17 brs17 requested a review from jackpot51 May 23, 2018 14:15
@brs17 brs17 changed the title WIP, DON'T MERGE: Add/fix missing keyboard shortcuts Add/fix missing keyboard shortcuts May 23, 2018
@jackpot51
Copy link
Member

Merge when ready @brs17

@brs17 brs17 merged commit c1e7b63 into master_bionic May 23, 2018
@brs17 brs17 deleted the keyboard-shortcuts branch May 23, 2018 20:14
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

4 participants