GNOME Bugzilla – Bug 700461
Check calibration warnings when having two tablets
Last modified: 2013-12-18 17:25:19 UTC
We need to check what happens to the desktop notification when two screen tablets are connected at the same time and resolution changes.
Created attachment 263663 [details] [review] wacom: Maintain per-device calibration notifications All information around calibration notifications is now in a separate struct, set as GObject data on the GsdWacomDevice. On calibration checks, each device updates its notication as required. This fixes: 1) The notification being possibly not shown if multiple in-screen devices are present. If the resolution changes on one monitor, but the device on the other monitor is checked last, the notification was then closed/removed. 2) Older but still valid notifications disappearing if a second device needs calibration.
Created attachment 263664 [details] [review] wacom: dismiss the calibration notification if it happened through other means Listen for changes in the last calibration resolution, so if the user did the calibration directly through g-c-c instead of the notification, the notification is dismissed from the notification area.
Created attachment 263665 [details] [review] wacom: Check for calibration state on startup/device-added So the user is notified when plugging a brand new tablet, or a previously known one that's maybe miscalibrated on the current set-up.
Review of attachment 263663 [details] [review]: Looks good otherwise. ::: plugins/wacom/gsd-wacom-manager.c @@ +102,3 @@ + NotifyNotification *calibration_notification; + GsdWacomDevice *device; + guint notification_timeout_id; align the variable names please, makes it easier to read/parse. @@ +1590,3 @@ GdkRectangle geometry; + g_debug ("Checking calibration for: %s", On the same line is fine. @@ +1742,1 @@ return FALSE; Return G_SOURCE_REMOVE instead now. @@ +1752,3 @@ + wacom_device_calibration_data_update (data); + + if (data->notification_timeout_id == 0) add braces here. @@ +1753,3 @@ + + if (data->notification_timeout_id == 0) + data->notification_timeout_id = put the function on the same line.
Review of attachment 263664 [details] [review]: Looks good.
Review of attachment 263665 [details] [review]: ::: plugins/wacom/gsd-wacom-manager.c @@ +1120,3 @@ device = gsd_wacom_device_new (gdk_device); device_name = gsd_wacom_device_get_name (device); + type = gsd_wacom_device_get_device_type (device); Can you please split the addition of the local "type" variable from the addition of the check? That would make it easier to read.
Attachment 263663 [details] pushed as a859aaa - wacom: Maintain per-device calibration notifications Attachment 263664 [details] pushed as 48c9f4f - wacom: dismiss the calibration notification if it happened through other means
Created attachment 264279 [details] [review] wacom: Keep GsdWacomDeviceType on the stack in device_added_cb() So gsd_wacom_device_get_device_type() isn't called multiple times.
Created attachment 264281 [details] [review] wacom: Keep GsdWacomDeviceType on the stack in device_added_cb() So gsd_wacom_device_get_device_type() isn't called multiple times.
Created attachment 264282 [details] [review] wacom: Check for calibration state on startup/device-added So the user is notified when plugging a brand new tablet, or a previously known one that's maybe miscalibrated on the current set-up.
Review of attachment 264279 [details] [review]: Looks good.
Review of attachment 264281 [details] [review]: ++
Review of attachment 264282 [details] [review]: Looks good otherwise. ::: plugins/wacom/gsd-wacom-manager.c @@ +1166,3 @@ } + set_wacom_settings (manager, device); Whitespace changes?
(In reply to comment #13) > Review of attachment 264282 [details] [review]: > > Looks good otherwise. > > ::: plugins/wacom/gsd-wacom-manager.c > @@ +1166,3 @@ > } > > + set_wacom_settings (manager, device); > > Whitespace changes? Well... that file altogether mixes tabs and spaces all over the place. that specific function uses tabs on some blocks and spaces on others, would you want a commit making all that consistent at once?
(In reply to comment #14) > (In reply to comment #13) > > Review of attachment 264282 [details] [review] [details]: > > > > Looks good otherwise. > > > > ::: plugins/wacom/gsd-wacom-manager.c > > @@ +1166,3 @@ > > } > > > > + set_wacom_settings (manager, device); > > > > Whitespace changes? > > Well... that file altogether mixes tabs and spaces all over the place. that > specific function uses tabs on some blocks and spaces on others, would you want > a commit making all that consistent at once? I'd just want the line not to change just because of a whitespace change. It's unrelated to the rest of the patch.
Attachment 264281 [details] pushed as db71965 - wacom: Keep GsdWacomDeviceType on the stack in device_added_cb() Attachment 264282 [details] pushed as 7edd93c - wacom: Check for calibration state on startup/device-added