GNOME Bugzilla – Bug 773405
PATCH: gsd-power-manager: Show kbd-brigthness osd on UPower.KbdBacklight.BrightnessChanged signal
Last modified: 2017-05-25 13:50:55 UTC
Created attachment 338322 [details] [review] [PATCH 5/5] gsd-power-manager: Show kbd-brigthness osd on UPower.KbdBacklight.BrightnessChanged signal If we receive a org.freedesktop.UPower.KbdBacklight.BrightnessChanged signal and the brightness is different then what we last set, show the kbd-brigthness osd. This makes the kbd-brigthness osd properly show up on laptops with hardwired keyboard backlight brightness hotkeys (which don't emit input events). Note this depends on the patches from bug 773402 and 773403, hence the [PATCH 5/5].
Review of attachment 338322 [details] [review]: ::: plugins/power/gsd-power-manager.c @@ +1488,3 @@ backlight_iface_emit_changed (manager, GSD_POWER_DBUS_INTERFACE_KEYBOARD, percentage); + + /* Brightness changed by firmware handled hotkey, show osd. */ OSD. Should be a couple of lines below, or reverse the check to make it clearer: if (manager->priv->shell_proxy != NULL) {... I really don't link this, as it splits the OSD between the media-keys and the power plugin, depending on whether the firmware or g-s-d handles the key. We need a better way than this.
Hi, Thanks for all the reviews, I'll go over them and post new versions. May take a while though as I'm rather busy (in the middle of switching teams). (In reply to Bastien Nocera from comment #1) > Review of attachment 338322 [details] [review] [review]: > > ::: plugins/power/gsd-power-manager.c > @@ +1488,3 @@ > backlight_iface_emit_changed (manager, > GSD_POWER_DBUS_INTERFACE_KEYBOARD, percentage); > + > + /* Brightness changed by firmware handled hotkey, show osd. */ > > OSD. Should be a couple of lines below, or reverse the check to make it > clearer: > if (manager->priv->shell_proxy != NULL) {... > > I really don't link this, as it splits the OSD between the media-keys and > the power plugin, depending on whether the firmware or g-s-d handles the > key. We need a better way than this. I already sort of expected this comment, but I decided to go with the KISS approach first. Since I did expect this I do have an alternative proposal: Make the media-keys plugin watch the Brightness property on org.gnome.SettingsDaemon.Power.Keyboard for changes and update the OSD based on this, this would work both for hardwired hotkeys, as well as for any changes done by the media-keys plugin itself as those will emit a PropertiesChanged signal for Brightness too, so this would then replace the show_osd() in gsd-media-keys-manager.c: update_brightness_cb() (for keyboards). This will need some careful handling of the initial PropertiesChanged signal the gsd-power-manager emits but I believe that can be dealt with. Please let me know what you think of this approach. If you like it I will take a shot at implementing it. Regards, Hans
(In reply to Hans de Goede from comment #2) > I already sort of expected this comment, but I decided to go with the KISS > approach first. Since I did expect this I do have an alternative proposal: > > Make the media-keys plugin watch the Brightness property on > org.gnome.SettingsDaemon.Power.Keyboard for changes and update the OSD based > on this, this would work both for hardwired hotkeys, as well as for any > changes done by the media-keys plugin itself as those will emit a > PropertiesChanged signal for Brightness too, so this would then replace the > show_osd() in gsd-media-keys-manager.c: update_brightness_cb() (for > keyboards). Hmm, I just realized that this will not work because this will also cause the osd to show when changing the brightness through the slider in gnome-control-panel. So I see 2 options: 1) Make org.gnome.SettingsDaemon.Power.Keyboard emit some new signal when the hardware autonomously changes the brightness, and make the media-keys plugin listen to this and show the osd 2) Move all keyboard backlight osd handling to gsd-power-manager.c I'm personally leaning towards 1. Regards, Hans
1) would work best for me as well.
Created attachment 344494 [details] [review] [PATCH 6/7] power: Add org.gnome.SettingsDaemon.Power.Keyboard.BrightnessChanged signal Thanks for the reviews I'm attaching a new set of patches, addressing all review remarks.
Created attachment 344495 [details] [review] [PATCH 7/7] media-keys: Show kbd-brigthness osd on org.gnome.SettingsDaemon.Power.Keyboard.BrightnessChanged signal
Review of attachment 344494 [details] [review]: s/Therefor/Therefore/ s/osd/OSD/ ::: plugins/power/gsd-power-manager.c @@ +1497,3 @@ + /* kbd_brightness_now != new_brightness, so the brightness was not + * changed by us, assume it was changed by a firmwware handle hotkey. */ I'm pretty sure this is racy.
Review of attachment 344495 [details] [review]: s/osd/OSD/ ::: plugins/media-keys/gsd-media-keys-manager.c @@ +3122,3 @@ + g_variant_get (parameters, "(is)", &brightness, &source); + + if (g_strcmp0 (source, "hotkey") != 0) You're leaking the source string here. Should be: const char *source; g_variant_get (parameters, "(i&s)", &brightness, &source); which doesn't make a copy of the string.
(In reply to Bastien Nocera from comment #7) > Review of attachment 344494 [details] [review] [review]: > > s/Therefor/Therefore/ > s/osd/OSD/ > > ::: plugins/power/gsd-power-manager.c > @@ +1497,3 @@ > > + /* kbd_brightness_now != new_brightness, so the brightness was not > + * changed by us, assume it was changed by a firmwware handle > hotkey. */ > > I'm pretty sure this is racy. This isn't racy for now, but it would be racy if we made those calls asynchronous. You'd also need to take "session_is_active" to avoid showing OSDs on inactive sessions.
(In reply to Bastien Nocera from comment #8) > Review of attachment 344495 [details] [review] [review]: > > s/osd/OSD/ > > ::: plugins/media-keys/gsd-media-keys-manager.c > @@ +3122,3 @@ > + g_variant_get (parameters, "(is)", &brightness, &source); > + > + if (g_strcmp0 (source, "hotkey") != 0) > > You're leaking the source string here. Should be: > const char *source; > g_variant_get (parameters, "(i&s)", &brightness, &source); > which doesn't make a copy of the string. Ok, will fix.
(In reply to Bastien Nocera from comment #9) > (In reply to Bastien Nocera from comment #7) > > Review of attachment 344494 [details] [review] [review] [review]: > > > > s/Therefor/Therefore/ > > s/osd/OSD/ > > > > ::: plugins/power/gsd-power-manager.c > > @@ +1497,3 @@ > > > > + /* kbd_brightness_now != new_brightness, so the brightness was not > > + * changed by us, assume it was changed by a firmwware handle > > hotkey. */ > > > > I'm pretty sure this is racy. > > This isn't racy for now, but it would be racy if we made those calls > asynchronous. Ack, so lets not make them asynchronous, if we do there is really nothing we can do to avoid the race. > You'd also need to take "session_is_active" to avoid showing OSDs on > inactive sessions. OK, will fix.
(In reply to Hans de Goede from comment #11) > (In reply to Bastien Nocera from comment #9) > > (In reply to Bastien Nocera from comment #7) > > > Review of attachment 344494 [details] [review] [review] [review] [review]: > > > > > > s/Therefor/Therefore/ > > > s/osd/OSD/ > > > > > > ::: plugins/power/gsd-power-manager.c > > > @@ +1497,3 @@ > > > > > > + /* kbd_brightness_now != new_brightness, so the brightness was not > > > + * changed by us, assume it was changed by a firmwware handle > > > hotkey. */ > > > > > > I'm pretty sure this is racy. > > > > This isn't racy for now, but it would be racy if we made those calls > > asynchronous. > > Ack, so lets not make them asynchronous, if we do there is really nothing we > can do to avoid the race. No, this is not an "if", this is a "when". We'll eventually make it asynchronous, so we need to build the framework to be able to handle that.
Created attachment 345355 [details] [review] [PATCH 2/3] power: Add org.gnome.SettingsDaemon.Power.Keyboard.BrightnessChanged signal Hi, Here is a new version addressing the review comments and using the new upower BrightnessChangedWithSource signal to address to race concerns. Regards, Hans
Created attachment 345357 [details] [review] [PATCH 3/3] media-keys: Show kbd-brigthness osd on org.gnome.SettingsDaemon.Power.Keyboard.BrightnessChanged signal
Created attachment 348071 [details] [review] [PATCH 2/3] power: Add org.gnome.SettingsDaemon.Power.Keyboard.BrightnessChanged signal Updated patch to match the upower changes as merged upstream.
Created attachment 348072 [details] [review] [PATCH 3/3] media-keys: Show kbd-brigthness osd on org.gnome.SettingsDaemon.Power.Keyboard.BrightnessChanged signal Updated patch to match the upower changes as merged upstream.
Ping? Anything blocking these from getting merged ? The upower and kernel patches are all in place so it would be nice to get this merged and the issues with keyboard brightness fixed.
*** Bug 672380 has been marked as a duplicate of this bug. ***
Review of attachment 348071 [details] [review]: looks fine
Review of attachment 348072 [details] [review]: lgtm