I am working on this in libgd:wip/rishi/main-box.

Created attachment 341629 [details] [review]
Add GdMainBoxItem

Created attachment 341630 [details] [review]
Add GdMainBoxChild

Created attachment 341631 [details] [review]
Add GdMainBoxGeneric

Created attachment 341632 [details] [review]
build: Add a _box-common flag to LIBGD_INIT

Created attachment 341633 [details] [review]
Add GdMainIconBoxChild

Created attachment 341634 [details] [review]
Add GdMainIconBox

Created attachment 341635 [details] [review]
build: Add a main-icon-box flag to LIBGD_INIT

Created attachment 341636 [details] [review]
Add GdMainBox

Created attachment 341637 [details] [review]
build: Add a main-box flag to LIBGD_INIT

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.

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.

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.

Created attachment 341741 [details] [review]
Add GdMainBoxItem

Created attachment 341742 [details] [review]
Add GdMainBoxChild

Created attachment 341743 [details] [review]
Add GdMainBoxGeneric

Created attachment 341744 [details] [review]
build: Add a _box-common flag to LIBGD_INIT

Created attachment 341745 [details] [review]
Add GdMainIconBoxChild

Created attachment 341746 [details] [review]
Add GdMainIconBox

Created attachment 341747 [details] [review]
build: Add a main-icon-box flag to LIBGD_INIT

Created attachment 341748 [details] [review]
Add GdMainBox

Created attachment 341749 [details] [review]
build: Add a main-box flag to LIBGD_INIT

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?)

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

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

Review of attachment 341744 [details] [review]:

Looks good

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

Review of attachment 341747 [details] [review]:

Looks good

Review of attachment 341749 [details] [review]:

Looks good

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

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?

(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.

(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.

(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.

(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.

(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.

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.

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?

(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.

(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.

Created attachment 342299 [details] [review]
icon-utils, main-view: Export the image surface copying code

Created attachment 342300 [details] [review]
icon-utils, main-view: Export the image surface copying code

Created attachment 342301 [details] [review]
icon-utils, main-view: Export the DnD counter creation code

Created attachment 342302 [details] [review]
Add GdMainBoxItem

Created attachment 342303 [details] [review]
Add GdMainBoxChild

Created attachment 342304 [details] [review]
Add GdMainBoxGeneric

Created attachment 342305 [details] [review]
Add GdMainIconBoxChild

Created attachment 342306 [details] [review]
Add GdMainIconBox

Created attachment 342308 [details] [review]
Add GdMainBox

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.

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.

Created attachment 342788 [details] [review]
icon-utils, main-view: Export the image surface copying code

Created attachment 342789 [details] [review]
icon-utils, main-view: Export the DnD counter creation code

Created attachment 342790 [details] [review]
Add GdMainBoxItem

Created attachment 342791 [details] [review]
Add GdMainBoxChild

Created attachment 342792 [details] [review]
Add GdMainBoxGeneric

Created attachment 342793 [details] [review]
build: Add a _box-common flag to LIBGD_INIT

Created attachment 342794 [details] [review]
Add GdMainIconBoxChild

Created attachment 342795 [details] [review]
Add GdMainIconBox

Created attachment 342796 [details] [review]
build: Add a main-icon-box flag to LIBGD_INIT

Created attachment 342797 [details] [review]
Add GdMainBox

Created attachment 342798 [details] [review]
build: Add a main-box flag to LIBGD_INIT

Created attachment 343082 [details] [review]
Add GdMainBoxItem

Created attachment 343083 [details] [review]
Add GdMainBoxChild

Created attachment 343084 [details] [review]
Add GdMainBoxGeneric

Created attachment 343085 [details] [review]
Add GdMainIconBoxChild

Created attachment 343086 [details] [review]
Add GdMainIconBox

Created attachment 343087 [details] [review]
Add GdMainBox

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.

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.

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

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

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

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?

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.

(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.

Created attachment 344337 [details] [review]
main-box-item: Remove unused virtual function pointer

Created attachment 344338 [details] [review]
main-box: Render a frame

Review of attachment 344337 [details] [review]:

OK

Review of attachment 344338 [details] [review]:

I would have not minded if instead we derived from GtkFrame, but this is OK in the meantime.

(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.

Created attachment 344425 [details] [review]
main-box: Enclose the view in a GtkFrame

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.

(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. :/

Created attachment 346097 [details] [review]
main-box-generic: Remove unused variable and function call

(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.

Created attachment 346098 [details] [review]
main-icon-box-child: Remove unused variables

Review of attachment 346097 [details] [review]:

Yep

Review of attachment 346098 [details] [review]:

OK

Created attachment 347195 [details] [review]
main-icon-box: Remove unused variables

Review of attachment 347195 [details] [review]:

Looks good, thanks.

(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 on attachment 347195 [details] [review]
main-icon-box: Remove unused variables

Pushed to master.