GNOME Bugzilla – Bug 703148
Use a GtkListBox for the buttons' mapping instead of a GtkTreeView
Last modified: 2015-12-07 15:05:48 UTC
The reason is that we avoid using the custom cell renderer for displaying the key modifiers and instead use a GtkKeyShortcutButton (still pending review/commit in GTK+).
Created attachment 247851 [details] [review] wacom: Use a GtkListBox for the buttons' mapping instead of a GtkTreeView
Adding the bug it depends on. It also makes https://bugzilla.gnome.org/show_bug.cgi?id=686782 obsolete.
Created attachment 247852 [details] Screenshot showing the listbox
I think that the widgets are supposed to be themed "white" as in the network panel for example, and the keyboard panel would need to be ported too.
Review of attachment 247851 [details] [review]: ::: panels/wacom/cc-wacom-page.c @@ +146,3 @@ +} CcWacomButtonRowPrivate; + +struct _CcWacomButtonRow { Please move this to a separate file.
Created attachment 248735 [details] [review] wacom: Use a GtkListBox for the buttons' mapping instead of a GtkTreeView This new version has the list box's row in a separate file and uses the key shortcut button from libgd instead of GTK+ (added here: https://bugzilla.gnome.org/show_bug.cgi?id=703865 ).
Dropping the key shortcut button in GTK+ as a dependency and adding the libgd's one instead...
Review of attachment 248735 [details] [review]: ::: configure.ac @@ +112,3 @@ gsettings-desktop-schemas >= $SCHEMAS_REQUIRED_VERSION" +LIBGD_INIT([_view-common notification main-toolbar header-bar stack static key-shortcut-button]) As mentioned earlier, I'd rather the button was copy/pasted between g-s-d and g-c-c for now. There's a tool to copy files from g-s-d already, look in panels/wacom/Makefile.am ::: panels/wacom/cc-wacom-page.c @@ +314,3 @@ + if (type >= G_N_ELEMENTS(action_table)) + return FALSE; + return TRUE; Why the space changes?
Okay, I'll move the widget again. The space changes are a mistake, my Emacs takes into account the surrounding indentation but I'll blame it anyway :)
Created attachment 249392 [details] [review] wacom: Use a GtkListBox for the buttons' mapping instead of a GtkTreeView New version addressing Bastien's comments. It also gets the shortcut button from the update-from-gsd script.
Review of attachment 249392 [details] [review]: ::: panels/wacom/Makefile.am @@ +87,2 @@ WACOMDIR=$(top_srcdir)/../gnome-settings-daemon/plugins/wacom/ +WACOMFILES=gsd-wacom-device.c gsd-wacom-device.h gsd-wacom-key-shortcut-button.c gsd-wacom-key-shortcut-button.h Split out this change into a separate patch, that you can apply, and run the update-from-gsd, and then apply the rest of this patch. ::: panels/wacom/cc-wacom-page.c @@ +328,2 @@ + actions_scrolled_window = GTK_CONTAINER (MWID ("actions_swindow")); + list_box = gtk_list_box_new (); Create the list box inside the .ui file instead. @@ +352,3 @@ + + row = cc_wacom_button_row_new (button, 0); + gtk_container_add (GTK_CONTAINER (list_box), GTK_WIDGET (row)); Split those 2 lines into a function that takes in a button and the list_box, and shows the widget. @@ +355,3 @@ } + + gtk_widget_show_all (list_box); Don't use show_all if at all possible. It shouldn't be needed with the above changes.
Created attachment 249882 [details] [review] wacom: Add gsd-wacom-key-shortcut-button to be updated from g-s-d
Created attachment 249883 [details] [review] wacom: Update from gnome-settings-daemon
Created attachment 249884 [details] [review] wacom: Use a GtkListBox for the buttons' mapping instead of a GtkTreeView
Review of attachment 249882 [details] [review]: Yes.
Review of attachment 249883 [details] [review]: Yes. Will need to be updated to match what actually gets committed to gnome-settings-daemon.
Review of attachment 249884 [details] [review]: Looks good.
Pushed after updating the patch Bastien mentioned. commit df161dba1845e79bb83498ba0c8d193b4c742e43 commit afea7e93f0c2fe17ac3a18b321fb82b75ff76c2e commit d12550526696adfa3f56a71531310f32ecdbf6d5
Just to be sure, this solves/solved also https://bugzilla.gnome.org/show_bug.cgi?id=686782 and https://bugzilla.gnome.org/show_bug.cgi?id=694440 ?
Yes, that solved it. Time to upgrade ;)