Closed Bug 421483 Opened 16 years ago Closed 16 years ago

Reorganize pre-populated smart bookmarks (add versioning)

Categories

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

defect

Tracking

()

VERIFIED FIXED
Firefox 3

People

(Reporter: faaborg, Assigned: mak)

References

Details

Attachments

(4 files, 6 obsolete files)

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.
Requested blocking at least get a wanted, if not blocking.
Flags: blocking-firefox3?
Is there a dupe on this? I thought I'd filed ...
Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P2
Assignee: nobody → mak77
Whiteboard: dupeme
Target Milestone: --- → Firefox 3 beta5
Target Milestone: Firefox 3 beta5 → Firefox 3
Whiteboard: dupeme → [swag: 0.5d]
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
Status: NEW → ASSIGNED
(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?
Whiteboard: [swag: 0.5d] → [swag: 0.5d][possible TS regression]
Summary: Reorganize pre-populated smart bookmarks → Reorganize pre-populated smart bookmarks (add versioning)
Whiteboard: [swag: 0.5d][possible TS regression] → [swag: 0.5d][check for possible TS regression]
Attached patch patch (obsolete) — Splinter Review
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)
Attached image screenshot - looking for position (obsolete) —
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
Attachment #313092 - Flags: ui-review?(beltzner)
Whiteboard: [swag: 0.5d][check for possible TS regression] → [has patch][check for possible TS regression]
Attachment #313092 - Attachment is patch: false
Attachment #313092 - Attachment mime type: text/plain → image/png
Attached patch much better (obsolete) — Splinter Review
much cleaner patch, easy management.

While waiting for Beltzner suggestion on position of items, we can go on with review.
Attachment #313090 - Attachment is obsolete: true
Attachment #313105 - Flags: review?(dietrich)
Marco, can you not have an icon for this instead. IMO it has a better name, but still looks ugly?
Typo:
prefBranch.setBoolPref("browser.places.smartBookmarksVersion", -1);
in BrowserGlue.js BrowserGlue.prototype
(In reply to comment #9)
> Typo:
> prefBranch.setBoolPref("browser.places.smartBookmarksVersion", -1);

thank you, should be Int
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
Attachment #313105 - Flags: review?(dietrich) → review-
ok, so after chat, modifying to
-1: disable creation for users that don't want them
0: restore to current version
Whiteboard: [has patch][check for possible TS regression] → [has patch][needs UI-R beltzner]
Attached patch patch v2, with unit test (obsolete) — Splinter Review
implements new behaviour, plus dedicated unit test
Attachment #313105 - Attachment is obsolete: true
Attachment #313590 - Flags: review?(dietrich)
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);
Attached patch patch v2.1 (obsolete) — Splinter Review
humm rethinking to that since i've another bug open on moveItem i'm moving that fix there, it's more pertinent. Removed
Attachment #313590 - Attachment is obsolete: true
Attachment #313591 - Flags: review?(dietrich)
Attachment #313590 - Flags: review?(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
Attachment #313591 - Flags: review?(dietrich) → review-
(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.
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).
(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
(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.
Attachment #313591 - Flags: review- → review+
(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.
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 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     >
Attachment #313092 - Flags: ui-review?(beltzner) → ui-review-
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.
thank you Luke, will address here
Attachment #313092 - Attachment is obsolete: true
Attachment #314134 - Flags: ui-review?(beltzner)
Attachment #314134 - Flags: ui-review?(beltzner) → ui-review+
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... .
(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.

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.
ok, then you have my blessing for checkin ;)
Attached patch patch v2.2 (obsolete) — Splinter Review
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.
Attachment #313591 - Attachment is obsolete: true
Attachment #314153 - Flags: review?(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
Attachment #314153 - Flags: review?(dietrich) → review+
Attached patch for check-inSplinter Review
Attachment #314153 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: [has patch][needs UI-R beltzner] → [has patch]
Whiteboard: [has patch] → [has patch][has review][has ui-review]
Flags: in-testsuite?
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.
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
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [has patch][has review][has ui-review]
Attached patch additional fixSplinter Review
smartBookmarksVersion should be 0 if the pref is not found!
Attachment #314451 - Flags: review?(dietrich)
Flags: in-testsuite? → in-testsuite+
Comment on attachment 314451 [details] [diff] [review]
additional fix

r=me for followup
Attachment #314451 - Flags: review?(dietrich) → review+
Attached image screentshot
"Most Visited" folder icon should be the same folder icon as "Recently ..." ?

old "smart bookmarks"
http://img241.imageshack.us/img241/7153/sb1jr9.jpg
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
(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
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.
Clint, this should show up in Minotaur reports, too.
(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.
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?
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.
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.
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.
(In reply to comment #48)
> I contacted Foxmarks about the issue.

thank you very much!

(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).
(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?
(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
(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?
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.
Status: RESOLVED → VERIFIED
Probably this caused bug 432323 (Trying to delete the "Smart Bookmarks" folder crashes Firefox [@ nsNavHistoryResult::OnItemChanged]).
Blocks: 413693
(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.

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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: