GNOME Bugzilla – Bug 773402
PATCH: gsd-power-manager: The Power.Keyboard.Brightness property is in percent
Last modified: 2017-01-30 11:21:55 UTC
Created attachment 338317 [details] [review] [PATCH 1/5] gsd-power-manager: s/upower_kdb_proxy/upower_kbd_proxy/ Fix get_other_property and 2 backlight_iface_emit_changed calls to properly report the brightness value in percent rather then reporting the abs value. This fixes the "Keyboard Brightness" slider in the control-center power panel not working on e.g. my XPS 15. Note the actual fix as described above is the 3th patch in the set of patches I'm attaching the other 2 (preparation) patches fix a typo and a small resource leak I noticed while working this.
Created attachment 338318 [details] [review] [PATCH 2/5] gsd-power-manager: deref upower_kbd_proxy on power_manager_stop()
Created attachment 338319 [details] [review] [PATCH 3/5] gsd-power-manager: The Power.Keyboard.Brightness property is in percent
Review of attachment 338317 [details] [review]: Looks good.
Review of attachment 338318 [details] [review]: > gsd-power-manager: deref upower_kbd_proxy on power_manager_stop() power: Free leaked UPower keyboard backlight proxy > Dereference upower_kbd_proxy on power_manager_stop(). Free. > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Please remove the SOB from all your patches though.
Review of attachment 338319 [details] [review]: > gsd-power-manager: The Power.Keyboard.Brightness property is in percent "power". Please also make sure to mention what is getting fixed in the commit subject. You're just summarising what's said below otherwise. > control-center power panel Power Settings panel, or gnome-control-center power panel. > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Please remove.
Thanks for the reviews I'm attaching a new set of patches, addressing all review remarks.
Created attachment 344489 [details] [review] [PATCH 1/7] power: s/upower_kdb_proxy/upower_kbd_proxy/
Created attachment 344490 [details] [review] [PATCH 2/7] power: Free leaked UPower keyboard backlight proxy
Created attachment 344491 [details] [review] [PATCH 3/7] power: The Power.Keyboard.Brightness property is in percent
Created attachment 344492 [details] [review] [PATCH 4/7] power: Make PERCENTAGE_TO_ABS round to the nearest value
Review of attachment 344489 [details] [review]: Great. Don't forget to add the bug URL at the bottom of the commit message.
Review of attachment 344489 [details] [review]: > power: s/upower_kdb_proxy/upower_kbd_proxy/ Please use the content of commit message body as the commit subject though.
Review of attachment 344490 [details] [review]: Note, a couple of further fixes if you fancy: - rename bus_cancellable to cancellable - use ->cancellable in those kbd proxy calls - use g_clear_object/_pointer when appropriate in _stop() Would be nice if you could at least file a bug about those. ::: plugins/power/gsd-power-manager.c @@ +2705,3 @@ g_clear_object (&manager->priv->idle_monitor); + g_clear_object (&manager->priv->upower_kbd_proxy); You can put this up on the line just below the g_clear_object() above.
Review of attachment 344491 [details] [review]: Sure.
Review of attachment 344492 [details] [review]: Don't forget the bug link in the commit message.
With the nits fixed
(In reply to Bastien Nocera from comment #13) > Review of attachment 344490 [details] [review] [review]: > > Note, a couple of further fixes if you fancy: > - rename bus_cancellable to cancellable > - use ->cancellable in those kbd proxy calls > - use g_clear_object/_pointer when appropriate in _stop() > > Would be nice if you could at least file a bug about those. I fixed those as well.