GNOME Bugzilla – Bug 652551
[patch] add button handling code from gnome-power-manager
Last modified: 2011-07-27 12:01:04 UTC
Created attachment 189894 [details] [review] test patch Initial patch, please review. I've decided to split up the uber-patch in #649470 as we had quite a lot of success with the color plugin recently, and it's certainly more manageable from my point of view. As the functionality gets added to g-s-d in dribs and drabs, I'll remove the functionality from g-p-m in master, so we can do unstable tarball releases without breaking things. This will be the first patch of many in the next few weeks :) Thanks, Richard.
Review of attachment 189894 [details] [review]: ::: plugins/media-keys/acme.h @@ +135,3 @@ + { SUSPEND_KEY, NULL, "XF86Sleep", NULL }, + { HIBERNATE_KEY, NULL, "XF86Hibernate", NULL }, + { LID_OPEN_KEY, NULL, NULL, NULL }, No. @@ +136,3 @@ + { HIBERNATE_KEY, NULL, "XF86Hibernate", NULL }, + { LID_OPEN_KEY, NULL, NULL, NULL }, + { LID_CLOSED_KEY, NULL, NULL, NULL }, No. Either it has a GSettings key associated, or it's hard-coded. I don't want to see handling of "fake keys" in the media-keys plugin. It's complicated enough handling actual keys.
Created attachment 189902 [details] [review] test patch (In reply to comment #1) > I don't want to see handling of "fake keys" in the media-keys plugin. It's > complicated enough handling actual keys. Fair enough, I can deal with this in the power plugin. New patch attached. Thanks.
Review of attachment 189902 [details] [review]: ::: plugins/media-keys/gsd-media-keys-manager.c @@ +1172,3 @@ + ret = up_client_suspend_sync (manager->priv->up_client, + NULL, &error); + if (!ret) { Can't the error checking be factored? @@ +1179,3 @@ + break; + case GSD_POWER_ACTION_SHUTDOWN: + /* TODO: shutdown the machine using ConsoleKit */; Where's the code to do that? @@ +1192,3 @@ + case GSD_POWER_ACTION_BLANK: + case GSD_POWER_ACTION_NOTHING: + case GSD_POWER_ACTION_INTERACTIVE: ? @@ +1294,3 @@ + /* call into the power plugin */ + g_dbus_proxy_call (manager->priv->power_screen_proxy, + action == GSD_BRIGHTNESS_ACTION_INCREMENT ? "StepUp" : "StepDown", Why do we need to define new "actions" when we could use the keys enum instead?
Created attachment 189987 [details] [review] test patch (In reply to comment #3) > Review of attachment 189902 [details] [review]: > > ::: plugins/media-keys/gsd-media-keys-manager.c > @@ +1172,3 @@ > + ret = up_client_suspend_sync (manager->priv->up_client, > + NULL, &error); > + if (!ret) { > > Can't the error checking be factored? Well, it could, but it's three lines of code... > @@ +1179,3 @@ > + break; > + case GSD_POWER_ACTION_SHUTDOWN: > + /* TODO: shutdown the machine using ConsoleKit */; > > Where's the code to do that? Added. > Why do we need to define new "actions" when we could use the keys enum instead? Good point. New patch attached. Thanks.
Review of attachment 189987 [details] [review]: ::: plugins/media-keys/Makefile.am @@ +94,3 @@ $(PLUGIN_CFLAGS) \ $(SETTINGS_PLUGIN_CFLAGS) \ + $(UPOWER_CFLAGS) \ Where's the configure.ac changes for that? ::: plugins/media-keys/acme.h @@ +138,3 @@ + { KEYBOARD_BRIGHTNESS_DOWN_KEY, NULL, "XF86KbdBrightnessDown", NULL }, + { KEYBOARD_BRIGHTNESS_TOGGLE_KEY, NULL, "XF86KbdLightOnOff", NULL }, + { LOCK_KEY, NULL, "XF86ScreenSaver", NULL }, SCREENSAVER2_KEY maybe? We already have a non-hardcoded key which does the exact same thing. I'd like it if I didn't need to think too hard about what each key does (same goes for Power/Sleep/Suspend/Hibernate, I'm pretty sure we have some non-hardcoded keys with the same semantics). ::: plugins/media-keys/gsd-media-keys-manager.c @@ +1245,3 @@ + case GSD_POWER_ACTION_BLANK: + case GSD_POWER_ACTION_NOTHING: + case GSD_POWER_ACTION_INTERACTIVE: Shouldn't interactive do something then? @@ +1274,3 @@ + action_type = g_settings_get_enum (manager->priv->power_settings, + "button-suspend"); + do_power_action_type (manager, action_type); do_sleep_action, do_suspend_action and do_power_action are the exact same function with a different key passed to GSettings.
Created attachment 190137 [details] [review] new patch
Created attachment 190138 [details] [review] new patch
Created attachment 190139 [details] [review] new patch Three more patches to review please. Thanks.
Review of attachment 190138 [details] [review]: ::: plugins/media-keys/gsd-media-keys-manager.c @@ +1289,3 @@ + g_warning ("No existing D-Bus connection trying to handle power keys"); + return; + } The code seems a little inconsistent wrt to whether it is needed handle connection == NULL or not (see do_xrandr_action vs gsd_media_key_pressed). Should probably be cleaned up, but not part of this patch. @@ +1468,3 @@ break; + case POWER_KEY: + do_config_power_action (manager, "button-power"); This seems a little wierd: You have a switch here that translated an enum key to a string that gets passed to do_config_power_action()...and if you look in that function, you translate the string back to _another_ enum and switch again. Could be straightened out, maybe. @@ +1490,3 @@ + case BATTERY_KEY: + /* don't actually do anything here until we've agreed + * something sane with the designers */ Bug reference, or url ?
Review of attachment 190139 [details] [review]: ::: plugins/power/gsd-power-manager.c @@ +216,3 @@ + tmp = up_client_get_lid_is_closed (manager->priv->up_client); + if (manager->priv->lid_is_closed == tmp) + return; How does lid_is_closed get initialized ? This looks to me like it has a 50% chance of triggering a random lid open/close the first time the up client emits a changed signal.
Comment on attachment 190137 [details] [review] new patch As long as it doesn't crash anything, while waiting for the rest of the patches.
Created attachment 190421 [details] [review] media keys patch New media keys patch. We have to do one lot of mapping from keys to actions as some keys are configurable in the power control center panel.
Created attachment 190422 [details] [review] add initial power plugin new patch addressing review. I've left in interactive, otherwise a lot of the enum logic gets messed up with the existing power panel and the gconf->gsettings conversion.
Created attachment 190423 [details] [review] register two interfaces This adds the interfaces for the media-keys plugin to use. the g_debug()s will be turned into real actions in the next few patches.
Created attachment 190424 [details] [review] keyboard backlight support Initial key backlight support (untested, as I don't have the hardware, but logic copied from known working g-p-m)
Review of attachment 190421 [details] [review]: Looks good with the few changes mentioned. ::: plugins/media-keys/acme.h @@ +75,3 @@ + KEYBOARD_BRIGHTNESS_DOWN_KEY, + KEYBOARD_BRIGHTNESS_TOGGLE_KEY, + SCREEN_BRIGHTNESS_UP_KEY, Put it next to the screensaver key? @@ +138,3 @@ + { KEYBOARD_BRIGHTNESS_DOWN_KEY, NULL, "XF86KbdBrightnessDown", NULL }, + { KEYBOARD_BRIGHTNESS_TOGGLE_KEY, NULL, "XF86KbdLightOnOff", NULL }, + { HIBERNATE_KEY, NULL, "XF86Hibernate", NULL }, Ditto. ::: plugins/media-keys/gsd-media-keys-manager.c @@ +1246,3 @@ + } + break; + g_dbus_proxy_call (proxy, Is this only to shut up gcc? If they really can't happen, add a default: case with a g_assert_not_reached().
Review of attachment 190424 [details] [review]: Looks fine. ::: plugins/power/gsd-power-manager.c @@ +471,3 @@ +/* on ACPI machines we have 4-16 levels, on others it's ~150 */ +static guint + -1, #define STEP_AMOUNT(max) (max < 20 ? 1 : max / 20)
Review of attachment 190422 [details] [review]: Looks good.
Review of attachment 190423 [details] [review]: Looks fine.
Please make extra sure that all the "in-flight" D-Bus calls would be appropriately cancelled when gnome-settings-daemon shuts down, or plugins are deactivated. It looks slack otherwise.
Created attachment 190425 [details] [review] add udisks interface code from gnome-power-manager Another chunk ported from g-p-m...
Review of attachment 190425 [details] [review]: I really don't like this. Either it's a feature to minimise the noise made, in which case it's incomplete. Or it's something to maximise the disk's lifespan. It doesn't know what it is, and doesn't serve any of the purposes adequately. Needs a use case, and some very good reason why leaving it up to the user to choose is a good idea.
(In reply to comment #22) > Review of attachment 190425 [details] [review]: > > I really don't like this. Fair enough. The patch is in bugzilla if anyone wants to revive it later. I've dropped this from my branch now.
Created attachment 190507 [details] [review] Add an internal error quark for future code Trivial, just to unload more code.
Created attachment 190565 [details] [review] Add laptop backlight panel hotkey control Initial patch for review; you need gnome-desktop from git master to be able to build this. Thanks.
Created attachment 190593 [details] [review] actually return a percentage from the various set methods for the media keys plugin
Created attachment 190595 [details] [review] add interface and common code needed for gnome-shell This won't actually do anything useful yet, I'm currently yak shaving the engine that actually populates the data. This is to unload yet another chunk of patch with all the string stuff checked. Thanks.
Review of attachment 190565 [details] [review]: Just one bug questions. Also, you'll need to up the gnome-desktop dep. ::: plugins/power/gsd-power-manager.c @@ +567,3 @@ + GnomeRROutput *output; + + /* get the laptop screen only */ Is this really what you want to do? That doesn't catch screens on single screen desktop setups...
Review of attachment 190593 [details] [review]: ::: plugins/power/gsd-power-manager.c @@ +59,3 @@ " <interface name='org.gnome.SettingsDaemon.Power.Screen'>" +" <method name='StepUp'>" +" <arg type=\"u\" name=\"new_percentage\" direction=\"out\"/>" Use single quotes, instead of over escaping.
Review of attachment 190595 [details] [review]: I don't like this. Is there any reason why that code can't be directly in gnome-shell, just like the sound status code, and transformed into a small helper library, like the gnome-bluetooth status code.
(In reply to comment #28) > Review of attachment 190565 [details] [review]: > > Just one bug questions. Also, you'll need to up the gnome-desktop dep. Fixed, thanks. > ::: plugins/power/gsd-power-manager.c > @@ +567,3 @@ > + GnomeRROutput *output; > + > + /* get the laptop screen only */ > > Is this really what you want to do? That doesn't catch screens on single screen > desktop setups... I asked the designers, and the consensus was that we should only control the internal laptop panel. If one day X develops the ability to set the brightness using a thread and i2c, we can revisit this and maybe set the brightness of the monitor that the cursor is on, or something sensible. I think it's highly unlikely X is going to be able to set non-internal-acpi panel brightness any time soon. I can make the code fall back to the primary monitor and try to set that brightness if you'd rather. (In reply to comment #29) > Review of attachment 190593 [details] [review]: > Use single quotes, instead of over escaping. Fixed, thanks. (In reply to comment #30) > Review of attachment 190595 [details] [review]: > I don't like this. Is there any reason why that code can't be directly in > gnome-shell, just like the sound status code, and transformed into a small > helper library, like the gnome-bluetooth status code. Well, this is maybe my fault for adding the interface before the code backing it. The code that backs it (the old GpmEngine code) does some pretty clever things like merging the various laptop battery devices into one virtual device suitable to be used for policy decisions and the icon gnome-shell. (It also does things like creating the virtual devices after talking to gnome-phone-manager) I don't think it makes to duplicate all that logic in the shell, nor do I think we need yet-another-helper-library that would need GIR'ization for javascript. By letting gnome-shell call directly into the plugin we leave all the code in the right place (in my opinion) and we minimize duplication. Plus, it's what's already implemented in gnome-shell, and for 3.2 I wanted to keep the number of changed parts to a minimum for my debugging sanity.
> > ::: plugins/power/gsd-power-manager.c > > @@ +567,3 @@ > > + GnomeRROutput *output; > > + > > + /* get the laptop screen only */ > > > > Is this really what you want to do? That doesn't catch screens on single screen > > desktop setups... > > I asked the designers, and the consensus was that we should only control the > internal laptop panel. If one day X develops the ability to set the brightness > using a thread and i2c, we can revisit this and maybe set the brightness of the > monitor that the cursor is on, or something sensible. I think it's highly > unlikely X is going to be able to set non-internal-acpi panel brightness any > time soon. I can make the code fall back to the primary monitor and try to set > that brightness if you'd rather. Whatever you choose, but actually document it. > (In reply to comment #30) > > Review of attachment 190595 [details] [review] [details]: > > I don't like this. Is there any reason why that code can't be directly in > > gnome-shell, just like the sound status code, and transformed into a small > > helper library, like the gnome-bluetooth status code. > > Well, this is maybe my fault for adding the interface before the code backing > it. The code that backs it (the old GpmEngine code) does some pretty clever > things like merging the various laptop battery devices into one virtual device > suitable to be used for policy decisions and the icon gnome-shell. (It also > does things like creating the virtual devices after talking to > gnome-phone-manager) But is that code used anywhere outside the dbus code to answer queries from gnome-shell or the stand-alone power-manager status icon (and where will that one live?). > I don't think it makes to duplicate all that logic in the shell, nor do I think > we need yet-another-helper-library that would need GIR'ization for javascript. > By letting gnome-shell call directly into the plugin we leave all the code in > the right place (in my opinion) and we minimize duplication. Except that it's really not clear to me whether that code is used by anything else or not. > Plus, it's what's already implemented in gnome-shell, and for 3.2 I wanted to > keep the number of changed parts to a minimum for my debugging sanity. But you'll need to reimplement the status icon separately though, talking over dbus.
(In reply to comment #32) > > (In reply to comment #30) > > > Review of attachment 190595 [details] [review] [details] [details]: > > > I don't like this. Is there any reason why that code can't be directly in > > > gnome-shell, just like the sound status code, and transformed into a small > > > helper library, like the gnome-bluetooth status code. > > > > Well, this is maybe my fault for adding the interface before the code backing > > it. The code that backs it (the old GpmEngine code) does some pretty clever > > things like merging the various laptop battery devices into one virtual device > > suitable to be used for policy decisions and the icon gnome-shell. (It also > > does things like creating the virtual devices after talking to > > gnome-phone-manager) > > But is that code used anywhere outside the dbus code to answer queries from > gnome-shell or the stand-alone power-manager status icon (and where will that > one live?). It's going to be used by unity too. The status area thing will be a tiny bolt on thing that uses the same interface, possibly in g-s-d (maybe in another plugin, I dunno). I think Andre wanted to work on that at one stage. New patches coming.
Created attachment 191099 [details] [review] gnome-phone-manager interface code, from g-p-m
Created attachment 191100 [details] [review] Clear notifications on battery full, for broken hardware that flutters
Created attachment 191101 [details] [review] notification for UPS discharging
Review of attachment 191100 [details] [review]: ::: plugins/power/gsd-power-manager.c @@ +168,3 @@ + GError *error = NULL; + + /* exists? */ really? @@ +172,3 @@ + return; + + GError *error = NULL; And again. @@ +871,3 @@ g_debug ("** EMIT: discharging"); } else if (state == UP_DEVICE_STATE_FULLY_CHARGED) { + g_debug ("fully charged"); g_debug ("fully charged, hiding notifications if any");
Review of attachment 191101 [details] [review]: ::: plugins/power/gsd-power-manager.c @@ +185,3 @@ +static void +notification_closed_cb (NotifyNotification *notification, g_object_add_weak_pointer() instead? @@ +206,3 @@ + icon_names = g_themed_icon_get_names (G_THEMED_ICON (icon)); + if (icon_names != NULL) +get_first_themed_icon_name (GIcon *icon) That's only valid as long as the GIcon exists. @@ +870,2 @@ static void +engine_device_discharging (GsdPowerManager *manager, UpDevice *device) engine_ups_discharging then? Or at least some explanation as to why you're ignoring every other type of devices. @@ +889,3 @@ + NULL); + + GIcon *icon = NULL; why? @@ +907,3 @@ + } else { + message = g_strdup (gpm_device_to_localised_string (device)); + NULL); Don't make the translators copy/paste the percentage format. Seeing as it'll still be brackets with a percentage at the end of the string, do it separately, without _(). @@ +918,3 @@ + + /* create a new notification */ + if (time_to_empty > 0) Don't split those type of lines, our screens are wide enough. @@ +933,3 @@ + ret = notify_notification_show (manager->priv->notification_discharging, + &error); + if (remaining_text != NULL) { if (notify_notification_show (bleh, &error) == FALSE) { ... } @@ +937,3 @@ + g_error_free (error); + g_object_unref (manager->priv->notification_discharging); + message = g_strdup_printf (_("%s of UPS backup power remaining (%.0f%%)"), out, which is a whole line below.
Created attachment 191105 [details] [review] notification for UPS discharging (version 2)
Created attachment 191107 [details] [review] Make notifications and sounds on low power events
Created attachment 191109 [details] [review] Actually do the critical action when required
Review of attachment 191105 [details] [review]: ::: plugins/power/gsd-power-manager.c @@ +904,3 @@ + manager->priv->notification_discharging = notify_notification_new (title, + message->str, + get_first_themed_icon_name (icon)); Need to call notify_notification_set_app_name here, else it will show up as 'gnome-settings-daemon'
Review of attachment 191107 [details] [review]: ::: plugins/power/gsd-power-manager.c @@ +1154,3 @@ + message, + get_first_themed_icon_name (icon)); + notify_notification_set_timeout (manager->priv->notification_low, Need to call notify_notification_set_app_name here @@ +1333,3 @@ + manager->priv->notification_low = notify_notification_new (title, + message, + get_first_themed_icon_name (icon)); And here too @@ +1483,3 @@ + manager->priv->notification_low = notify_notification_new (title, + message, + get_first_themed_icon_name (icon)); And here too @@ +1504,3 @@ + CA_PROP_EVENT_ID, "battery-caution", + /* TRANSLATORS: this is the sound description */ + CA_PROP_EVENT_DESCRIPTION, _("Battery is low"), NULL); Should the event description be different from the one further up ?
(In reply to comment #42) > Need to call notify_notification_set_app_name here, else it will show up as > 'gnome-settings-daemon' All fixed. (In reply to comment #43) > @@ +1504,3 @@ > + CA_PROP_EVENT_ID, "battery-caution", > + /* TRANSLATORS: this is the sound > description */ > + CA_PROP_EVENT_DESCRIPTION, _("Battery is > low"), NULL); > > Should the event description be different from the one further up ? Yup, good catch. That bug actually existed in g-p-m for years without anyone noticing.
Created attachment 191234 [details] [review] Add a helper GObject to interface with X11 IDLETIME counters Note: I've got some self-test code for this functionality (it's a manual test, not suitable for 'make check') so yell if you want me to include that in g-s-d too.
Review of attachment 191234 [details] [review]: ::: plugins/power/gpm-idletime.c @@ +281,3 @@ + alarm_item = gpm_idletime_alarm_new (idletime, id); + + /* add to array */ yay, comments. @@ +364,3 @@ + /* get the sync event */ + if (!XSyncQueryExtension (idletime->priv->dpy, &idletime->priv->sync_event, &sync_error)) { + g_warning ("No Sync extension."); Why should that be a g_warning? @@ +370,3 @@ + /* gtk_init should do XSyncInitialize for us */ + counters = XSyncListSystemCounters (idletime->priv->dpy, &ncounters); + for (i=0; i < ncounters && !idletime->priv->idle_counter; i++) { "i = 0"
Created attachment 191273 [details] [review] Ensure any display devices are not still suspended at system resume time
Created attachment 191274 [details] [review] Ensure the panel is turned back on after the lid has been opened
Created attachment 191275 [details] [review] Ensure the panel is explicitly turned off if the lid has been closed
Created attachment 191276 [details] [review] Ensure we just blank the screen for any of the policy actions that are set to 'blank'
Created attachment 191277 [details] [review] Close existing notifications on resume as the power state is likely different
Created attachment 191278 [details] [review] Simulate user activity on resume so that the unlock dialog box is shown rather than a black screen
Created attachment 191279 [details] [review] Lock the screen before allowing the computer to sleep, if configured to do so
Created attachment 191280 [details] [review] Show a modal dialog if any battery in the system has been recalled and may be dangerous
Review of attachment 191234 [details] [review]: Committed with fixes, thanks.
Review of attachment 191273 [details] [review]: Looks good.
Review of attachment 191274 [details] [review]: Yep.
Review of attachment 191275 [details] [review]: Yep.
Review of attachment 191276 [details] [review]: "any to 'blank'" any what? any screen? Really unclear.
Review of attachment 191277 [details] [review]: Likely different? What if they aren't, would we get new notifications?
(In reply to comment #60) > Review of attachment 191277 [details] [review]: > > Likely different? What if they aren't, would we get new notifications? Yup. The case I'm trying to prevent here is: Laptop critically low User closes lid and suspends Inserts power adapter (some time elapses) User opens lid Sees notification of critical low battery for a few seconds User gets confused.
Review of attachment 191278 [details] [review]: Looks good.
Review of attachment 191279 [details] [review]: Looks good.
Review of attachment 191280 [details] [review]: I like the feature, but I don't like the implementation. System modal dialogues need to be implemented in gnome-shell.
Created attachment 191396 [details] [review] Connect to gnome-session to get the inhibit and presence data
Created attachment 191397 [details] [review] Create an idletime watcher to track X idle status
Review of attachment 191396 [details] [review]: Fine.
Review of attachment 191397 [details] [review]: ::: plugins/power/gsd-power-manager.c @@ +75,3 @@ #define GSD_POWER_MANAGER_RECALL_DELAY 30 /* seconds */ +#define GSD_POWER_IDLETIME_ID 1 /* counter id */ It's not passed in to gpm_idletime_new(), but is to alarm_remove(). I must say I don't quite understand what this constant is about. Should it be in gpm-idletime.h instead?
(In reply to comment #68) > Review of attachment 191397 [details] [review]: > > ::: plugins/power/gsd-power-manager.c > @@ +75,3 @@ > #define GSD_POWER_MANAGER_RECALL_DELAY 30 /* seconds */ > > +#define GSD_POWER_IDLETIME_ID 1 /* counter id */ > > It's not passed in to gpm_idletime_new(), but is to alarm_remove(). Right, in the pending chunk that I'm working on we use the ID to get an X counter. > I must say I don't quite understand what this constant is about. Should it be > in gpm-idletime.h instead? I'm not sure myself. I think the power plugin only ever needs 1 counter value (and one reset of course) but in the past I experimented with two (before the counter races in X were fixed). If anything else in the g-s-d process ever uses idletime I guess it's would have to use a counter != 1 -- hence the define. I can write that up as a comment to the define if you'd like. Richard.
(In reply to comment #69) > (In reply to comment #68) > > Review of attachment 191397 [details] [review] [details]: > > > > ::: plugins/power/gsd-power-manager.c > > @@ +75,3 @@ > > #define GSD_POWER_MANAGER_RECALL_DELAY 30 /* seconds */ > > > > +#define GSD_POWER_IDLETIME_ID 1 /* counter id */ > > > > It's not passed in to gpm_idletime_new(), but is to alarm_remove(). > > Right, in the pending chunk that I'm working on we use the ID to get an X > counter. > > > I must say I don't quite understand what this constant is about. Should it be > > in gpm-idletime.h instead? > > I'm not sure myself. I think the power plugin only ever needs 1 counter value > (and one reset of course) but in the past I experimented with two (before the > counter races in X were fixed). If anything else in the g-s-d process ever uses > idletime I guess it's would have to use a counter != 1 -- hence the define. I > can write that up as a comment to the define if you'd like. In that case, remove all uses of the counter in a single commit, and mention the reason for removal in the commit message. If we ever need it again, we can revert the commit. In the meanwhile, it's just confusing.