Created attachment 301478 [details] [review]
gtkplacessidebar: new bookmark row only at first position

Following the new mockups, put the new bookmark row always
in the first position, so it's easier to drop an item.

Created attachment 301479 [details] [review]
gtkplacessidebar: add API for emulate drag sources

It is convenient to allow applications to show all the drop
targets at once.
This improves the user experience with drag an drop.

Created attachment 301480 [details] [review]
gtkplacessidebar: add new bookmark icon

Following design mockups

Created attachment 301481 [details] [review]
gtkfilechooserwidget: show drop hints on gtkplacessidebar

Now with the API addition on gtkplacessidebar, we can show some
hints for the drop targets.

Review of attachment 301479 [details] [review]:

::: gtk/gtkplacessidebar.c
@@ +5285,3 @@
+ * has been finished.
+ *
+ * Since: 3.18

I don't like the name much. There's nothing being emulated here, really. We just want the sidebar to switch to a mode in which it shows its drop target.

Maybe gtk_places_sidebar_set_drop_target_visible (GtkPlacesSidebar, gboolean) ?

Review of attachment 301478 [details] [review]:

looks ok

Review of attachment 301480 [details] [review]:

::: gtk/gtkplacessidebar.c
@@ +1676,3 @@
   if (sidebar->drop_state == DROP_STATE_NORMAL)
     {
+      new_bookmark_icon =g_themed_icon_new ("bookmark-new-symbolic");

missing space after =

Review of attachment 301481 [details] [review]:

ok, modulo renaming of the api.

*** Bug 747641 has been marked as a duplicate of this bug. ***

one placessidebar annoyance we should fix while we are touching all this:

Make sure that we don't ellipsize the 'built-in' items, such as

Enter Location
New Bookmark
Browse Network

out of the box.

(In reply to Matthias Clasen from comment #8)
> Review of attachment 301481 [details] [review] [review]:
> 
> ok, modulo renaming of the api.

What does that mean?

(In reply to Matthias Clasen from comment #10)
> one placessidebar annoyance we should fix while we are touching all this:
> 
> Make sure that we don't ellipsize the 'built-in' items, such as
> 
> Enter Location
> New Bookmark
> Browse Network
> 
> out of the box.

To be honest, I though it's a good idea to stop using GtkTreeView (since we don't use a hierarchy at all) and port it to GtkListBox.

This will fix the hover state, css theming, eject button, etc.

So not sure if worth the effort to fix now some things. I would do it for 3.18.

Created attachment 301663 [details] [review]
gtkplacessidebar: make insensitive invalid drop targets

Created attachment 301664 [details] [review]
gtkplacessidebar: add color the new bookmark row

So it's more noticeable

The label rendering is a theme issue

Ditto, *was* a theme issue :-)

commit 149e7df6085dd13728e598f181a7a3221cd630ee
Author: Lapo Calamandrei <calamandrei@gmail.com>
Date:   Wed Apr 15 20:20:52 2015 +0200

    Adwaita: GtkPlacesSidebar insensitive items.
    
    Set the right colors there.

(In reply to Lapo Calamandrei from comment #16)
> Ditto, *was* a theme issue :-)
> 
> commit 149e7df6085dd13728e598f181a7a3221cd630ee
> Author: Lapo Calamandrei <calamandrei@gmail.com>
> Date:   Wed Apr 15 20:20:52 2015 +0200
> 
>     Adwaita: GtkPlacesSidebar insensitive items.
>     
>     Set the right colors there.

Ah, my patch included the fix for this, but your commit uses others colors that are probably more logical =)
I will commit without that part of the patch.

(In reply to Carlos Soriano from comment #11)
> (In reply to Matthias Clasen from comment #8)
> > Review of attachment 301481 [details] [review] [review] [review]:
> > 
> > ok, modulo renaming of the api.
> 
> What does that mean?

I meant that the name changes from the previous patch will need to be reflected here.

Review of attachment 301479 [details] [review]:

::: gtk/gtkplacessidebar.c
@@ +1833,3 @@
 
+static void
+drag_finalize (GtkPlacesSidebar *sidebar)

Not a fan of this name either - 'finalize' has a fairly specific meaning in gobject code bases.

Better to call this something else

Review of attachment 301663 [details] [review]:

Looks

::: gtk/gtkplacessidebar.c
@@ +1894,3 @@
+      stop_drop_feedback (sidebar);
+      update_possible_drop_targets (sidebar, FALSE);
+    }

Should update_possible_drop_targets just be called in stop_drop_feedback, perhaps ? ?

Review of attachment 301664 [details] [review]:

I have to try this again, I seem to remember that this felt a bit abrupt - a transition would be nicer, but that will have to wait for de-treeview-ification.

I'm also afraid that hardcoding black will not work great with a dark theme...

(In reply to Matthias Clasen from comment #21)
> Review of attachment 301664 [details] [review] [review]:
> 
> I have to try this again, I seem to remember that this felt a bit abrupt - a
> transition would be nicer, but that will have to wait for
> de-treeview-ification.
> 
> I'm also afraid that hardcoding black will not work great with a dark
> theme...

Yeah, it will come soon anyway.

What do you propose for the background then? If we have to take into account all themes possibilites... should I use css or something?

If you give me a style class I can take care of the recoloring in adwaita, as a bonus you can reuse it when it is de-treeviewified

Created attachment 302769 [details] [review]
gtkplacessidebar: new bookmark row only at first position

Following the new mockups, put the new bookmark row always
in the first position, so it's easier to drop an item.

Created attachment 302770 [details] [review]
gtkplacessidebar: add API for emulate drag sources

It is convenient to allow applications to show all the drop
targets at once.
This improves the user experience with drag an drop.

Created attachment 302771 [details] [review]
gtkplacessidebar: add new bookmark icon

Following design mockups

Created attachment 302772 [details] [review]
gtkplacessidebar: make insensitive invalid drop targets

Created attachment 302773 [details] [review]
patch

aplied the proposed changes and removed the patch for adding color to the bookmark, since seems it will be difficult to add a css style to a cellrenderer, and I don't think it would be very nice to have something custom in the code to directly lookup for a color in the css.

So instead, since I'm going to work on the port to GtkListBox starting tomorrow, it will be done at that time.

Review of attachment 302769 [details] [review]:

::: gtk/gtkplacessidebar.c
@@ +1681,3 @@
+
+  /* Add the row if it doesn't exists yet */
+  if (sidebar->drop_state == DROP_STATE_NORMAL)

Seems awkward to check the drop_state here...

@@ +1713,1 @@
       sidebar->drop_state = DROP_STATE_NEW_BOOKMARK_ARMED;

...but update it outside that function.

@@ +3897,3 @@
+    }
+
+  g_warning ("Index of bookmarks not calculable. No bookmarks heading");

The else branch here is totally redundant, and I don't think the warnings here add anything. I'd just drop them. This is basically a 'can't happen' condition, right ?

Review of attachment 302770 [details] [review]:

Still want those names changed...

Review of attachment 302771 [details] [review]:

sure

Review of attachment 302772 [details] [review]:

Please split the theme changes off into a separate commit.

::: gtk/gtkplacessidebar.c
@@ +1904,3 @@
+    {
+      stop_drop_feedback (sidebar);
+    }

No need for those {} here anymore

Review of attachment 302773 [details] [review]:

::: gtk/gtkfilechooserwidget.c
@@ +1656,3 @@
+                         GtkFileChooserWidget *impl)
+{
+  gtk_places_sidebar_drop_hints_start (GTK_PLACES_SIDEBAR (impl->priv->places_sidebar));

I like the function name better, but I don't think I've seen a patch that adds this function...

(In reply to Matthias Clasen from comment #34)
> Review of attachment 302773 [details] [review] [review]:
> 
> ::: gtk/gtkfilechooserwidget.c
> @@ +1656,3 @@
> +                         GtkFileChooserWidget *impl)
> +{
> +  gtk_places_sidebar_drop_hints_start (GTK_PLACES_SIDEBAR
> (impl->priv->places_sidebar));
> 
> I like the function name better, but I don't think I've seen a patch that
> adds this function...

The one you said: "I still want those names changed...."
It actually changed the names. Can you double check that review?

Review of attachment 302770 [details] [review]:

Also, please add the new api to docs/reference/gtk/gtk3-sections.txt at the suitable place.

::: gtk/gtkplacessidebar.h
@@ +139,1 @@
 

You need to rename them here, too.

(In reply to Matthias Clasen from comment #36)
> Review of attachment 302770 [details] [review] [review]:
> 
> Also, please add the new api to docs/reference/gtk/gtk3-sections.txt at the
> suitable place.
> 
> ::: gtk/gtkplacessidebar.h
> @@ +139,1 @@
>  
> 
> You need to rename them here, too.

argh yeah, I don't even know how it was working.

And sorry for the silly mistakes in the other patches as well...

Created attachment 302970 [details] [review]
gtkplacessidebar: new bookmark row only at first position

Following the new mockups, put the new bookmark row always
in the first position, so it's easier to drop an item.

Created attachment 302971 [details] [review]
gtkplacessidebar: add API for show drop hints

It is convenient to allow applications to show all the drop
targets at once. This improves the user experience with drag
an drop.

The new API allows the application to set the gtkplacessidebar
in a mode where invalid drop targets are insensitive and it
adds a "new bookmark" row. This mode is intended to be set
when the application is aware of a dnd operation and needs to
be stopped kwhen the application is aware that dnd operation
was cancelled or ended in a different part than gtkplacesisdebar.

Created attachment 302972 [details] [review]
gtkplacessidebar: make insensitive invalid drop targets

Created attachment 302974 [details] [review]
gtkplacessidebar: add API for show drop hints

It is convenient to allow applications to show all the drop
targets at once. This improves the user experience with drag
an drop.

The new API allows the application to set the gtkplacessidebar
in a mode where invalid drop targets are insensitive and it
adds a "new bookmark" row. This mode is intended to be set
when the application is aware of a dnd operation and needs to
be stopped kwhen the application is aware that dnd operation
was cancelled or ended in a different part than gtkplacesisdebar.

Review of attachment 302974 [details] [review]:

::: gtk/gtkplacessidebar.c
@@ +5382,3 @@
+ */
+void
+gtk_places_sidebar_drop_hints_start (GtkPlacesSidebar *sidebar)

I note that this is an unfortunate name too, because both 'drop' and 'start' are possible verbs :-(
Sorry for not noticing this last round.

void gtk_places_sidebar_set_drop_target_visible (GtkPlacesSidebar, gboolean)

is what I suggested earlier.
Or maybe:

gtk_places_sidebar_show_drop_hints
gtk_places_sidebar_hide_drop_gints

Or:

gtk_places_sidebar_set_drop_mode

Created attachment 303228 [details] [review]
gtkplacessidebar: add API for show drop hints

It is convenient to allow applications to show all the drop
targets at once. This improves the user experience with drag
an drop.

The new API allows the application to set the gtkplacessidebar
in a mode where invalid drop targets are insensitive and it
adds a "new bookmark" row. This mode is intended to be set
when the application is aware of a dnd operation and needs to
be stopped kwhen the application is aware that dnd operation
was cancelled or ended in a different part than gtkplacesisdebar.

Created attachment 303229 [details] [review]
gtkplacessidebar: make insensitive invalid drop targets

Created attachment 303230 [details] [review]
gtkfilechooserwidget: show drop hints on gtkplacessidebar

Now with the API addition on gtkplacessidebar, we can show some
hints for the drop targets.

You are right.

Just changed the API and attached the patches that slightly changed with your advice.

a little messy now - which order do they apply in ?

I can upload all of them again, is that better?

Created attachment 303260 [details] [review]
gtkplacessidebar: new bookmark row only at first position

Following the new mockups, put the new bookmark row always
in the first position, so it's easier to drop an item.

Created attachment 303261 [details] [review]
gtkplacessidebar: add API for show drop hints

It is convenient to allow applications to show all the drop
targets at once. This improves the user experience with drag
an drop.

The new API allows the application to set the gtkplacessidebar
in a mode where invalid drop targets are insensitive and it
adds a "new bookmark" row. This mode is intended to be set
when the application is aware of a dnd operation and needs to
be stopped kwhen the application is aware that dnd operation
was cancelled or ended in a different part than gtkplacesisdebar.

Created attachment 303262 [details] [review]
gtkplacessidebar: add new bookmark icon

Following design mockups

Created attachment 303263 [details] [review]
gtkplacessidebar: make insensitive invalid drop targets

Created attachment 303266 [details] [review]
gtkfilechooserwidget: show drop hints on gtkplacessidebar.patch

Looking at the branch again today. Here are a few comments from trying it briefly:

- I still think it is wrong to have drag highlighting on all the rows that don't accept the drop.

- Ellipsization kicks in much to early - there's a lot of unused space on the right side of the sidebar - maybe there's some room reserved for buttons and whatnot ? Looks wrong

I also get a crash when closing testfilechooser:

+ Trace 235135

When dragging from another application (nautilus) to the file chooser, the drag highlighting is not effective, since the filechooser is in backdrop state. We should tweak the theme to make drag highlights work in backdrop too

The drag highlighting change breaks drag highlighting pretty much everywhere except for in the sidebar. This will need more work in the theme if we really want to get rid of the cairo rectangle.

I'll also notice that we probably need the same workaround that you have for the treeview for the iconview.

Thanks Mathias.

"I still think it is wrong to have drag highlighting on all the rows that don't accept the drop."
I guess you speak about in the file chooser. I have been going back and forward on this. Given that we show drop hints before actually being above the sidebar widget, we cannot know all the details (like the dnd action the client wants to do). So either we show wrong drop hints in that case and correct them when the dnd source is above the sidebar widget, or we do as we do now.
I implemented the former now.

"Ellipsization kicks in much to early"
This is on pourpose, instead of hiding the eject button I make it transparent & insensitive, so the vertical alignment is preserved.
So either we don't preserve the vertical aligment, or we reduce paddings, or we accept the trade-off. I think reducing paddings is not a good option.

"The drag highlighting change breaks drag highlighting pretty much everywhere except for in the sidebar. This will need more work in the theme if we really want to get rid of the cairo rectangle."
I tried the dnd I know where it was used, so if you have more examples (like the iconview that I missed or the backdrop) please tell me. And sure, I want to get rid of it, although probably we will need to special case GtkTreeView and draw a rectangle.
So in gtkdnd:
if (GTK_IS_TREE_VIEW (widget))
draw rectangle
else
add css

Ok, comments fixed in wip/csoriano/bookmarks branch.

The only thing remaining as said before without solution I can think of is the "hints" when using gtk_sidebar_show_drop_hints and the drop target is not above the gtkplacesidebar widget, but I think we cannot do anything here, and probably is a minor issue since it corrects the "hints" when dragging above the gtkplacesidebar widget.

Could you take another round of overview review?

You've addressed my concerns from comment 54, thanks.

Here are a few more things I observed now. These can probably be addressed as fixups afterwards:

- When rearranging bookmarks, the drag icon looks a bit misshapen - the spacing between icon and label is lost

- In a small file chooser, it is possible that the only active drop target ('new bookmark') is scrolled off to the bottom, which makes for an awkward experience. We should at least autoscroll if I move the pointer close to the bottom edge during the drag. Or possibly even scroll the 'new bookmark' item into view when it is added.

Hi Mathias,

Adressed point 1.
I will defer point 2 for afterwards fixes, since the expected time for this work is becoming a little tight.

Also I fixed completely the problem with filechooser. Basically now drop hints are always correctly detected, even when the source is not above the sidebar widget (achieved sending the drag context to the placesidebar to ask for the action the source wants to perform with the drag).

Should I put the patches here for fine review now?

Only one further nit that I've discovered while looking at individual patches:

the drag highlight rectangle appears on the new bookmark row on hover, but it doesn't disappear as I move elsewhere in the sidebar. It should, I think. It does disappear if I move out of the sidebar altogether.

Sorry for pushing an unrelated fix onto your branch - accident.

Fixed

More detailed review:

- I get

warning: implicit declaration of function ‘glib_autoptr_cleanup_GtkListBoxRow’ [-Wimplicit-function-declaration]

seems to me you can't use G_DECLARE_TYPE and friends with a parent type thats manually declared ?

- GtkPlacesSidebarSectionType and GtkPlacesSidebarPlaceType should not go into public headers if they are not meant as public API (and I can't see them used in any api)

- Since gtksidebarrow.h is not public api, the convention is to name it gtksidebarrowprivate.h

fixed

I think this should be closed ?

whops yes