498162 - left pane title check nulls anyone else using ORGANIZER_QUERY_ANNO

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[alta88]

Attachment: fix

i use this anno to allow folder icon setting via css, no reason to recreate that.  just a simple fix to ignore titles of items not in the organizer system.

Comment 1

[Drew Willcoxon :adw]

Comment on attachment 383130 [details] [diff] [review]
fix

Can you use a different annotation for your purposes?  Anyway, I'm not qualified to give a review here (and definitely not a superreview! :)  You want to ask Marco or Dietrich.

Comment 2

[alta88]

Comment on attachment 383130 [details] [diff] [review]
fix


well, the reason i use places is because it does 90% of what i need in a tree and would rather reuse than rewrite.  plus, not doing that check leaves one open to a broad default nulling which doesn't seem best practice.  btw, this is for the Labs snowl project.

Comment 3

[Nochum Sossonko [:Natch]]

Comment on attachment 383130 [details] [diff] [review]
fix

I'm not a reviewer, sorry.

Comment 4

[Marco Bonardo [:mak]]

I have some concern about the fact extensions should use ORGANIZER_QUERY_ANNO to decorate their entries.
If that could be done differently i would really largely prefer the alternative, since this potentially could bring to bugs when we find something we don't expect.

Any idea to properly obtain what you are trying to do?

Comment 5

[alta88]

there are 2 independent issues, i think.

1)the current code is a hacky solution to 'corruption' of titles, something that shouldn't happen and should be fixed at the root (and may be, and this is just leftovers).  plus, the solution of broadly nulling titles for all  the anno's itemIds not in this 'table' object is just not a good practice; the most likely manifestation of future bugs is when additional Organizer folders are added and all of a sudden you have null titles.

2)i'm not sure exactly what the concerns could really be, if an extension were to reuse an anno.  if some bug arises, like this one, it would be fixed, either here (where it makes sense) or in the extension.  to not use the existing anno, means i'd just create another anno and then have to override the nsITree cell code and add an Atom property just like you already do.  this, imo, is a very big waste of reusability and adds bloat, in general principal.

if you like, take a look at the latest snowl dev xpi for an idea of how Places is used (List view), here:
https://people.mozilla.com/%7Emyk/snowl/dist/snowl-dev-latest.xpi
any comments welcome via email.

Comment 6

[Marco Bonardo [:mak]]

(In reply to comment #5)
> there are 2 independent issues, i think.
> 
> 1)the current code is a hacky solution to 'corruption' of titles, something
> that shouldn't happen and should be fixed at the root (and may be, and this is
> just leftovers).

it is fixed at the root, this is to catch old problem.

>  plus, the solution of broadly nulling titles for all  the
> anno's itemIds not in this 'table' object is just not a good practice; the most
> likely manifestation of future bugs is when additional Organizer folders are
> added and all of a sudden you have null titles.

For how the code is built adding an organizer folder without adding the title to the "table" would be quite crazy.

> 2)i'm not sure exactly what the concerns could really be, if an extension were
> to reuse an anno.  if some bug arises, like this one, it would be fixed, either
> here (where it makes sense) or in the extension.

The problem is that we could use the anno to identify organizer queries and apply particular code paths to those. This doesn't happen today, could happen in future.

> means i'd just create another anno and then have to override the nsITree cell
> code and add an Atom property just like you already do.  this, imo, is a very
> big waste of reusability and adds bloat, in general principal.

indeed, i'm asking if we can change our code to make easier for extensions to hook icons, not to change your code.

Comment 7

[alta88]

(In reply to comment #6)
> 
> indeed, i'm asking if we can change our code to make easier for extensions to
> hook icons, not to change your code.

ah.  i would design it thus: create a new type of anno, perhaps called 'places/Properties', with associated apis.  the apis would be a wrapped version of setItemAnnotation and setPageAnnotation, as the anno value would contain a comma|space delimited string of any number of property tokens.  at nsITreeView time, the array of such properties for an itemId is returned/iterated and each is added as an atom.  the wrapper part would of course be addItemProperty, removeItemProperty, changeItemPropery to parse/get/set the tokens in the anno value string, and would be friendlier than the caller having to setAnno and figure out the string themselves.  

so: 
addItemProperty(LIVEMARKSid, "hasUnvisited")
removeItemProperty(LIVEMARKSid, "hasUnvisited")
changeItemProperty(BOOKMARKS_MENU, oldTitle, newTitle)
and associated css.

in general, the places tree has worked ok, the major work has been to fix up rt click and multiselection.  i would even give a kudo to whoever designed the tree contextmenu - very nice and flexible for reuse.  some other areas, however, are problematic re reuse, but that's OT.

Comment 8

[Marco Bonardo [:mak]]

this would mean, for each result node, get an annotation, loop through values and add atom. sounds like slower...
We should probably rely on some data we already know (something already available in node)

Comment 9

[alta88]

then a field 'properties' should be added to each bookmark record type (folder, shortcut, query, etc.) and to the result node.  note that the atom can be anything useful and would not necessarily be found in the current record.  ie, unVisited example above.  places (and my current use) only uses the title so far, but i am going to expand this for my purposes.

or is there already a free form string field and access apis to both the db and result node?

in fact, even for places, the code only adds the title atom and only for items with ORGANIZER_QUERY_ANNO, so this is rather restrictive for things you may want to do to enhance tree styling.

Comment 10

[Marco Bonardo [:mak]]

Comment on attachment 383130 [details] [diff] [review]
fix

>diff -r 4572679480d2 browser/components/places/content/utils.js
>--- a/browser/components/places/content/utils.js	Sat Jun 13 13:16:40 2009 -0700
>+++ b/browser/components/places/content/utils.js	Sat Jun 13 17:10:00 2009 -0600
>@@ -1197,17 +1197,18 @@ var PlacesUIUtils = {
>       this.leftPaneQueries = {};
>       var items = as.getItemsWithAnnotation(ORGANIZER_QUERY_ANNO, {});
>       // While looping through queries we will also check for titles validity.
>       for (var i = 0; i < items.length; i++) {
>         var queryName = as.getItemAnnotation(items[i], ORGANIZER_QUERY_ANNO);
>         this.leftPaneQueries[queryName] = items[i];
>         // Titles could have been corrupted or the user could have changed his
>         // locale.  Check title is correctly set and eventually fix it.
>-        if (bs.getItemTitle(items[i]) != queriesTitles[queryName])
>+        if (queriesTitles[queryName] &&

this check is bad because will skip PlacesRoot (or any other root) that has an empty title.

Comment 11

[alta88]

ah, of course, it should be:
+        if (queryName in queriesTitles &&

in any event, i'm just adding the title atom as an overload to getCellProperties, and have removed the anno.  

since i will need other props based on external data, i'll have to develop a custom scheme anyway.  so far it doesn't looke like reading an anno is slow at all, since it's just for each row on the visible list.  likely it would be more elegant/efficient to use propertyBag somehow but there aren't any usages in the Places codebase.

i would suggest that perhaps Places could just automatically add the node.title as an atom to all folders/shortcuts (rather than just those for that anno), as it seems useful that people might want to style their own folders..

Comment 12

[Marco Bonardo [:mak]]

each node has a propertyBag already, nobody ever used it and it is for temporary informations (not persisted)

If, by chance, you create something we could inherit or embed, let us know, we're always interested in enhancing extensions support in Places.

Comment 14

[Marco Bonardo [:mak]]

fixed with bug 525299

Comment 15

[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