421483 - Reorganize pre-populated smart bookmarks (add versioning)

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P2)

Description

[Alex Faaborg [:faaborg] (Firefox UX)]

Beltzner writes in email:

>what I'm thinking is:
>
>- put "Most Visited" on the Bookmark Toolbar as default
>- put "Recently Tagged", and "Recently Bookmarked" as defaults in
>the Bookmarks Menu

We are worried that the current mouse path to these subfolders is too tedious to make them actually useful.  Also, they are not currently very discoverable.

Comment 1

[Alex Faaborg [:faaborg] (Firefox UX)]

Requested blocking at least get a wanted, if not blocking.

Comment 2

[Mike Beltzner [:beltzner, not reading bugmail]]

Is there a dupe on this? I thought I'd filed ...

Comment 3

[Marco Bonardo [:mak]]

i'm going add versioning to queries.

We cannot remove Smart Bookmarks folder for old users for some reason:
1. We have not annotated it, so we should rely on a folder name
2. A user could have renamed it
3. A user could have added other queries/things inside it

So i'll simply recreate queries in the right place, beta users will have to manually delete their old Smart bookmarks folder

Other problems:
4. a user deletes one query, on version bump we should not recreate it
5. a user moves one query, on version bump we should recreate in the current folder

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to comment #3)
> So i'll simply recreate queries in the right place, beta users will have to
> manually delete their old Smart bookmarks folder

David, one more thing which should be mentioned on the beta user page?

Comment 5

[Marco Bonardo [:mak]]

Attachment: patch

reorganized smart bookmarks like requested, i still need to know exact position where we want them (will attach a screen)

notes:
- old smart bookmarks folder will last, user have to delete it manually
- added versioning, so in future we can replace them / add new ones and bump up version
- users can reset their smart bookmarks setting version = 0 (position will not be restored though, only title and uri, and missing ones will be recreated)
- in case of an upgrade, missing smart bookmarks will be recreated, while others will be replaced in the same position (so if a user has moved them they will not bounce to another place)

Comment 6

[Marco Bonardo [:mak]]

Attachment: screenshot - looking for position

Mike can you tell me where do we want them in the menu?

For toolbar i suppose that first position (like corrent smart bookmarks folder) is good

Comment 7

[Marco Bonardo [:mak]]

Attachment: much better

much cleaner patch, easy management.

While waiting for Beltzner suggestion on position of items, we can go on with review.

Comment 8

[Recall]

Marco, can you not have an icon for this instead. IMO it has a better name, but still looks ugly?

Comment 9

[Thomas Stache]

Typo:
prefBranch.setBoolPref("browser.places.smartBookmarksVersion", -1);
in BrowserGlue.js BrowserGlue.prototype

Comment 10

[Marco Bonardo [:mak]]

(In reply to comment #9)
> Typo:
> prefBranch.setBoolPref("browser.places.smartBookmarksVersion", -1);

thank you, should be Int

Comment 11

[Dietrich Ayala (:dietrich)]

Comment on attachment 313105 [details] [diff] [review]
much better


>+        // MOST VISITED
>+        var smart = {queryId: "MostVisited", // don't change this
>+                     itemId: null,
>+                     title: this._placesBundle.GetStringFromName("mostVisitedTitle"),
>+                     uri: this._uri("place:queryType=" +
>+                                    Ci.nsINavHistoryQueryOptions.QUERY_TYPE_HISTORY +
>+                                    "&sort=" +
>+                                    Ci.nsINavHistoryQueryOptions.SORT_BY_VISITCOUNT_DESCENDING +
>+                                    "&maxResults=" + MAX_RESULTS),
>+                     parent: bmsvc.toolbarFolder,
>+                     position: 0};
>+        smartBookmarks.push(smart);
>+
>+        // RECENLTY BOOKMARKED

typo: RECENTLY

>Index: browser/components/places/tests/unit/test_bookmarks_html.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/places/tests/unit/test_bookmarks_html.js,v
>retrieving revision 1.35
>diff -u -8 -p -r1.35 test_bookmarks_html.js
>--- browser/components/places/tests/unit/test_bookmarks_html.js	1 Apr 2008 18:26:09 -0000	1.35
>+++ browser/components/places/tests/unit/test_bookmarks_html.js	2 Apr 2008 16:42:12 -0000
>@@ -85,17 +85,17 @@ const LAST_CHARSET_ANNO = "URIProperties
> 
> // main
> function run_test() {
>   // get places import/export service
>   var importer = Cc["@mozilla.org/browser/places/import-export-service;1"].getService(Ci.nsIPlacesImportExportService);
> 
>   // avoid creating the places smart folder during tests
>   Cc["@mozilla.org/preferences-service;1"].getService(Ci.nsIPrefBranch).
>-  setBoolPref("browser.places.createdSmartBookmarks", true);
>+  setIntPref("browser.places.smartBookmarksVersion", 9999);

ew. can you please implement the pref so that -1 = rebuild, 0 = disabled, and used the disabled value in the tests?

also, please add a test that confirms the queries are created properly, as well as the prefs set properly.

finally, update the comment here: http://mxr.mozilla.org/seamonkey/source/browser/components/nsIBrowserGlue.idl#73

Comment 12

[Marco Bonardo [:mak]]

ok, so after chat, modifying to
-1: disable creation for users that don't want them
0: restore to current version

Comment 13

[Marco Bonardo [:mak]]

Attachment: patch v2, with unit test

implements new behaviour, plus dedicated unit test

Comment 14

[Marco Bonardo [:mak]]

PS: while working on unit test i found a typo in bookmarks idl... i think it does not deserve having a dedicated bug report so i've fixed it here:

-  void moveItem(in long long aFolder, in long long newParent, in long aIndex);
+  void moveItem(in long long aItemId, in long long newParent, in long aIndex);

Comment 15

[Marco Bonardo [:mak]]

Attachment: patch v2.1

humm rethinking to that since i've another bug open on moveItem i'm moving that fix there, it's more pertinent. Removed

Comment 16

[Dietrich Ayala (:dietrich)]

Comment on attachment 313591 [details] [diff] [review]
patch v2.1



>+        var smartBookmarkItemIds = annosvc.getItemsWithAnnotation(SMART_BOOKMARKS_ANNO, {});
>+        // set current itemId, parent and position if Smart Bookmark exists
>+        for each(var itemId in smartBookmarkItemIds) {
>+          var queryId = annosvc.getItemAnnotation(itemId, SMART_BOOKMARKS_ANNO);
>+          for (var i = 0; i < smartBookmarks.length; i++){
>+            if (smartBookmarks[i].queryId == queryId) {
>+              smartBookmarks[i].itemId = itemId;
>+              smartBookmarks[i].parent = bmsvc.getFolderIdForItem(itemId);
>+              smartBookmarks[i].position = bmsvc.getItemIndex(itemId);
>+              // remove current item, since it will be replaced
>+              bmsvc.removeItem(itemId);
>+              break;
>+            }
>+          }
>+          //XXX should we remove no more used Smart Bookmarks?

hrm, i don't really like this. what if a user customizes one of these queries? i think it might be a safer path to just add the new ones, since we shouldn't be doing this very often.

it's easy for a user to delete the duplicate item, but impossible for them to recover the one we deleted. (well, outside of doing a full bookmarks restore)

>+  // TEST 2: create smart bookmarks
>+  pref.setIntPref("browser.places.smartBookmarksVersion", 0);
>+  gluesvc.ensurePlacesDefaultQueriesInitialized();
>+  smartBookmarkItemIds = annosvc.getItemsWithAnnotation(SMART_BOOKMARKS_ANNO, {});
>+  do_check_neq(smartBookmarkItemIds.length, 0);

please explicitly check the amount

Comment 17

[Dietrich Ayala (:dietrich)]

(In reply to comment #16)
> (From update of attachment 313591 [details] [diff] [review])

> hrm, i don't really like this. what if a user customizes one of these queries?
> i think it might be a safer path to just add the new ones, since we shouldn't
> be doing this very often.

do need to remove the annotation from the old queries.

Comment 18

[Marco Bonardo [:mak]]

problem is when we have to change a query because the old version does not work anymore, for example when we changed TAG query to use RESULTS_AS_TAG_QUERY. But on the other side if the user has personalized the query (changing for example the max number of returned records) we don't know and could be lost. 
I could add a new property "forceUpdate: false" to a query to indicate if it should be replaced, this way a change that does not make the old query obsolete will simply add new query and remove anno from the old, one that makes old obsolete will replace it (since old would not work anymore).

Comment 19

[Marco Bonardo [:mak]]

(In reply to comment #16)
> >+  // TEST 2: create smart bookmarks
> >+  pref.setIntPref("browser.places.smartBookmarksVersion", 0);
> >+  gluesvc.ensurePlacesDefaultQueriesInitialized();
> >+  smartBookmarkItemIds = annosvc.getItemsWithAnnotation(SMART_BOOKMARKS_ANNO, {});
> >+  do_check_neq(smartBookmarkItemIds.length, 0);
> 
> please explicitly check the amount

so, when modifying smart bookmarks, we will have to update the test... i didn't do that for this reason

Comment 20

[Dietrich Ayala (:dietrich)]

(In reply to comment #18)
> problem is when we have to change a query because the old version does not work
> anymore, for example when we changed TAG query to use RESULTS_AS_TAG_QUERY. But
> on the other side if the user has personalized the query (changing for example
> the max number of returned records) we don't know and could be lost. 
> I could add a new property "forceUpdate: false" to a query to indicate if it
> should be replaced, this way a change that does not make the old query obsolete
> will simply add new query and remove anno from the old, one that makes old
> obsolete will replace it (since old would not work anymore).
> 

ugh, right. in those cases the user will end up with duplicates *and* brokenness, so lets stick with the current patch for now.

(In reply to comment #19)
> > please explicitly check the amount
> 
> so, when modifying smart bookmarks, we will have to update the test... i didn't
> do that for this reason
> 

ok, that's fine.

Comment 21

[Dietrich Ayala (:dietrich)]

(In reply to comment #18)
> I could add a new property "forceUpdate: false" to a query to indicate if it
> should be replaced, this way a change that does not make the old query obsolete
> will simply add new query and remove anno from the old, one that makes old
> obsolete will replace it (since old would not work anymore).
> 

i don't think adding this kind of hacky complexity is worth it given the edge-case nature of the issue.

Comment 22

[Marco Bonardo [:mak]]

instead we could do something similar to what has been done to avoid hardcoding roots... use a SMART_BOOKMARK_MAX_RESULTS const and translate it in the backend, providing an option to the user. there is another bug about that IIRC.

Comment 23

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 313092 [details]
screenshot - looking for position

Hm, a couple of things are wrong here.

First, Organize bookmarks somehow slipped beneath that first separator, it should be above it.

Second, I think we want a separator after these queries, as they're special.

So what I think we should do is:

 - move Organize Bookmarks above the first separator
 - insert the new queries at index=0
 - add a separator

The result, on a default profile, should look like:

Bookmark This Page
Subscribe to This Page
Bookmark All Tabs
Organize Bookmarks
-----------------------
Bookmarks Toolbar   >
-----------------------
Recently Bookmarked >
Recent Tags         >
-----------------------
Get Bookmark Add-ons
-----------------------
Mozilla Firefox     >

Comment 24

[Luke Iliffe (Harlequin99)]

Bug 427558 has just been filed to add a separator between Organize Bookmarks and Bookmark Toolbar. This would make the top of the menu look like:

Bookmark This Page
Subscribe to This Page
Bookmark All Tabs
-----------------------
Organize Bookmarks
-----------------------
Bookmarks Toolbar   >
-----------------------

This is an extension of bug 408938 and is mentioned in comment 9 of that bug.

Comment 25

[Marco Bonardo [:mak]]

thank you Luke, will address here

Comment 26

[Marco Bonardo [:mak]]

Attachment: screenshot - new position

Comment 27

[(mostly gone) XtC4UaLL [:xtc4uall]]

ermm, since you're about adding even more 'clutter' to the bookmars menu (in addition to the bookmarks toolbar folder), i strongly hope that the 'recently bookmarked' & 'recent tags' entries are

a) deletable in the menu
*and*
b) still being kept in the 'most visited' folder in the bookmarks toolbar if deleted in the menu

sorry about the negative tone, but i don't like this progression... .

Comment 28

[Marco Bonardo [:mak]]

(In reply to comment #27)
> ermm, since you're about adding even more 'clutter' to the bookmars menu (in
> addition to the bookmarks toolbar folder), i strongly hope that the 'recently
> bookmarked' & 'recent tags' entries are
> 
> a) deletable in the menu

Yes, deletable and movable

> b) still being kept in the 'most visited' folder in the bookmarks toolbar if
> deleted in the menu

You can move all queries to a single folder (like current Smart bookmarks) if you want. Most Visited is a query like the others, so you'll have to create a new folder and move all of them inside.

Comment 29

[Mike Beltzner [:beltzner, not reading bugmail]]

See earlier comments in this bug for the rationale for moving it. That rationale still stays. This is for the default view, users can always customize.

Comment 30

[(mostly gone) XtC4UaLL [:xtc4uall]]

ok, then you have my blessing for checkin ;)

Comment 31

[Marco Bonardo [:mak]]

Attachment: patch v2.2

Fixed menu position, removed annos on no more used Smart Bookmarks, create separator in bookmarks menu if we are creating Smart Bookmarks from ground up.
Asking a new review due to changes.

Comment 32

[Dietrich Ayala (:dietrich)]

Comment on attachment 314153 [details] [diff] [review]
patch v2.2


>+            }
>+            // We don't remove no more used Smart Bookmarks because user

"We don't remove the old Smart Bookmarks" is clearer I think.

r=me

Comment 33

[Marco Bonardo [:mak]]

Attachment: for check-in

Comment 34

[Marco Bonardo [:mak]]

NOTE for beta tester users. Old Smart Bookmarks folder should be manually removed, or you can move new queries inside it, but please delete old queries and retain newly created ones.

Comment 35

[Reed Loden [:reed]]

Checking in browser/components/nsBrowserGlue.js;
/cvsroot/mozilla/browser/components/nsBrowserGlue.js,v  <--  nsBrowserGlue.js
new revision: 1.86; previous revision: 1.85
done
Checking in browser/components/nsIBrowserGlue.idl;
/cvsroot/mozilla/browser/components/nsIBrowserGlue.idl,v  <--  nsIBrowserGlue.idl
new revision: 1.5; previous revision: 1.4
done
Checking in browser/components/places/tests/unit/test_384370.js;
/cvsroot/mozilla/browser/components/places/tests/unit/test_384370.js,v  <--  test_384370.js
new revision: 1.18; previous revision: 1.17
done
Checking in browser/components/places/tests/unit/test_bookmarks_html.js;
/cvsroot/mozilla/browser/components/places/tests/unit/test_bookmarks_html.js,v  <--  test_bookmarks_html.js
new revision: 1.37; previous revision: 1.36
done
Checking in browser/base/content/browser-menubar.inc;
/cvsroot/mozilla/browser/base/content/browser-menubar.inc,v  <--  browser-menubar.inc
new revision: 1.158; previous revision: 1.157
done
Checking in toolkit/components/places/src/nsNavHistory.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v  <--  nsNavHistory.cpp
new revision: 1.289; previous revision: 1.288
done
RCS file: /cvsroot/mozilla/browser/components/places/tests/unit/test_421483.js,v
done
Checking in browser/components/places/tests/unit/test_421483.js;
/cvsroot/mozilla/browser/components/places/tests/unit/test_421483.js,v  <--  test_421483.js
initial revision: 1.1
done

Comment 36

[Marco Bonardo [:mak]]

Attachment: additional fix

smartBookmarksVersion should be 0 if the pref is not found!

Comment 37

[Dietrich Ayala (:dietrich)]

Comment on attachment 314451 [details] [diff] [review]
additional fix

r=me for followup

Comment 38

[pal-moz]

Attachment: screentshot

"Most Visited" folder icon should be the same folder icon as "Recently ..." ?

old "smart bookmarks"
http://img241.imageshack.us/img241/7153/sb1jr9.jpg

Comment 39

[Dietrich Ayala (:dietrich)]

followup checked in:

Checking in browser/components/nsBrowserGlue.js;
/cvsroot/mozilla/browser/components/nsBrowserGlue.js,v  <--  nsBrowserGlue.js
new revision: 1.88; previous revision: 1.87
done

Comment 40

[Marco Bonardo [:mak]]

(In reply to comment #38)
> "Most Visited" folder icon should be the same folder icon as "Recently ..." ?

that feels like a problem in the toolbar view that is not appying special styling to queries... should be filled as a new bug imo

Comment 41

[Axel Hecht [:Pike]]

beltzner, this looks like moving a string from one place to another, in particular from one CSS constrained size to another CSS constrained size.

That is, different crop lengths apply to the existing string.

marcoos notes that this makes things easier for most locales, but I don't fully understand when which string moved from where to where.

If my analysis is roughly right, could we get an announcement in .l10n explaining what to look for and how to reproduce?

This is really subtle, but I know that people go for worse translations of items to not get them cropped with ellipses due to theme CSS, so I think there is some l10n impact involved.

Comment 42

[Axel Hecht [:Pike]]

Clint, this should show up in Minotaur reports, too.

Comment 43

[Axel Hecht [:Pike]]

Marek stepped up and posted http://groups.google.de/group/mozilla.dev.l10n/browse_thread/thread/2f405d181d7cd841.

Comment 44

[cmtalbert]

(In reply to comment #42)
> Clint, this should show up in Minotaur reports, too.
> 
I believe they will be visible there.  I've been following this bug, and will make certain of it when I dive into Minotaur to make it export the default protocol handler information later this week.

Comment 45

[Thomas Stache]

Marco or Dietrich, can you please clarify the behavior of this patch in the following scenario using Foxmarks bookmarks synchronization:
(I just went through it, but it will of course happen each time a user installs a new Firefox version that revs the smartBookmarksVersion)

1) Firefox on computer 1 runs smart bookmarks update, creates new queries, massages annotations, sets version pref to new value.

2) On computer 2 Foxmarks runs synchronization, before Firefox can be updated to the new build/release. Foxmarks syncs new queries to places.sqlite, but smartBookmarksVersion pref is of course not incremented.

3) New build/release runs for the first time, runs smart query migration, because version pref is not up-to-date, although the queries are up-to-date.

4) the new/current smart bookmarks are apparently duplicated.

Questions:
A) is it possible to access the stored annotations for a bookmark, to see what's going on here (SQLite Explorer)?
B) Assuming Foxmarks syncs annotations at all, are "foxmarks'" smart bookmarks "invalidated" by removing the annotations, and the new queries created again?

Comment 46

[Marco Bonardo [:mak]]

as you can see in the code we do:
var smartBookmarkItemIds = annosvc.getItemsWithAnnotation(SMART_BOOKMARKS_ANNO, {});
with
const SMART_BOOKMARKS_ANNO = "Places/SmartBookmark";

So when syncing you should bring with you that annotation, or we will not find any smart bookmark, and they will be rebuilt by zero in their default position.

Comment 47

[Thomas Stache]

So to me it looks like only the parent, position and itemID are preserved, the title, uri properties are carried over from the smartBookmarks array.

I can only report that after I ran through the smart bookmark update on two computers in roughly the sequence outlined in comment 45 I had all smart bookmarks duplicated: Most Visited on the toolbar, and Recent Tags, Recently Bookmarked in the menu.

Foxmarks for Firefox 3 is here: http://beta.foxmarks.com, in case QA or someone wants to try... it is a Recommended Extension on AMO.

Comment 48

[Thomas Stache]

Before I get on everybody's nerves:
using a third machine I confirmed that the Places/SmartBookmarks annotation doesn't make it to the other machines. I contacted Foxmarks about the issue.

Comment 49

[Marco Bonardo [:mak]]

(In reply to comment #48)
> I contacted Foxmarks about the issue.

thank you very much!

Comment 50

[cmtalbert]

(In reply to comment #42)
> Clint, this should show up in Minotaur reports, too.
> 
Axel, I've been testing this change with Minotaur, all the smart bookmark titles show up properly, though I can't tell if they have been clipped to ellipsis or not by the theme (as I get the values from an interface rather than screen scraping).

Minotaur also now reports the browser.places.smartBookmarksVersion preference, since I wasn't sure if you wanted that or not and it was easy to include.  

Let's take the Minotaur specific discussion to bug 396132 which is where the changes for this will land in the minotaur tool. (I'm rolling them in together with the protocol handler changes).

Comment 51

[Thomas Stache]

(In reply to comment #49)
> (In reply to comment #48)
> > I contacted Foxmarks about the issue.
> 
> thank you very much!
> 

My conversations with Todd Agulnick from Foxmarks tended towards changing nothing on Foxmarks side, the effort vs. benefit ratio of syncing the "hidden bookmark title" in the annotation seems not high enough for them.

However, we noticed that e.g. the "Most Visited" query is synced not as
"place:queryType=0&sort=8&maxResults=10" but as
"place:sort=8&maxResults=10"
They use nsINavHistoryResultNode.uri to read and nsINavBookmarksService.changeBookmarkURI to update bookmarks. Is that not the right way to do it?

Comment 52

[Marco Bonardo [:mak]]

(In reply to comment #51)
> My conversations with Todd Agulnick from Foxmarks tended towards changing
> nothing on Foxmarks side, the effort vs. benefit ratio of syncing the "hidden
> bookmark title" in the annotation seems not high enough for them.

That is not an hidden bookmark title, it's a string identifier to recognize a query from another.

> However, we noticed that e.g. the "Most Visited" query is synced not as
> "place:queryType=0&sort=8&maxResults=10" but as
> "place:sort=8&maxResults=10"
> They use nsINavHistoryResultNode.uri to read and
> nsINavBookmarksService.changeBookmarkURI to update bookmarks. Is that not the
> right way to do it?

IIRC querytype=0 should be the default, so the uri should work

Comment 53

[Thomas Stache]

(In reply to comment #52)
> > However, we noticed that e.g. the "Most Visited" query is synced not as
> > "place:queryType=0&sort=8&maxResults=10" but as
> > "place:sort=8&maxResults=10"
> > They use nsINavHistoryResultNode.uri to read and
> > nsINavBookmarksService.changeBookmarkURI to update bookmarks. Is that not the
> > right way to do it?
> 
> IIRC querytype=0 should be the default, so the uri should work
> 

Yeah, of course it does... but the user looks at the bookmarks on both his machines and expects to see the same properties. Why does the Places API and the Places Library expose different values for the URI? What property (of which interface) is fed into the Library tree?

Comment 54

[Al Billings [:abillings - ex-MoCo]]

I've verified that the pre-populated bookmarks have been moved as expected. This has been the case for a while but I checked specifically with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008050804 Minefield/3.0pre.

Comment 55

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Probably this caused bug 432323 (Trying to delete the "Smart Bookmarks" folder crashes Firefox [@ nsNavHistoryResult::OnItemChanged]).

Comment 56

[Marco Bonardo [:mak]]

(In reply to comment #55)
> Probably this caused bug 432323 (Trying to delete the "Smart Bookmarks" folder
> crashes Firefox [@ nsNavHistoryResult::OnItemChanged]).

i doubt, there's no code involving the old smart bookmarks folder, it simpy didn't get touched (indeed it had to be removed manually) and we create new queries. The crash could instead involve some relation between the toolbar view and a folder containing queries.

Comment 57

[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