After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 711682 - Keybindings capture bugs
Keybindings capture bugs
Status: RESOLVED FIXED
Product: gnome-shell
Classification: Core
Component: general
3.8.x
Other Linux
: Normal normal
: ---
Assigned To: gnome-shell-maint
gnome-shell-maint
Depends on:
Blocks:
 
 
Reported: 2013-11-08 13:25 UTC by Bastien Nocera
Modified: 2014-12-19 13:14 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
ScreenShield: remove obsolete comment and hack (1.70 KB, patch)
2014-07-31 14:46 UTC, Giovanni Campagna
committed Details | Review
shellDBus: Add mode parameter to AcceleratorActivated signal (1.53 KB, patch)
2014-11-12 19:59 UTC, Florian Müllner
committed Details | Review
shellDBus: Change AcceleratorActivated signature (1.87 KB, patch)
2014-11-12 20:28 UTC, Florian Müllner
committed Details | Review
shellDBus: Add mode parameter to AcceleratorActivated signal (1.30 KB, patch)
2014-11-12 20:29 UTC, Florian Müllner
committed Details | Review

Description Bastien Nocera 2013-11-08 13:25:33 UTC
In bug 707095, we created new keybindings that behave slightly differently for when in the lock/unlock screens, and allowed the power key keybinding to be used in the login screen.

However, that brought up 2 bugs in gnome-shell:
- you can't capture the same keybinding for 2 different modes (so g-s-d would know that it's in the lock screen and make sure not to show polkit or interactive dialogues in the lock screen)
- when in gdm, the keybinding mode is LOCK_SCREEN instead of LOGIN_SCREEN

When those 2 bugs are fixed, it should be possible to shutdown the machine by pressing the power button in the lock screen after having changed the button's behaviour:
gsettings set org.gnome.settings-daemon.plugins.power button-power "shutdown"

And ditto in gdm:
# cat >/etc/dconf/db/gdm.d/01-power <<EOF
[org/gnome/settings-daemon/plugins/power]
button-power="shutdown"
EOF
# dconf update
Comment 1 Scott Reeves 2014-07-15 00:16:39 UTC
Hi,

Poking at this bug on a 3.10 box and it appears to still be a problem at the gdm screen. Do you know if there was any progress on this - glancing through the code even on master branch I don't see anything but I could be overlooking it.
Comment 2 Giovanni Campagna 2014-07-31 14:46:55 UTC
Created attachment 282149 [details] [review]
ScreenShield: remove obsolete comment and hack

We don't need to wait to until the stage window is mapped to take
the modal grab, because that code now runs in a startup-prepared
signal handler, which in turn runs some time after the mainloop
has started and well after the stage window is mapped.
Comment 3 Jasper St. Pierre (not reading bugmail) 2014-07-31 14:50:03 UTC
Review of attachment 282149 [details] [review]:

OK.
Comment 4 Giovanni Campagna 2014-07-31 14:54:45 UTC
Attachment 282149 [details] pushed as cd4eda8 - ScreenShield: remove obsolete comment and hack
Comment 5 Bastien Nocera 2014-08-05 10:53:20 UTC
Are you sure this fixes both the bugs I brought up?
Comment 6 Florian Müllner 2014-08-05 11:07:31 UTC
No, keybindings can still only be grabbed once, independent from the mode flags. But maybe it is easier anyway to just handle it differently according to the "Mode" property on org.gnome.Shell?
Comment 7 Bastien Nocera 2014-08-05 12:27:01 UTC
(In reply to comment #6)
> No, keybindings can still only be grabbed once, independent from the mode
> flags. But maybe it is easier anyway to just handle it differently according to
> the "Mode" property on org.gnome.Shell?

Not that much easier. gnome-shell already allows us to capture keys with a particular mode, I would expect each of the captures to be separate, each of those with its own ID.

Also, using this command, I don't see the mode property changing when locking the screen for example:
gdbus monitor  --session --dest org.gnome.Shell

Is it only expected to switch between user and login?
Comment 8 Florian Müllner 2014-08-05 12:31:35 UTC
(In reply to comment #7)
> Also, using this command, I don't see the mode property changing when locking
> the screen for example:
> gdbus monitor  --session --dest org.gnome.Shell

Right, turns out the property shows the mode Shell was started in[0], not the currently active one.

[0] https://git.gnome.org/browse/gnome-shell/tree/js/ui/shellDBus.js#n236
Comment 9 Bastien Nocera 2014-11-10 18:49:37 UTC
(In reply to comment #8)
> (In reply to comment #7)
> > Also, using this command, I don't see the mode property changing when locking
> > the screen for example:
> > gdbus monitor  --session --dest org.gnome.Shell
> 
> Right, turns out the property shows the mode Shell was started in[0], not the
> currently active one.
> 
> [0] https://git.gnome.org/browse/gnome-shell/tree/js/ui/shellDBus.js#n236

Adding a Mode flag to the AcceleratorActivated signal would fix it. Should we do that?
Comment 10 Florian Müllner 2014-11-10 19:04:21 UTC
(In reply to comment #9)
> Adding a Mode flag to the AcceleratorActivated signal would fix it. Should we
> do that?

That would be very non-intrusive on the mutter/shell side, so if you're fine with this on the gsd side I'm all for it.
Comment 11 Bastien Nocera 2014-11-12 10:08:50 UTC
(In reply to comment #10)
> (In reply to comment #9)
> > Adding a Mode flag to the AcceleratorActivated signal would fix it. Should we
> > do that?
> 
> That would be very non-intrusive on the mutter/shell side, so if you're fine
> with this on the gsd side I'm all for it.

Can you look into that? I can take care of the gnome-settings-daemon changes.
Comment 12 Florian Müllner 2014-11-12 19:59:18 UTC
Created attachment 290544 [details] [review]
shellDBus: Add mode parameter to AcceleratorActivated signal

This will allow g-s-d to handle actions differently based on the
current mode - namely, allow the power button when locked, but
make sure to never show any dialogs in that case.
Comment 13 Florian Müllner 2014-11-12 20:02:00 UTC
I wonder if we should change the signal signature to 'ua{sv}' instead, so we can extend it more easily without requiring updating g-s-d and g-s in lock step.
Comment 14 Bastien Nocera 2014-11-12 20:08:38 UTC
(In reply to comment #13)
> I wonder if we should change the signal signature to 'ua{sv}' instead, so we
> can extend it more easily without requiring updating g-s-d and g-s in lock
> step.

Would be great, so we only break it once...
Comment 15 Jasper St. Pierre (not reading bugmail) 2014-11-12 20:10:29 UTC
Meh. You're still breaking API by having different keys in the a{sv}. What happens if the key is missing? I'd rather just have an API break when the API actually breaks.
Comment 16 Florian Müllner 2014-11-12 20:21:26 UTC
(In reply to comment #15)
> What happens if the key is missing?

This obviously assumes that g-s-d deals gracefully with this case.
Comment 17 Florian Müllner 2014-11-12 20:28:24 UTC
Created attachment 290545 [details] [review]
shellDBus: Change AcceleratorActivated signature

Adding new parameters to the signal currently will break keybindings
until gnome-settings-daemon is updated to the new API as well.
Put additional parameters into a dictionary instead to make future
extensions easier.

For what it's worth, here's a patch for using a dictionary
Comment 18 Florian Müllner 2014-11-12 20:29:11 UTC
Created attachment 290546 [details] [review]
shellDBus: Add mode parameter to AcceleratorActivated signal

Updated attachment 290544 [details] [review] for the dictionary change (not obsoleting the existing patch until we decide on the approach).
Comment 19 Florian Müllner 2014-12-19 13:14:10 UTC
Attachment 290544 [details] pushed as 6803528 - shellDBus: Add mode parameter to AcceleratorActivated signal
Attachment 290545 [details] pushed as 2aa4fb0 - shellDBus: Change AcceleratorActivated signature
Attachment 290546 [details] pushed as 6803528 - shellDBus: Add mode parameter to AcceleratorActivated signal