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 774914 - Add a GtkFlowBox based equivalent of GdMainIconView
Add a GtkFlowBox based equivalent of GdMainIconView
Status: RESOLVED FIXED
Product: libgd
Classification: Platform
Component: general
unspecified
Other All
: Normal enhancement
: ---
Assigned To: libgd maintainer(s)
libgd maintainer(s)
Depends on: 776012 776187 776306 779074
Blocks: 690623
 
 
Reported: 2016-11-23 13:34 UTC by Debarshi Ray
Modified: 2017-03-04 19:50 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
Add GdMainBoxItem (9.05 KB, patch)
2016-12-08 17:38 UTC, Debarshi Ray
none Details | Review
Add GdMainBoxChild (7.28 KB, patch)
2016-12-08 17:39 UTC, Debarshi Ray
none Details | Review
Add GdMainBoxGeneric (11.09 KB, patch)
2016-12-08 17:39 UTC, Debarshi Ray
none Details | Review
build: Add a _box-common flag to LIBGD_INIT (3.06 KB, patch)
2016-12-08 17:39 UTC, Debarshi Ray
none Details | Review
Add GdMainIconBoxChild (12.41 KB, patch)
2016-12-08 17:39 UTC, Debarshi Ray
none Details | Review
Add GdMainIconBox (19.01 KB, patch)
2016-12-08 17:40 UTC, Debarshi Ray
none Details | Review
build: Add a main-icon-box flag to LIBGD_INIT (2.96 KB, patch)
2016-12-08 17:40 UTC, Debarshi Ray
none Details | Review
Add GdMainBox (24.96 KB, patch)
2016-12-08 17:40 UTC, Debarshi Ray
none Details | Review
build: Add a main-box flag to LIBGD_INIT (2.69 KB, patch)
2016-12-08 17:40 UTC, Debarshi Ray
none Details | Review
Add GdMainBoxItem (9.82 KB, patch)
2016-12-11 14:47 UTC, Debarshi Ray
none Details | Review
Add GdMainBoxChild (7.28 KB, patch)
2016-12-11 14:47 UTC, Debarshi Ray
none Details | Review
Add GdMainBoxGeneric (15.52 KB, patch)
2016-12-11 14:48 UTC, Debarshi Ray
none Details | Review
build: Add a _box-common flag to LIBGD_INIT (3.06 KB, patch)
2016-12-11 14:48 UTC, Debarshi Ray
none Details | Review
Add GdMainIconBoxChild (12.35 KB, patch)
2016-12-11 14:49 UTC, Debarshi Ray
none Details | Review
Add GdMainIconBox (25.91 KB, patch)
2016-12-11 14:49 UTC, Debarshi Ray
none Details | Review
build: Add a main-icon-box flag to LIBGD_INIT (2.96 KB, patch)
2016-12-11 14:50 UTC, Debarshi Ray
none Details | Review
Add GdMainBox (16.77 KB, patch)
2016-12-11 14:51 UTC, Debarshi Ray
none Details | Review
build: Add a main-box flag to LIBGD_INIT (2.69 KB, patch)
2016-12-11 14:51 UTC, Debarshi Ray
none Details | Review
icon-utils, main-view: Export the image surface copying code (4.65 KB, patch)
2016-12-21 07:49 UTC, Debarshi Ray
none Details | Review
icon-utils, main-view: Export the image surface copying code (4.54 KB, patch)
2016-12-21 08:18 UTC, Debarshi Ray
none Details | Review
icon-utils, main-view: Export the DnD counter creation code (9.18 KB, patch)
2016-12-21 08:18 UTC, Debarshi Ray
none Details | Review
Add GdMainBoxItem (9.86 KB, patch)
2016-12-21 08:19 UTC, Debarshi Ray
none Details | Review
Add GdMainBoxChild (7.34 KB, patch)
2016-12-21 08:19 UTC, Debarshi Ray
none Details | Review
Add GdMainBoxGeneric (15.67 KB, patch)
2016-12-21 08:20 UTC, Debarshi Ray
none Details | Review
Add GdMainIconBoxChild (12.00 KB, patch)
2016-12-21 08:20 UTC, Debarshi Ray
none Details | Review
Add GdMainIconBox (32.71 KB, patch)
2016-12-21 08:21 UTC, Debarshi Ray
none Details | Review
Add GdMainBox (16.73 KB, patch)
2016-12-21 08:22 UTC, Debarshi Ray
none Details | Review
icon-utils, main-view: Export the image surface copying code (4.54 KB, patch)
2017-01-03 20:01 UTC, Debarshi Ray
committed Details | Review
icon-utils, main-view: Export the DnD counter creation code (9.18 KB, patch)
2017-01-03 20:01 UTC, Debarshi Ray
committed Details | Review
Add GdMainBoxItem (9.86 KB, patch)
2017-01-03 20:02 UTC, Debarshi Ray
none Details | Review
Add GdMainBoxChild (8.43 KB, patch)
2017-01-03 20:02 UTC, Debarshi Ray
none Details | Review
Add GdMainBoxGeneric (18.88 KB, patch)
2017-01-03 20:02 UTC, Debarshi Ray
none Details | Review
build: Add a _box-common flag to LIBGD_INIT (3.07 KB, patch)
2017-01-03 20:03 UTC, Debarshi Ray
committed Details | Review
Add GdMainIconBoxChild (14.69 KB, patch)
2017-01-03 20:03 UTC, Debarshi Ray
none Details | Review
Add GdMainIconBox (34.96 KB, patch)
2017-01-03 20:04 UTC, Debarshi Ray
none Details | Review
build: Add a main-icon-box flag to LIBGD_INIT (3.01 KB, patch)
2017-01-03 20:04 UTC, Debarshi Ray
committed Details | Review
Add GdMainBox (20.96 KB, patch)
2017-01-03 20:04 UTC, Debarshi Ray
none Details | Review
build: Add a main-box flag to LIBGD_INIT (2.69 KB, patch)
2017-01-03 20:05 UTC, Debarshi Ray
committed Details | Review
Add GdMainBoxItem (9.86 KB, patch)
2017-01-07 13:01 UTC, Debarshi Ray
committed Details | Review
Add GdMainBoxChild (8.43 KB, patch)
2017-01-07 13:01 UTC, Debarshi Ray
committed Details | Review
Add GdMainBoxGeneric (18.81 KB, patch)
2017-01-07 13:02 UTC, Debarshi Ray
committed Details | Review
Add GdMainIconBoxChild (14.69 KB, patch)
2017-01-07 13:02 UTC, Debarshi Ray
committed Details | Review
Add GdMainIconBox (34.74 KB, patch)
2017-01-07 13:02 UTC, Debarshi Ray
committed Details | Review
Add GdMainBox (21.48 KB, patch)
2017-01-07 13:03 UTC, Debarshi Ray
committed Details | Review
main-box-item: Remove unused virtual function pointer (988 bytes, patch)
2017-01-26 17:40 UTC, Debarshi Ray
committed Details | Review
main-box: Render a frame (841 bytes, patch)
2017-01-26 17:40 UTC, Debarshi Ray
accepted-commit_now Details | Review
main-box: Enclose the view in a GtkFrame (2.63 KB, patch)
2017-01-27 15:10 UTC, Debarshi Ray
committed Details | Review
main-box-generic: Remove unused variable and function call (981 bytes, patch)
2017-02-17 17:12 UTC, Debarshi Ray
committed Details | Review
main-icon-box-child: Remove unused variables (1.57 KB, patch)
2017-02-17 17:19 UTC, Debarshi Ray
committed Details | Review
main-icon-box: Remove unused variables (2.02 KB, patch)
2017-03-04 09:54 UTC, Debarshi Ray
committed Details | Review

Description Debarshi Ray 2016-11-23 13:34:57 UTC
The current designs of our content applications really mandate using GtkFlowBox instead of GtkIconView. Let's try to write something reasonably generic that can shared across multiple applications.
Comment 1 Debarshi Ray 2016-11-23 16:02:31 UTC
I am working on this in libgd:wip/rishi/main-box.
Comment 2 Debarshi Ray 2016-12-08 17:38:54 UTC
Created attachment 341629 [details] [review]
Add GdMainBoxItem
Comment 3 Debarshi Ray 2016-12-08 17:39:10 UTC
Created attachment 341630 [details] [review]
Add GdMainBoxChild
Comment 4 Debarshi Ray 2016-12-08 17:39:24 UTC
Created attachment 341631 [details] [review]
Add GdMainBoxGeneric
Comment 5 Debarshi Ray 2016-12-08 17:39:38 UTC
Created attachment 341632 [details] [review]
build: Add a _box-common flag to LIBGD_INIT
Comment 6 Debarshi Ray 2016-12-08 17:39:54 UTC
Created attachment 341633 [details] [review]
Add GdMainIconBoxChild
Comment 7 Debarshi Ray 2016-12-08 17:40:08 UTC
Created attachment 341634 [details] [review]
Add GdMainIconBox
Comment 8 Debarshi Ray 2016-12-08 17:40:23 UTC
Created attachment 341635 [details] [review]
build: Add a main-icon-box flag to LIBGD_INIT
Comment 9 Debarshi Ray 2016-12-08 17:40:41 UTC
Created attachment 341636 [details] [review]
Add GdMainBox
Comment 10 Debarshi Ray 2016-12-08 17:40:55 UTC
Created attachment 341637 [details] [review]
build: Add a main-box flag to LIBGD_INIT
Comment 11 Debarshi Ray 2016-12-08 17:48:31 UTC
The trickiest / messiest part of these patches is the selection handling. It seems that we are fighting against GtkFlowBox's built-in selection handling to match the behaviour of GdMainView. I have gotten it to match the old behaviour except that range selection doesn't work with the keyboard. ie., shift+enter won't select a range of items.

I don't know how attached we are to the current GdMainView behaviour, or maybe there are ways to make the code cleaner and retain the same behaviour?

I see that GdMainView has minor divergences from the HIG [1] because shift+<activate> (and not just ctrl+<activate>) should also initiate the selection mode.

Theming is also completely absent. I guess, we can get to that once the code is ready.

[1] https://developer.gnome.org/hig/stable/selection-mode.html.
Comment 12 Debarshi Ray 2016-12-08 17:55:57 UTC
You can try these with the patches in bug 690623

I have left some debug statements in the code. I will remove them as we progress through the review.
Comment 13 Debarshi Ray 2016-12-11 14:46:47 UTC
I am happier with the code now.

(In reply to Debarshi Ray from comment #11)
> The trickiest / messiest part of these patches is the selection handling. It
> seems that we are fighting against GtkFlowBox's built-in selection handling
> to match the behaviour of GdMainView. I have gotten it to match the old
> behaviour except that range selection doesn't work with the keyboard. ie.,
> shift+enter won't select a range of items.

GdMainIconBox now derives from GtkFlowBox instead of GtkBin, and I moved all the selection handling hacks into gd-main-icon-box.c. I think that it makes the hacks a bit more palatable, and will keep the GdMainIconBox hacks separate from a future GdMainListBox.

Range selection now works with the keyboard and I have replaced the debug statements with comments.

I noticed that initiating a rubberband selection with GtkFlowBox causes us to lose the previous selection. We might want to change that to match the GdMainView behaviour. Not sure what that will involve, though.
Comment 14 Debarshi Ray 2016-12-11 14:47:15 UTC
Created attachment 341741 [details] [review]
Add GdMainBoxItem
Comment 15 Debarshi Ray 2016-12-11 14:47:40 UTC
Created attachment 341742 [details] [review]
Add GdMainBoxChild
Comment 16 Debarshi Ray 2016-12-11 14:48:06 UTC
Created attachment 341743 [details] [review]
Add GdMainBoxGeneric
Comment 17 Debarshi Ray 2016-12-11 14:48:50 UTC
Created attachment 341744 [details] [review]
build: Add a _box-common flag to LIBGD_INIT
Comment 18 Debarshi Ray 2016-12-11 14:49:09 UTC
Created attachment 341745 [details] [review]
Add GdMainIconBoxChild
Comment 19 Debarshi Ray 2016-12-11 14:49:30 UTC
Created attachment 341746 [details] [review]
Add GdMainIconBox
Comment 20 Debarshi Ray 2016-12-11 14:50:43 UTC
Created attachment 341747 [details] [review]
build: Add a main-icon-box flag to LIBGD_INIT
Comment 21 Debarshi Ray 2016-12-11 14:51:03 UTC
Created attachment 341748 [details] [review]
Add GdMainBox
Comment 22 Debarshi Ray 2016-12-11 14:51:22 UTC
Created attachment 341749 [details] [review]
build: Add a main-box flag to LIBGD_INIT
Comment 23 Cosimo Cecchi 2016-12-18 23:22:16 UTC
Review of attachment 341741 [details] [review]:

::: libgd/gd-main-box-item.c
@@ +206,3 @@
+
+  if (icon != NULL)
+    g_boxed_free (CAIRO_GOBJECT_TYPE_SURFACE, icon);

Why not having a vfunc for this as well, instead of this hack to make it transfer none? (Or any reason not to make it transfer full instead?)
Comment 24 Cosimo Cecchi 2016-12-18 23:23:25 UTC
Review of attachment 341742 [details] [review]:

::: libgd/gd-main-box-child.c
@@ +76,3 @@
+
+  if (item != NULL)
+    g_object_unref (item);

Same comment as in the previous patch
Comment 25 Cosimo Cecchi 2016-12-18 23:28:16 UTC
Review of attachment 341743 [details] [review]:

::: libgd/gd-main-box-generic.c
@@ +85,3 @@
+          id = gd_main_box_item_get_id (item);
+	  if (g_strcmp0 (id, last_selected_id) == 0)
+            other_index = (gint) i;

I would prefer an explicit break here, instead of nesting the condition in the loop line

@@ +259,3 @@
+
+  if (model != NULL)
+    g_object_unref (model);

Same comment as the other two patches
Comment 26 Cosimo Cecchi 2016-12-18 23:29:09 UTC
Review of attachment 341744 [details] [review]:

Looks good
Comment 27 Cosimo Cecchi 2016-12-18 23:35:29 UTC
Review of attachment 341745 [details] [review]:

::: libgd/gd-main-icon-box-child.c
@@ +44,3 @@
+enum
+{
+  BUTTON_RELEASED,

Perhaps this should be called "clicked" instead?

@@ +271,3 @@
+  GObjectClass *oclass = G_OBJECT_CLASS (klass);
+  GtkWidgetClass *wclass = GTK_WIDGET_CLASS (klass);
+  GdkModifierType activate_modifiers[] = { GDK_SHIFT_MASK, GDK_CONTROL_MASK, GDK_SHIFT_MASK | GDK_CONTROL_MASK };

This looks unused
Comment 28 Cosimo Cecchi 2016-12-18 23:44:05 UTC
Review of attachment 341747 [details] [review]:

Looks good
Comment 29 Cosimo Cecchi 2016-12-18 23:44:35 UTC
Review of attachment 341749 [details] [review]:

Looks good
Comment 30 Cosimo Cecchi 2016-12-18 23:47:52 UTC
Review of attachment 341748 [details] [review]:

::: libgd/gd-main-box.c
@@ +195,3 @@
+  priv = gd_main_box_get_instance_private (self);
+
+  G_OBJECT_CLASS (gd_main_box_parent_class)->finalize (obj);

Remove this, since it's empty
Comment 31 Cosimo Cecchi 2016-12-19 00:28:08 UTC
Review of attachment 341746 [details] [review]:

::: libgd/gd-main-icon-box.c
@@ +81,3 @@
+  item = gd_main_box_child_get_item (child);
+  id = gd_main_box_item_get_id (item);
+  priv->last_selected_id = g_strdup (id);

Consider comparing the ids before updating the property.

@@ +207,3 @@
+    return;
+
+  g_clear_pointer (&priv->last_selected_id, g_free);

Consider making gd_main_icon_box_update_last_selected_id() work if passed a NULL child to accomplish this.

@@ +279,3 @@
+        priv->key_shift_pressed = TRUE;
+
+      priv->key_pressed = TRUE;

Where is this reset to false?

@@ +305,3 @@
+  if (!priv->selection_mode &&
+      (event->button == 1 && (event->state & GDK_CONTROL_MASK) != 0 ||
+       event->button == 3))

Consider using GDK_BUTTON_PRIMARY/etc instead of numbers for buttons

@@ +315,3 @@
+      if (event->button == 1)
+        {
+          if (!initiating && (event->state & GDK_SHIFT_MASK) != 0)

Consider factoring out this condition into a variable, since it's not immediately obvious what the condition represents.

@@ +419,3 @@
+
+static gboolean
+gd_main_icon_box_focus (GtkWidget *widget, GtkDirectionType direction)

Why is this method override needed? focus seems unrelated to the selection event handling you do elsewhere.

@@ +581,3 @@
+      g_signal_emit_by_name (self, "selection-changed");
+      priv->selection_changed = FALSE;
+    }

You could factor this out in a separate helper, since it's used a couple of times.

@@ +594,3 @@
+  G_OBJECT_CLASS (gd_main_icon_box_parent_class)->constructed (obj);
+
+  gtk_flow_box_set_selection_mode (GTK_FLOW_BOX (self), GTK_SELECTION_NONE);

Can't you do this in init?
Comment 32 Debarshi Ray 2016-12-19 06:31:12 UTC
(In reply to Cosimo Cecchi from comment #23)
> Review of attachment 341741 [details] [review] [review]:
> 
> ::: libgd/gd-main-box-item.c
> @@ +206,3 @@
> +
> +  if (icon != NULL)
> +    g_boxed_free (CAIRO_GOBJECT_TYPE_SURFACE, icon);
> 
> Why not having a vfunc for this as well, instead of this hack to make it
> transfer none? (Or any reason not to make it transfer full instead?)

(In reply to Cosimo Cecchi from comment #24)
> Review of attachment 341742 [details] [review] [review]:
> 
> ::: libgd/gd-main-box-child.c
> @@ +76,3 @@
> +
> +  if (item != NULL)
> +    g_object_unref (item);
> 
> Same comment as in the previous patch

Getters are usually transfer none, which is why didn't go for transfer full. However, it is harder to use g_object_get with 'gchar *' properties without involving a needless malloc/free, and we can have lots of GdMainBoxItems in an application. So I went for the vfunc to avoid the needless malloc/free whenever possible.

The ref-counted properties are easier to handle with just g_object_get and I saw that interfaces in gtk+ also use the unref hack before returning it from the getter. So, I went for it. We can add a vfunc to avoid the hack, but it would add a few extra lines in the implementation classes.

No strong opinion, though. :) Upto you.
Comment 33 Debarshi Ray 2016-12-19 15:03:46 UTC
(In reply to Cosimo Cecchi from comment #27)
> Review of attachment 341745 [details] [review] [review]:
> 
> ::: libgd/gd-main-icon-box-child.c
> @@ +44,3 @@
> +enum
> +{
> +  BUTTON_RELEASED,
> 
> Perhaps this should be called "clicked" instead?

Yes, that sounds better.

Luckily, mclasen has agreed in principle to add gtk_flow_box_get_child_at_pos to gtk3 (bug 776187). So this workaround won't be necessary anymore.

> @@ +271,3 @@
> +  GObjectClass *oclass = G_OBJECT_CLASS (klass);
> +  GtkWidgetClass *wclass = GTK_WIDGET_CLASS (klass);
> +  GdkModifierType activate_modifiers[] = { GDK_SHIFT_MASK,
> GDK_CONTROL_MASK, GDK_SHIFT_MASK | GDK_CONTROL_MASK };
> 
> This looks unused

Oops. That's right.
Comment 34 Debarshi Ray 2016-12-19 19:30:21 UTC
(In reply to Cosimo Cecchi from comment #25)
> Review of attachment 341743 [details] [review] [review]:
> 
> ::: libgd/gd-main-box-generic.c
> @@ +85,3 @@
> +          id = gd_main_box_item_get_id (item);
> +	  if (g_strcmp0 (id, last_selected_id) == 0)
> +            other_index = (gint) i;
> 
> I would prefer an explicit break here, instead of nesting the condition in
> the loop line

Done. Also added a g_object_unref to go with the break.
Comment 35 Debarshi Ray 2016-12-19 19:30:58 UTC
(In reply to Cosimo Cecchi from comment #30)
> Review of attachment 341748 [details] [review] [review]:
> 
> ::: libgd/gd-main-box.c
> @@ +195,3 @@
> +  priv = gd_main_box_get_instance_private (self);
> +
> +  G_OBJECT_CLASS (gd_main_box_parent_class)->finalize (obj);
> 
> Remove this, since it's empty

Removed.
Comment 36 Debarshi Ray 2016-12-19 19:39:11 UTC
(In reply to Cosimo Cecchi from comment #31)
> Review of attachment 341746 [details] [review] [review]:
> 
> ::: libgd/gd-main-icon-box.c
> @@ +81,3 @@
> +  item = gd_main_box_child_get_item (child);
> +  id = gd_main_box_item_get_id (item);
> +  priv->last_selected_id = g_strdup (id);
> 
> Consider comparing the ids before updating the property.

Of course. Done.

> @@ +207,3 @@
> +    return;
> +
> +  g_clear_pointer (&priv->last_selected_id, g_free);
> 
> Consider making gd_main_icon_box_update_last_selected_id() work if passed a
> NULL child to accomplish this.

Good idea. Done.
 
> @@ +279,3 @@
> +        priv->key_shift_pressed = TRUE;
> +
> +      priv->key_pressed = TRUE;
> 
> Where is this reset to false?

Nowhere. Thanks for catching it.

> @@ +305,3 @@
> +  if (!priv->selection_mode &&
> +      (event->button == 1 && (event->state & GDK_CONTROL_MASK) != 0 ||
> +       event->button == 3))
> 
> Consider using GDK_BUTTON_PRIMARY/etc instead of numbers for buttons

Done.

> @@ +315,3 @@
> +      if (event->button == 1)
> +        {
> +          if (!initiating && (event->state & GDK_SHIFT_MASK) != 0)
> 
> Consider factoring out this condition into a variable, since it's not
> immediately obvious what the condition represents.

Good point. Instead of a variable, I ended up going with a comment to match the others.

> @@ +419,3 @@
> +
> +static gboolean
> +gd_main_icon_box_focus (GtkWidget *widget, GtkDirectionType direction)
> 
> Why is this method override needed? focus seems unrelated to the selection
> event handling you do elsewhere.

This is to deal with TAB. Without the override, if you TAB through GdMainIconBox, it will wipe out any existing selection. ie., the selection will move with the TAB's focus.

I noticed that we don't need to mess with the SHIFT modifier here - only CONTROL. I have fixed that now.

> @@ +581,3 @@
> +      g_signal_emit_by_name (self, "selection-changed");
> +      priv->selection_changed = FALSE;
> +    }
> 
> You could factor this out in a separate helper, since it's used a couple of
> times.

Yeah. Unfortunately, there are subtle differences that prevent us from unifying most of them. :( I am not too happy with the way we use priv->selection_changed.

> @@ +594,3 @@
> +  G_OBJECT_CLASS (gd_main_icon_box_parent_class)->constructed (obj);
> +
> +  gtk_flow_box_set_selection_mode (GTK_FLOW_BOX (self), GTK_SELECTION_NONE);
> 
> Can't you do this in init?

I can. It's a relic from gd-main-icon-view.c. Moved to _init.
Comment 37 Debarshi Ray 2016-12-19 19:42:21 UTC
I have implemented drag-and-drop now (using the patches from bug 776187). I am pushing everything to libgd:wip/rishi/main-box as before. I will re-attach the patches after some more testing.
Comment 38 Debarshi Ray 2016-12-19 19:51:48 UTC
I don't see a way to retain the existing rubberband selection behaviour where the rubberband never wipes out any existing selection. That's because it is all baked into a GtkGestureDrag inside gtk/gtkflowbox.c. I don't know how to override it.

Other than that, I think all the features are in place now.

I am wondering if we should somehow share gd_main_view_get_counter_icon copy_surface from gd-main-view.c. Right now I copy-pasted them to gd-main-icon-box.c. They are not at all tied to GdMain*, but it might only be used by those widgets. What do you think?
Comment 39 Cosimo Cecchi 2016-12-20 22:28:00 UTC
(In reply to Debarshi Ray from comment #32)
> Getters are usually transfer none, which is why didn't go for transfer full.
> However, it is harder to use g_object_get with 'gchar *' properties without
> involving a needless malloc/free, and we can have lots of GdMainBoxItems in
> an application. So I went for the vfunc to avoid the needless malloc/free
> whenever possible.
> 
> The ref-counted properties are easier to handle with just g_object_get and I
> saw that interfaces in gtk+ also use the unref hack before returning it from
> the getter. So, I went for it. We can add a vfunc to avoid the hack, but it
> would add a few extra lines in the implementation classes.
> 
> No strong opinion, though. :) Upto you.

I see the reasoning behind it, but since we are already having so many vfuncs, I don't believe that adding a couple more would make the code less maintainable -- and at the same time it becomes easier to understand the flow. So I would be in favor adding the other vfuncs too :-)

(In reply to Debarshi Ray from comment #38)
> I am wondering if we should somehow share gd_main_view_get_counter_icon
> copy_surface from gd-main-view.c. Right now I copy-pasted them to
> gd-main-icon-box.c. They are not at all tied to GdMain*, but it might only
> be used by those widgets. What do you think?

Yeah, I would not mind to move those to gd-icon-utils and make this code set the gtk-hacks flag upon build.
Comment 40 Debarshi Ray 2016-12-21 07:48:09 UTC
(In reply to Cosimo Cecchi from comment #39)
> (In reply to Debarshi Ray from comment #32)
> > Getters are usually transfer none, which is why didn't go for transfer full.
> > However, it is harder to use g_object_get with 'gchar *' properties without
> > involving a needless malloc/free, and we can have lots of GdMainBoxItems in
> > an application. So I went for the vfunc to avoid the needless malloc/free
> > whenever possible.
> > 
> > The ref-counted properties are easier to handle with just g_object_get and I
> > saw that interfaces in gtk+ also use the unref hack before returning it from
> > the getter. So, I went for it. We can add a vfunc to avoid the hack, but it
> > would add a few extra lines in the implementation classes.
> > 
> > No strong opinion, though. :) Upto you.
> 
> I see the reasoning behind it, but since we are already having so many
> vfuncs, I don't believe that adding a couple more would make the code less
> maintainable -- and at the same time it becomes easier to understand the
> flow. So I would be in favor adding the other vfuncs too :-)

Ok, I will add the vfuncs.

> (In reply to Debarshi Ray from comment #38)
> > I am wondering if we should somehow share gd_main_view_get_counter_icon
> > copy_surface from gd-main-view.c. Right now I copy-pasted them to
> > gd-main-icon-box.c. They are not at all tied to GdMain*, but it might only
> > be used by those widgets. What do you think?
> 
> Yeah, I would not mind to move those to gd-icon-utils and make this code set
> the gtk-hacks flag upon build.

Ok.
Comment 41 Debarshi Ray 2016-12-21 07:49:06 UTC
Created attachment 342299 [details] [review]
icon-utils, main-view: Export the image surface copying code
Comment 42 Debarshi Ray 2016-12-21 08:18:13 UTC
Created attachment 342300 [details] [review]
icon-utils, main-view: Export the image surface copying code
Comment 43 Debarshi Ray 2016-12-21 08:18:32 UTC
Created attachment 342301 [details] [review]
icon-utils, main-view: Export the DnD counter creation code
Comment 44 Debarshi Ray 2016-12-21 08:19:13 UTC
Created attachment 342302 [details] [review]
Add GdMainBoxItem
Comment 45 Debarshi Ray 2016-12-21 08:19:38 UTC
Created attachment 342303 [details] [review]
Add GdMainBoxChild
Comment 46 Debarshi Ray 2016-12-21 08:20:01 UTC
Created attachment 342304 [details] [review]
Add GdMainBoxGeneric
Comment 47 Debarshi Ray 2016-12-21 08:20:29 UTC
Created attachment 342305 [details] [review]
Add GdMainIconBoxChild
Comment 48 Debarshi Ray 2016-12-21 08:21:49 UTC
Created attachment 342306 [details] [review]
Add GdMainIconBox
Comment 49 Debarshi Ray 2016-12-21 08:22:13 UTC
Created attachment 342308 [details] [review]
Add GdMainBox
Comment 50 Debarshi Ray 2016-12-21 08:32:57 UTC
Other than the changes you mentioned, I have:

(a) Dropped the workaround for bug 776012.

(b) Drag-and-drop works, as far as I can tell.

(c) Dropped the getter to expose the GdMainBoxGeneric widget to the user. It seems better to keep it as an implementation detail so that we can change the internal interfaces with impunity. We can reconsider if some application really needs it.

(d) Directly clicking the GtkCheckButton in a child will now select the entire child.


The problem with the rubberband selection wiping out the existing selection is still there.

I wonder if:

(a) It is problematic that we are not update the timestamp of our faked GdkEvents.

(b) We should make GtkListBox and GtkFlowBox more flexible in gtk4 so that we don't have to delicately side-step their default behaviour. Currently GdMainIconBox relies on the exact order in which things happen (eg., signals are emitted) inside GtkFlowBox, which seems fragile.
Comment 51 Debarshi Ray 2017-01-03 20:00:36 UTC
I have now added show-primary-text and show-secondary-text properties so that applications can optionally show or hide the GtkLabels. It is likely that we won't be using the labels in Photos, while other applications would want them.
Comment 52 Debarshi Ray 2017-01-03 20:01:25 UTC
Created attachment 342788 [details] [review]
icon-utils, main-view: Export the image surface copying code
Comment 53 Debarshi Ray 2017-01-03 20:01:48 UTC
Created attachment 342789 [details] [review]
icon-utils, main-view: Export the DnD counter creation code
Comment 54 Debarshi Ray 2017-01-03 20:02:11 UTC
Created attachment 342790 [details] [review]
Add GdMainBoxItem
Comment 55 Debarshi Ray 2017-01-03 20:02:34 UTC
Created attachment 342791 [details] [review]
Add GdMainBoxChild
Comment 56 Debarshi Ray 2017-01-03 20:02:50 UTC
Created attachment 342792 [details] [review]
Add GdMainBoxGeneric
Comment 57 Debarshi Ray 2017-01-03 20:03:16 UTC
Created attachment 342793 [details] [review]
build: Add a _box-common flag to LIBGD_INIT
Comment 58 Debarshi Ray 2017-01-03 20:03:47 UTC
Created attachment 342794 [details] [review]
Add GdMainIconBoxChild
Comment 59 Debarshi Ray 2017-01-03 20:04:05 UTC
Created attachment 342795 [details] [review]
Add GdMainIconBox
Comment 60 Debarshi Ray 2017-01-03 20:04:30 UTC
Created attachment 342796 [details] [review]
build: Add a main-icon-box flag to LIBGD_INIT
Comment 61 Debarshi Ray 2017-01-03 20:04:49 UTC
Created attachment 342797 [details] [review]
Add GdMainBox
Comment 62 Debarshi Ray 2017-01-03 20:05:05 UTC
Created attachment 342798 [details] [review]
build: Add a main-box flag to LIBGD_INIT
Comment 63 Debarshi Ray 2017-01-07 13:01:31 UTC
Created attachment 343082 [details] [review]
Add GdMainBoxItem
Comment 64 Debarshi Ray 2017-01-07 13:01:48 UTC
Created attachment 343083 [details] [review]
Add GdMainBoxChild
Comment 65 Debarshi Ray 2017-01-07 13:02:06 UTC
Created attachment 343084 [details] [review]
Add GdMainBoxGeneric
Comment 66 Debarshi Ray 2017-01-07 13:02:25 UTC
Created attachment 343085 [details] [review]
Add GdMainIconBoxChild
Comment 67 Debarshi Ray 2017-01-07 13:02:49 UTC
Created attachment 343086 [details] [review]
Add GdMainIconBox
Comment 68 Debarshi Ray 2017-01-07 13:03:02 UTC
Created attachment 343087 [details] [review]
Add GdMainBox
Comment 69 Debarshi Ray 2017-01-07 13:13:50 UTC
I tweaked the layout of the widgets a bit.

Earlier, if you had less than the number of children needed to scroll, the children would be vertically aligned in the centre of the box. We can't set valign=START on the children themselves because if the thumbnails have different aspect ratios, then each row will be aligned along the top edge.

Instead, I have set valign=START on the whole GdMainIconBox. To ensure that we don't lose the textured background, I set the style class on GdMainBox and had it render a background.
Comment 70 Debarshi Ray 2017-01-12 14:56:40 UTC
I learnt from ebassi and ramcq on IRC that cosimoc going to be AFK for a bit due to CES and holidays. Since we are getting close to the UI freeze for GNOME 3.24, I am going to hesitantly merge these patches, so that they can be exposed to a larger segment of users.

We can continue to fix and improve the code in master.
Comment 71 Cosimo Cecchi 2017-01-23 05:05:04 UTC
Review of attachment 343082 [details] [review]:

::: libgd/gd-main-box-item.h
@@ +41,3 @@
+  const gchar      * (* get_secondary_text)  (GdMainBoxItem *self);
+  cairo_surface_t  * (* get_icon)            (GdMainBoxItem *self);
+  gboolean           (* get_pulse)           (GdMainBoxItem *self);

It does not look like this is actually used by the getter
Comment 72 Cosimo Cecchi 2017-01-23 05:15:25 UTC
Review of attachment 343084 [details] [review]:

::: libgd/gd-main-box-generic.c
@@ +476,3 @@
+
+  model = gd_main_box_generic_get_model (self);
+  last_selected_id = gd_main_box_generic_get_last_selected_id (self);

This looks unused
Comment 73 Cosimo Cecchi 2017-01-23 05:20:58 UTC
Review of attachment 343085 [details] [review]:

::: libgd/gd-main-icon-box-child.c
@@ +60,3 @@
+
+  parent = gtk_widget_get_parent (GTK_WIDGET (self));
+  if (!GTK_IS_FLOW_BOX (parent))

Why would this happen?

@@ +292,3 @@
+  GdMainIconBoxChildPrivate *priv;
+
+  priv = gd_main_icon_box_child_get_instance_private (self);

This looks unused

@@ +320,3 @@
+  GdMainIconBoxChildPrivate *priv;
+
+  priv = gd_main_icon_box_child_get_instance_private (self);

This looks unused

@@ +351,3 @@
+{
+  GObjectClass *oclass = G_OBJECT_CLASS (klass);
+  GtkWidgetClass *wclass = GTK_WIDGET_CLASS (klass);

This looks unused
Comment 74 Cosimo Cecchi 2017-01-23 05:34:46 UTC
Review of attachment 343086 [details] [review]:

::: libgd/gd-main-icon-box.c
@@ +118,3 @@
+  GtkFlowBoxChild *child;
+
+  priv = gd_main_icon_box_get_instance_private (self);

This looks unused

@@ +151,3 @@
+  GList *selected_children;
+
+  priv = gd_main_icon_box_get_instance_private (self);

This looks unused

@@ +197,3 @@
+  GdMainIconBoxPrivate *priv;
+
+  priv = gd_main_icon_box_get_instance_private (self);

This looks unused

@@ +294,3 @@
+  GdMainIconBoxPrivate *priv;
+
+  priv = gd_main_icon_box_get_instance_private (self);

This looks unused

::: libgd/gd-main-icon-box.h
@@ +35,3 @@
+
+  /* signals */
+  gboolean  (* move_cursor)            (GdMainIconBox *self, GtkMovementStep step, gint count);

I don't think this is required?
Comment 75 Cosimo Cecchi 2017-01-23 05:40:32 UTC
Review of attachment 343087 [details] [review]:

::: libgd/gd-main-box.c
@@ +189,3 @@
+  height = gtk_widget_get_allocated_height (widget);
+  width = gtk_widget_get_allocated_width (widget);
+  gtk_render_background (context, cr, 0, 0, width, height);

You should probably also render a frame... But at that point, the class could just derive from GtkFrame instead.
Comment 76 Cosimo Cecchi 2017-01-23 05:42:06 UTC
(In reply to Debarshi Ray from comment #70)
> I learnt from ebassi and ramcq on IRC that cosimoc going to be AFK for a bit
> due to CES and holidays. Since we are getting close to the UI freeze for
> GNOME 3.24, I am going to hesitantly merge these patches, so that they can
> be exposed to a larger segment of users.
> 
> We can continue to fix and improve the code in master.

Sorry for the delay, Debarshi, and thanks for pushing this forward! I found some time for a quick review of the patches and left some minor comments. Feel free to fix them directly in master if you agree.
Comment 77 Debarshi Ray 2017-01-26 17:40:36 UTC
Created attachment 344337 [details] [review]
main-box-item: Remove unused virtual function pointer
Comment 78 Debarshi Ray 2017-01-26 17:40:56 UTC
Created attachment 344338 [details] [review]
main-box: Render a frame
Comment 79 Cosimo Cecchi 2017-01-26 18:56:08 UTC
Review of attachment 344337 [details] [review]:

OK
Comment 80 Cosimo Cecchi 2017-01-26 18:58:50 UTC
Review of attachment 344338 [details] [review]:

I would have not minded if instead we derived from GtkFrame, but this is OK in the meantime.
Comment 81 Debarshi Ray 2017-01-27 15:10:00 UTC
(In reply to Cosimo Cecchi from comment #80)
> Review of attachment 344338 [details] [review] [review]:
> 
> I would have not minded if instead we derived from GtkFrame, but this is OK
> in the meantime.

I chose not to inherit from GtkFrame because I didn't want to expose all the GtkFrame API to GdMainBox users. How about putting a GtkFrame inside the GdMainBox? Looks like GtkFrame also renders a background.
Comment 82 Debarshi Ray 2017-01-27 15:10:21 UTC
Created attachment 344425 [details] [review]
main-box: Enclose the view in a GtkFrame
Comment 83 Cosimo Cecchi 2017-01-27 17:23:10 UTC
Review of attachment 344425 [details] [review]:

I think you could still derive from GtkFrame and do something like GtkPlacesSidebar does (https://git.gnome.org/browse/gtk+/tree/gtk/gtkplacessidebar.h) to avoid exposing the widget type in the header at all. But this patch looks OK if you strongly disagree with that.
Comment 84 Debarshi Ray 2017-02-03 18:43:23 UTC
(In reply to Cosimo Cecchi from comment #83)
> Review of attachment 344425 [details] [review] [review]:
> 
> I think you could still derive from GtkFrame and do something like
> GtkPlacesSidebar does
> (https://git.gnome.org/browse/gtk+/tree/gtk/gtkplacessidebar.h) to avoid
> exposing the widget type in the header at all. But this patch looks OK if
> you strongly disagree with that.

Making the class final would still reveal the hierarchy in gtk-doc documentation. :/
Comment 85 Debarshi Ray 2017-02-17 17:12:32 UTC
Created attachment 346097 [details] [review]
main-box-generic: Remove unused variable and function call
Comment 86 Debarshi Ray 2017-02-17 17:18:25 UTC
(In reply to Cosimo Cecchi from comment #73)
> Review of attachment 343085 [details] [review] [review]:
> 
> ::: libgd/gd-main-icon-box-child.c
> @@ +60,3 @@
> +
> +  parent = gtk_widget_get_parent (GTK_WIDGET (self));
> +  if (!GTK_IS_FLOW_BOX (parent))
> 
> Why would this happen?

I did this as a fail-safe - in case we got a spurious GtkCheckButton::toggled emission before it was added to a GtkFlowBox.
Comment 87 Debarshi Ray 2017-02-17 17:19:11 UTC
Created attachment 346098 [details] [review]
main-icon-box-child: Remove unused variables
Comment 88 Cosimo Cecchi 2017-02-18 00:28:49 UTC
Review of attachment 346097 [details] [review]:

Yep
Comment 89 Cosimo Cecchi 2017-02-18 00:29:04 UTC
Review of attachment 346098 [details] [review]:

OK
Comment 90 Debarshi Ray 2017-03-04 09:54:14 UTC
Created attachment 347195 [details] [review]
main-icon-box: Remove unused variables
Comment 91 Cosimo Cecchi 2017-03-04 19:45:32 UTC
Review of attachment 347195 [details] [review]:

Looks good, thanks.
Comment 92 Debarshi Ray 2017-03-04 19:48:01 UTC
(In reply to Cosimo Cecchi from comment #74)
> ::: libgd/gd-main-icon-box.h
> @@ +35,3 @@
> +
> +  /* signals */
> +  gboolean  (* move_cursor)            (GdMainIconBox *self,
> GtkMovementStep step, gint count);
> 
> I don't think this is required?

It is necessary to override GtkWidgetClass->focus and GtkFlowBoxClass->move_cursor to make the selection mode behave like GdMainIconView.

Specifically, without GtkFlowBoxClass->move_cursor, if you navigate from one child to another in the selection mode, the selection will change and move to the child currently in focus. This was not how GdMainIconView behaved. There, the existing set of selected children won't change due to keyboard navigation.

I don't like these hacks. Especially the part where we create fake GdkEvents to work around the GtkFlowBox's behaviour. But I couldn't find a better way.

We should probably do something to improve the situation in gtk+ 4.x - either make GtkFlowBox behave more like GdMainIconBox, or add API to let sub-classes change its behaviour. Right now, GdMainIconBox is very fragile. It relies on specific GtkFlowBox signals and vfuncs being emitted in a specific order, and so on.
Comment 93 Debarshi Ray 2017-03-04 19:50:56 UTC
Comment on attachment 347195 [details] [review]
main-icon-box: Remove unused variables

Pushed to master.