GNOME Bugzilla – Bug 672691
Hide/show mapping buttons depending on the presence of a PAD on the device
Last modified: 2012-05-25 10:53:09 UTC
The attached patches hide the "map button" from the UI when no pad is present on the device. It's slightly more complex than just hiding the button because of a race condition between the wacom driver and he the UI. The device_added_cb is called once for each tool added. The wacom driver hotplugs tools in the order stylus, eraser, cursor, pad. update_current_page will add a new page once a tablet has stylus and eraser, before cursor and pad exist. priv->pad is thus always NULL, causing, cc_wacom_page's update_tablet_ui to remove the "Map Buttons..." button for any device. Change the code to update the tool list for every new tool we get, merely triggering the visibility of the button instead of destroying it completely. Also, in the case of a screen tablet, the calbrator cannot run if a matching is not made with a Wacom monitor, this patches also make the "calibrate" button insensitive when calibration cannot be run (better that keeping a button without any effect).
Created attachment 210426 [details] [review] Patch 1/3
Created attachment 210427 [details] [review] Patch 2/3
Created attachment 210428 [details] [review] Patch 3/3
Review of attachment 210426 [details] [review]: Looks good. Remove the Signed-Off-By lines though.
Review of attachment 210427 [details] [review]: Please merge the patch into Peter's. ::: panels/wacom/cc-wacom-panel.c @@ -226,3 @@ g_hash_table_insert (priv->pages, g_strdup (tablet->name), page); changed = TRUE; } else If one condition branch has braces, please use them in both branches.
Review of attachment 210428 [details] [review]: Is this a clean up patch? Looks like a bug fix for a previous patch, and an undocumented functional change. ::: panels/wacom/cc-wacom-page.c @@ +1056,2 @@ gtk_widget_show (WID ("button-calibrate")); + if (gsd_wacom_device_get_display_monitor (page->priv->stylus) < 0) This looks unrelated to the rest of the patch.
(In reply to comment #6) > Review of attachment 210428 [details] [review]: > > Is this a clean up patch? Looks like a bug fix for a previous patch, and an > undocumented functional change. All 3 patches are dependant, it's separate commits that occurred at different point in time (git format-patch output). If more appropriate I could provide a simpler git diff but we'd be loosing history and git commit. > ::: panels/wacom/cc-wacom-page.c > @@ +1056,2 @@ > gtk_widget_show (WID ("button-calibrate")); > + if (gsd_wacom_device_get_display_monitor (page->priv->stylus) < 0) > > This looks unrelated to the rest of the patch. Unrelated but not undocumented, that's the "this patches also make the "calibrate" button insensitive when calibration cannot be run" part in comment #0
I'd rather a single commit with "With help from..." than 3 separate commits, 2 of which are bug fixes to the original commit.
(In reply to comment #8) > I'd rather a single commit with "With help from..." than 3 separate commits, 2 > of which are bug fixes to the original commit. OK I'll prepare a new patch. While I'm at it Do you want to keep the "make the calibrate button insensitive when calibration cannot be run" part or would you prefer a separate bug for that?
If you can separate out the commits, I'd rather they be separate (as they're functionally separate). Thanks.
Created attachment 211813 [details] [review] Updated (consolidated) patch (In reply to comment #10) > If you can separate out the commits, I'd rather they be separate (as they're > functionally separate). Thanks. Consolidated patch (without the calibrate fix which will be sent in a separate bugzilla)
Created attachment 211815 [details] [review] Updated (consolidated) patch Same with curly braces as per comment 5
Review of attachment 211815 [details] [review]: Can you create your patch with "git format-patch -1" rather than "git show" please? It won't apply with "git am" otherwise.
Created attachment 211927 [details] [review] Updated (consolidated) patch (In reply to comment #13) > Can you create your patch with "git format-patch -1" rather than "git show" > please? It won't apply with "git am" otherwise. Sure, here it comes :)
*** Bug 676791 has been marked as a duplicate of this bug. ***