Carlos, can you fix this ?

(In reply to Matthias Clasen from comment #0)
> The commit 89a34210cb2804a0908ea91a9d38769cc3ab09b6 causes criticals and
> worse:
> 
> ./testfilechooserbutton 
> Gtk-Message: GtkDialog mapped without a transient parent. This is
> discouraged.
> 
> (lt-testfilechooserbutton:4949): GLib-GObject-WARNING **: invalid cast from
> 'GtkSeparator' to 'GtkSpinner'
> 
> (lt-testfilechooserbutton:4949): Gtk-CRITICAL **: gtk_spinner_stop:
> assertion 'GTK_IS_SPINNER (spinner)' failed
> 
> 
> 
> I also question why it introduces "is-network" as object data on rows, when
> GtkPlacesViewRow already has an is-network property.
The placeholder row is not a GtkPlacesViewRow, since the UI and data differs too much, we would need a new class for it or do some tricks on GtkPlacesViewRow, which I didn't think worth the hassel.
Even then, we want to mark this placeholder row as a is-network, so it gets ordered properly (and other placeholders rows, which at the moment we don't need, can follow the same way and they will just work).

Should I do it differently?

(In reply to Matthias Clasen from comment #1)
> Carlos, can you fix this ?

Yes, patch comming.

Created attachment 310125 [details] [review]
gtkplacesview: only filter out placeholder if searching

We were filtering out placeholders if the list box filters
while not searching, which is not what we want, since placeholders
should only be hidden if the view is searching.

Created attachment 310126 [details] [review]
gtkplacesview: avoid updating network status if local only

This shouldn't be a problem at all, although probably we don't
want to fetch the networks if not needed.
The problem relies on GtkListBox, which when filters because
the local only property changes, it modifies the row widgets
hierarchy, parenting and unparenting the header, depending on if
the associated row to the header is visible or not based on the
filtering.

That's a problem for this case, since we rely on some header widgets,
like a spinner on the network header, which becomes unusable after being
filtered and modify by the widgetry management of GtkListBox.

Proably what we want is GtkListBox to not modify at all the headers
widgets even if their associated row is filtered out, but we can figure
that out when we filter only using a model instead of GtkListBox
directly as its planned.

For now, just avoid to rely on the header widgetry if the row is
filtered out.

While these patches make the warning go away, I'm not convined that the handling of network_header_spinner is kosher now.

Instead of blaming GtkListView in the commit message, you should realize that it does not give you any guarantees about how often your header function is called for each row, and you can't just keep pointers to header ingredients around if you don't have any means of cleaning them up. At the very least, there needs to be a weak pointer or something, but I am not a fan of this side-effect-of-header_func setup altogether.

A few more observations:

- I've not been able to see the placeholder (or the spinner) at all here, even after killing all my network volumes&mounts, and slowing down the network:/// listing artificially - most likely because the placeholder is only ever created in update_network_state(), which only gets called after the enumeration is already done. Seems fishy - shouldn't you put the placeholder there before the enumeration gets going ?

- How is "loading" different from "fetching-networks" ? Why do we need two booleans here ? If fetching-networks was a property that would actually describe the state, an elegant solution to the header spinner issue might be to bind its 'active' property to 'fetching-networks' - then you wouldn't need to keep a pointer around to update the state manually, and that pointer would start dangling...

(In reply to Matthias Clasen from comment #6)
> While these patches make the warning go away, I'm not convined that the
> handling of network_header_spinner is kosher now.
> 
> Instead of blaming GtkListView in the commit message, you should realize
> that it does not give you any guarantees about how often your header
> function is called for each row, and you can't just keep pointers to header
> ingredients around if you don't have any means of cleaning them up. At the
> very least, there needs to be a weak pointer or something, but I am not a
> fan of this side-effect-of-header_func setup altogether.
> 
Not sure I understood the comment. Probably my commit message is not clear...
What I mean is, I realize there is no guarantees, at least when the list is filtered, but I was expecting that if GtkListBox allows you to set a header widget to it that the client has created, I don't expect GtkListBox to modify it.
Like if you bind a model, you don't expect GtkListBox to modify the model itself, as a client data, I was expecting them to be immutable by GtkListBox.
But I guess that, since there are plans to allow sorting and filtering directly on GListModel, this won't be something to discuss in future (also I'm probably wrong in my thinkering).

I'm not a fan either of the side-effect-of-header, but if was the solution I found better to reuse the actual header that is used when there is networks in the original bug report.
I will try to come with something different then.

> A few more observations:
> 
> - I've not been able to see the placeholder (or the spinner) at all here,
> even after killing all my network volumes&mounts, and slowing down the
> network:/// listing artificially - most likely because the placeholder is
> only ever created in update_network_state(), which only gets called after
> the enumeration is already done. Seems fishy - shouldn't you put the
> placeholder there before the enumeration gets going ?
> 

right, I removed the update_networks line at the start as a fallout of the last review in the original bug report, sorry for that.

> - How is "loading" different from "fetching-networks" ? Why do we need two
> booleans here ? If fetching-networks was a property that would actually
> describe the state, an elegant solution to the header spinner issue might be
> to bind its 'active' property to 'fetching-networks' - then you wouldn't
> need to keep a pointer around to update the state manually, and that pointer
> would start dangling...

The reason for this is that the view could take more to load than just for fetching networks. So for example, if searching was something different than just filtering, loading would be something like searching || fetching networks.
As you can imagine, this is used by the abstraction of Nautilus.
As you pointed, my intention was to actually describe the state, and binding would be much better, I agree. I will write a patch using a bind.

Created attachment 310285 [details] [review]
gtkplacesview: only filter out placeholder if searching

We were filtering out placeholders if the list box filters
while not searching, which is not what we want, since placeholders
should only be hidden if the view is searching.

Created attachment 310286 [details] [review]
gtkplacesview: create placeholder before fetching networks

If not we don't show the header at all.
This was an accidental removal before commiting.

Created attachment 310287 [details] [review]
gtkplacesview: don't rely on widgets on headers

We are showing a GtkSpinner on the networks header to provide feedback
to the user if we are fetching networks, therefore we have to modify
the spinner state when doing it.
However GtkListBox doesn't give guarantees about the widgets
set by gtk_list_box_set_header, and we could access an invalid
widget.

To avoid to access invalid widgets, bind the fetching networks
view property to the networks header spinner active property instead
of modifying directly the spinner in the private structure.

Not having the spinner in the private structure also makes the code
cleaner.

Created attachment 310288 [details] [review]
gtkplacesview: dont invalidate headers if not necessary

We were invalidating the headers after adding the placeholder
row, but GtkListBox already update the header of that row in that
case.

Created attachment 310289 [details] [review]
gtkplacesview: don't fetch networks if local only

We were fetching networks even on local only mode.
Avoid to do extra work if not necessary.

Created attachment 310290 [details] [review]
gtkplacesview: make consistent loading state

Until now the code was not very clear about why the loading property is
needed, since we didn't forced all the async operations to mark the
view as loading. This cause that clients are not aware when the view
is busy on those situations.

For instance Nautilus uses the property for a few things, one of it
is to show a busy spinner on the tab title.

To improve the situation, mark as loading when a volume operation,
a mount operation or a connect to server operation is being performed.

I attached a patch to fix the actual issue following your recommendation, a few more patches to try to fix/improve your observations, and a few more while I was on it.

Created attachment 310293 [details] [review]
gtkplacesview: don't warn for cancelled operations

This was intended to filter out cancelled operations, not
the other way around.

Review of attachment 310293 [details] [review]:

This one I already pushed, I believe

(In reply to Matthias Clasen from comment #16)
> Review of attachment 310293 [details] [review] [review]:
> 
> This one I already pushed, I believe

this is actually another one that was doing it wrong as well.

Review of attachment 310293 [details] [review]:

.

These all look good to me. Please push them so we have this for .91.

A few more questions though:

- You are calling update_places an awful lot - whenever anything changes, basically. And it throws away all your list contents and recreates it every time. Seems a bit wasteful, maybe. I haven't actually counted how often update_places gets called when you're just bringing up the file chooser - ideally, it would be only once, but I have my doubts.

- On the other hand, if you change local-only, update_places is not (directly, at least) called, and it is not clear to me how exactly fetch_networks() gets called in this case.

(In reply to Matthias Clasen from comment #19)
> These all look good to me. Please push them so we have this for .91.
> 
> A few more questions though:
> 
> - You are calling update_places an awful lot - whenever anything changes,
> basically. And it throws away all your list contents and recreates it every
> time. Seems a bit wasteful, maybe. I haven't actually counted how often
> update_places gets called when you're just bringing up the file chooser -
> ideally, it would be only once, but I have my doubts.
> 

Agree, was one of the points on the review. Luckily it's actually called "just" twice or so if there is no changes on volumes or so. Also to be fair, GtkPlacesSidebar has been doing the same since I know its code, and probably Georges based his code on it.
We could improve that and the sidebar for that matter.

> - On the other hand, if you change local-only, update_places is not
> (directly, at least) called, and it is not clear to me how exactly
> fetch_networks() gets called in this case.

Right, now that we only fetch networks if it is no local only, we have to fetch them and update the list in that case. I will add that to the "gtkplacesview: don't fetch networks if local only" patch

Attachment 310285 [details] pushed as a427f1e - gtkplacesview: only filter out placeholder if searching
Attachment 310286 [details] pushed as bdb17a0 - gtkplacesview: create placeholder before fetching networks
Attachment 310287 [details] pushed as 03cd8e9 - gtkplacesview: don't rely on widgets on headers
Attachment 310288 [details] pushed as 9fe9b78 - gtkplacesview: dont invalidate headers if not necessary
Attachment 310289 [details] pushed as 150bb62 - gtkplacesview: don't fetch networks if local only
Attachment 310290 [details] pushed as 0d93db7 - gtkplacesview: make consistent loading state
Attachment 310293 [details] pushed as 3cdf8fd - gtkplacesview: don't warn for cancelled operations