After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 703148 - Use a GtkListBox for the buttons' mapping instead of a GtkTreeView
Use a GtkListBox for the buttons' mapping instead of a GtkTreeView
Status: RESOLVED FIXED
Product: gnome-control-center
Classification: Core
Component: Wacom
unspecified
Other All
: Normal normal
: ---
Assigned To: Joaquim Rocha
Control-Center Maintainers
Depends on: 701200
Blocks:
 
 
Reported: 2013-06-26 18:39 UTC by Joaquim Rocha
Modified: 2015-12-07 15:05 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
wacom: Use a GtkListBox for the buttons' mapping instead of a GtkTreeView (32.96 KB, patch)
2013-06-26 18:39 UTC, Joaquim Rocha
needs-work Details | Review
Screenshot showing the listbox (43.65 KB, image/png)
2013-06-26 18:42 UTC, Joaquim Rocha
  Details
wacom: Use a GtkListBox for the buttons' mapping instead of a GtkTreeView (24.42 KB, patch)
2013-07-09 14:41 UTC, Joaquim Rocha
needs-work Details | Review
wacom: Use a GtkListBox for the buttons' mapping instead of a GtkTreeView (23.88 KB, patch)
2013-07-17 10:10 UTC, Joaquim Rocha
needs-work Details | Review
wacom: Add gsd-wacom-key-shortcut-button to be updated from g-s-d (975 bytes, patch)
2013-07-23 10:48 UTC, Joaquim Rocha
accepted-commit_now Details | Review
wacom: Update from gnome-settings-daemon (23.23 KB, patch)
2013-07-23 10:48 UTC, Joaquim Rocha
reviewed Details | Review
wacom: Use a GtkListBox for the buttons' mapping instead of a GtkTreeView (22.83 KB, patch)
2013-07-23 10:49 UTC, Joaquim Rocha
accepted-commit_now Details | Review

Description Joaquim Rocha 2013-06-26 18:39:01 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+).
Comment 1 Joaquim Rocha 2013-06-26 18:39:04 UTC
Created attachment 247851 [details] [review]
wacom: Use a GtkListBox for the buttons' mapping instead of a GtkTreeView
Comment 2 Joaquim Rocha 2013-06-26 18:41:41 UTC
Adding the bug it depends on.

It also makes https://bugzilla.gnome.org/show_bug.cgi?id=686782 obsolete.
Comment 3 Joaquim Rocha 2013-06-26 18:42:44 UTC
Created attachment 247852 [details]
Screenshot showing the listbox
Comment 4 Bastien Nocera 2013-07-05 08:47:22 UTC
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.
Comment 5 Bastien Nocera 2013-07-05 08:49:44 UTC
Review of attachment 247851 [details] [review]:

::: panels/wacom/cc-wacom-page.c
@@ +146,3 @@
+} CcWacomButtonRowPrivate;
+
+struct _CcWacomButtonRow {

Please move this to a separate file.
Comment 6 Joaquim Rocha 2013-07-09 14:41:28 UTC
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 ).
Comment 7 Joaquim Rocha 2013-07-09 14:43:24 UTC
Dropping the key shortcut button in GTK+ as a dependency and adding the libgd's one instead...
Comment 8 Bastien Nocera 2013-07-16 14:59:28 UTC
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?
Comment 9 Joaquim Rocha 2013-07-16 15:26:46 UTC
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 :)
Comment 10 Joaquim Rocha 2013-07-17 10:10:54 UTC
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.
Comment 11 Bastien Nocera 2013-07-22 11:18:59 UTC
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.
Comment 12 Joaquim Rocha 2013-07-23 10:48:31 UTC
Created attachment 249882 [details] [review]
wacom: Add gsd-wacom-key-shortcut-button to be updated from g-s-d
Comment 13 Joaquim Rocha 2013-07-23 10:48:44 UTC
Created attachment 249883 [details] [review]
wacom: Update from gnome-settings-daemon
Comment 14 Joaquim Rocha 2013-07-23 10:49:09 UTC
Created attachment 249884 [details] [review]
wacom: Use a GtkListBox for the buttons' mapping instead of a GtkTreeView
Comment 15 Bastien Nocera 2013-07-23 13:11:19 UTC
Review of attachment 249882 [details] [review]:

Yes.
Comment 16 Bastien Nocera 2013-07-23 13:11:47 UTC
Review of attachment 249883 [details] [review]:

Yes. Will need to be updated to match what actually gets committed to gnome-settings-daemon.
Comment 17 Bastien Nocera 2013-07-23 13:19:23 UTC
Review of attachment 249884 [details] [review]:

Looks good.
Comment 18 Joaquim Rocha 2013-07-24 09:23:17 UTC
Pushed after updating the patch Bastien mentioned.

commit df161dba1845e79bb83498ba0c8d193b4c742e43
commit afea7e93f0c2fe17ac3a18b321fb82b75ff76c2e
commit d12550526696adfa3f56a71531310f32ecdbf6d5
Comment 19 Pander 2015-12-07 15:02:33 UTC
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 ?
Comment 20 Bastien Nocera 2015-12-07 15:05:48 UTC
Yes, that solved it. Time to upgrade ;)