476952 - Places tree view's load method should set place attribute

Status: RESOLVED INACTIVE

(Firefox :: Bookmarks & History, defect)

Description

[Drew Willcoxon :adw]

Attachment: v1.0

There are two paths to updating a tree view's query: 1) calling the load method and 2) setting the place property.  The view's places attribute is not updated on path 1.  Path 2 goes through path 1, so the attribute update should be pushed to path 1.

The missed update of the place attribute screws up the workings of other properties of the view.

Comment 1

[Drew Willcoxon :adw]

Steps to reproduce one symptom of the problem:
1. Create a tree view with an empty query.
2. Use the tree view's load method to load a new non-empty query for display.
3. Toggle the tree view's showRoot property once or many times.

Actual results:
Nothing happens on step 3.

Expected results:
The view should be updated to display or hide the root node, depending on whether showRoot was toggled on or off.

Comment 2

[Drew Willcoxon :adw]

Attachment: v2.0

Added a chrome test case to browser/components/places/tests.

Comment 3

[Marco Bonardo [:mak]]

Comment on attachment 360778 [details] [diff] [review]
v2.0

Actually the advantage of having two different ways to populate a places tree is mainly based on the fact we have a place uri or query/options objects, and directly calling load avoids a serialization step.
With this patch the called code are practically the same: setting place property the queries are unserialized and then serialized again, while calling load now needs to do a serialization step.

On the other side we should sync the attribute with the actual query.

Personally i would prefer changing our code to always serialize queries and set place, making load an internal helper used by place setter, and changing docs to show place property as the only way to setup a Places tree view. The advantage is still lost (since load would go away) but at least the path would be more clear and less error prone.

Load has a small advantage by the fact you could load a temporary filtered view, and then clear it out simply forcing a rebuild (tree.place = tree.place).

Hwv, since i did not work on the original design, and Mano could know much more about that, i'm forwarding the review to him.

Comment 4

[Drew Willcoxon :adw]

(In reply to comment #3)
Making load internal is certainly one reasonable way to go, so I'll only comment on keeping it external, which seems to me to be the better option.

> Actually the advantage of having two different ways to populate a places tree
> is mainly based on the fact we have a place uri or query/options objects, and
> directly calling load avoids a serialization step.

The *outside caller* avoids the serialization step, which I thought was the whole point of the load method, a convenience for the caller.  This patch doesn't change that.

> With this patch the called code are practically the same: setting place
> property the queries are unserialized and then serialized again, while calling
> load now needs to do a serialization step.

If you're saying that this is a performance issue, then OK, but again, I would argue that the caller doesn't care so much whether the query is deserialized then serialized again.  In fact this seems like a necessary step to ensure that the query isn't gibberish.  The current code is happy to set the place attribute to any string you pass in to the place property setter.

> Personally i would prefer changing our code to always serialize queries and set
> place, making load an internal helper used by place setter, and changing docs
> to show place property as the only way to setup a Places tree view. The
> advantage is still lost (since load would go away) but at least the path would
> be more clear and less error prone.

Again, because the place setter relies on the load method, I would argue that this patch actually makes the path less error prone than before.

> Load has a small advantage by the fact you could load a temporary filtered
> view, and then clear it out simply forcing a rebuild (tree.place = tree.place).

This seems like voodoo to me, relying on the internal workings of the view (which in this case I would argue is a bug).

Comment 5

[Marco Bonardo [:mak]]

(In reply to comment #4)
> This seems like voodoo to me, relying on the internal workings of the view
> (which in this case I would argue is a bug).

yeah i agree, but i was simply guessing on possible reasons for the current approach

Comment 6

[Asaf Romano (gone)]

Comment on attachment 360778 [details] [diff] [review]
v2.0

I agree that load() should rather go away. Do note though that if "place" wasn't a property (i.e. if it was just an attribute), we wouldn't be commited to update it, at all.

Comment 7

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv

Comment 8

[BMO Automation]

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.