GNOME Bugzilla – Bug 698754
power: Use a DBus property for screen brightness
Last modified: 2013-06-23 00:17:56 UTC
See patch. This will be used in gnome-shell to implement the brightness slider.
Created attachment 242336 [details] [review] power: Use a DBus property for screen brightness This is a much easier interface for clients to manage.
Get/SetPercentage means that we don't need to make sure the brightness is synced with its actual value. The explicit API also ensure that errors are returned when getting or settings fails. I'm not sure it's that big a problem, but those should be mentioned in the commit log. You'd also need to create more patches, g-s-d's color plugin uses SetPercentage, so does the Power panel in g-c-c.
(In reply to comment #2) > Get/SetPercentage means that we don't need to make sure the brightness is > synced with its actual value. Doesn't this mean that the slider in g-c-c (or in the new g-s designs) will get out of sync with the real brightness in some cases, then? > The explicit API also ensure that errors are > returned when getting or settings fails. I'm not sure it's that big a problem, > but those should be mentioned in the commit log. OK, will do. > You'd also need to create more patches, g-s-d's color plugin uses > SetPercentage, so does the Power panel in g-c-c. g-c-c patch is in bug #698757. Will work on the color plugin; didn't know it used the DBus API.
Created attachment 243702 [details] [review] power: Use a DBus property for screen brightness A property is easier for clients to manage, as they'll automatically get new PropertiesChanged signals when the value changes, and with GDBus bindings, properties are cached field lookups that don't require asynchronous access. The downside is that we cannot report any errors when getting the brightness. For most user interface designs, however, this is insignificant. Also adapt the color plugin to the new interface.
Review of attachment 243702 [details] [review]: ::: plugins/color/gsd-color-manager.c @@ +656,3 @@ + "org.freedesktop.DBus.Properties", + "Set", + g_variant_new_parsed ("('" GSD_DBUS_INTERFACE_POWER_SCREEN "'," That is quite ugly, any way to make this cleaner? ::: plugins/power/gsd-power-manager.c @@ +2329,3 @@ return; + + value = backlight_get_percentage (manager->priv->rr_screen, NULL); You're re-reading the value you've probably just set. That shouldn't be necessary. @@ +3899,3 @@ + /* Check session pointer as a proxy for whether the manager is in the + start or stop state */ + if (manager->priv->session == NULL) { You already check that in handle_get_property(). @@ +3906,3 @@ + guint32 value; + value = backlight_get_percentage (manager->priv->rr_screen, NULL); + retval = g_variant_new_uint32 (value); How does it return the lack of backlight devices to the caller for example? Magic number? Return an error?
Created attachment 246433 [details] [review] power: Use a DBus property for screen brightness A property is easier for clients to manage, as they'll automatically get new PropertiesChanged signals when the value changes, and with GDBus bindings, properties are cached field lookups that don't require asynchronous access. The downside is that we cannot report any errors when getting the brightness. For most user interface designs, however, this is insignificant. Also adapt the color plugin to the new interface. Review notes taken into account. Property reports -1 on error. Suggestions for better cleanups welcome.
Review of attachment 246433 [details] [review]: (In reply to comment #6) <snip> > The downside is that we cannot report any errors when getting the > brightness. <snip> > Review notes taken into account. Property reports -1 on error. The commit message needs updating. Nearly there! ::: plugins/power/gsd-power-manager.c @@ +107,2 @@ " <method name='StepUp'>" " <arg type='u' name='new_percentage' direction='out'/>" StepUp/StepDown will need to use "i" as well, so they can report the new negative value on error, and the media-keys plugin should be fixed for the new API. @@ +3864,2 @@ static GVariant * +handle_get_property_main (GsdPowerManager *manager, I'd have split the code move from handle_get_property() to this new function.
Created attachment 246674 [details] [review] power: Split out get_property We want to add properties on the Screen iface, so remove this.
Created attachment 246675 [details] [review] power: Use a DBus property for screen brightness A property is easier for clients to manage, as they'll automatically get new PropertiesChanged signals when the value changes, and with GDBus bindings, properties are cached field lookups that don't require asynchronous access. If there are any errors when fetching the new brightness, the value -1 is returned, so the interface has been modified slightly to support signed values. Also adapt the color and media-keys plugins to the new interface.
Review of attachment 246674 [details] [review]: Looks good.
Review of attachment 246675 [details] [review]: Looks good. ::: plugins/color/gsd-color-manager.c @@ +654,3 @@ GSD_DBUS_NAME_POWER, GSD_DBUS_PATH_POWER, + "org.freedesktop.DBus.Properties", Can you file a new bug against the color plugin to check the new value of Brightness after the call has finished to check whether the call succeeded? There's good chances we want to have an error here.
Review of attachment 246675 [details] [review]: ::: plugins/color/gsd-color-manager.c @@ +654,3 @@ GSD_DBUS_NAME_POWER, GSD_DBUS_PATH_POWER, + "org.freedesktop.DBus.Properties", I don't understand. The old callback just emit a warning. I figured that wasn't necessary.
(In reply to comment #12) > Review of attachment 246675 [details] [review]: > > ::: plugins/color/gsd-color-manager.c > @@ +654,3 @@ > GSD_DBUS_NAME_POWER, > GSD_DBUS_PATH_POWER, > + "org.freedesktop.DBus.Properties", > > I don't understand. The old callback just emit a warning. I figured that wasn't > necessary. Then we should at least emit the warning. That's helpful to developers (meaning, Richard) to debug this problems with the color plugin.
Attachment 246674 [details] pushed as 9763448 - power: Split out get_property Attachment 246675 [details] pushed as a4c3299 - power: Use a DBus property for screen brightness