Closed Bug 484263 Opened 13 years ago Closed 13 years ago

Most visited, recently bookmarked and recent tags folder re-added when they are upgraded

Categories

(Firefox :: Bookmarks & History, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 3.6a1

People

(Reporter: u88484, Assigned: mak)

References

()

Details

(Whiteboard: [see comment 12 to opt-out from smart bookmarks upgrades])

Attachments

(1 file, 1 obsolete file)

The most visited, recently bookmarked and recent tags folders were re-added with the 20080318 nightly.  I deleted these folders a long, long time ago and I have no clue why they would have been re-added.

I actually did not get the most visited folder back but others on mozillazine have reported that they got all three folders back, some users didn't get any of them back and then others that already had those folders, got duplicates.

And if for some reason this is supposed to be by design when the version number changes...this needs fixed so Firefox is not shoving these down users throats trying to force them to use it.
Summary: Most visited, recently bookmarked and recent tags folder re-added with the 20080318 nightly → Most visited, recently bookmarked and recent tags folder re-added/dupliacted with the 20080318 nightly
(In reply to comment #0)
> The most visited, recently bookmarked and recent tags folders were re-added
> with the 20080318 nightly.  I deleted these folders a long, long time ago and I have no clue why they would have been re-added.
> 
> I actually did not get the most visited folder back but others on mozillazine
> have reported that they got all three folders back, some users didn't get any
> of them back and then others that already had those folders, got duplicates.
> 
> And if for some reason this is supposed to be by design when the version number changes...this needs fixed so Firefox is not shoving these down users throats trying to force them to use it.

FWIW-In my specific cases--this occurred on both of my PCs, a desktop and a notebook/laptop--I had moved the three folders to different locations within my bookmarks.
Summary: Most visited, recently bookmarked and recent tags folder re-added/dupliacted with the 20080318 nightly → Most visited, recently bookmarked and recent tags folder re-added/duplicated with the 20080318 nightly
Summary: Most visited, recently bookmarked and recent tags folder re-added/duplicated with the 20080318 nightly → Most visited, recently bookmarked and recent tags folder re-added/duplicated with the 20090318 nightly
also fwiw, i can also reproduce this on xp and windows seven.
Duplicate of this bug: 484366
blocking for investigation.
Flags: blocking-firefox3.5+
Whiteboard: [blocking for investigation]
Also saw this on Linux
Absolutely expected, if you remove smart bookmarks and in future we change them they are added back, exclusively for a reason, since the smart bookmark has changed could be no more the one you removed time ago, and you could be interested in having the new one. This is not a "you are forced to use them" but a "we have fixed them in some part, take a look if now they're better for your needs, or remove them again if still not", the update happens very rarely though, so is not a real annoyance.

The workaround is easy, remove them if they still are not satisfying you, you can.
(In reply to comment #1)
> FWIW-In my specific cases--this occurred on both of my PCs, a desktop and a
> notebook/laptop--I had moved the three folders to different locations within my
> bookmarks.

this is instead a bug, we should not move them, if they are in a different location, but simply update them in place.
Blocks: 428690
(In reply to comment #7)
> (In reply to comment #1)
> > FWIW-In my specific cases--this occurred on both of my PCs, a desktop and a
> > notebook/laptop--I had moved the three folders to different locations within my
> > bookmarks.
> 
> this is instead a bug, we should not move them, if they are in a different
> location, but simply update them in place.

this happens trhough annotations so could be your queries were missing them, if you use any sync service (foxmarks or weave) i really hope them sync annotations.
(In reply to comment #6)
> they are added back, exclusively for a reason, since the smart bookmark has
> changed could be no more the one you removed time ago, and you could be
> interested in having the new one. This is not a "you are forced to use them"
> but a "we have fixed them in some part, take a look if now they're better for
> your needs, or remove them again if still not"

I still find it annoying, although I guess the impact on non-nightly users is much reduced or nonexistent. I would be unhappy if we did this to our end-users, for sure, as regardless of intent it smacks of "we know better than you". The same way I get annoyed every time the QuickTime installer puts its stupid tray icon back on Windows on every single install. If I've made a choice to remove or disable some feature, the app should not override my explicit choice.
This will impact everyone who upgrades from 3.0 to 3.5 right?
yes on 1.9.1 would impact any upgrade. So there are 2 points here i think are important to understand:

- smart bookmarks version has changed: this means we have updated/fix them, we have added/removed any of them. It's important those changes get caught from everybody, since for example i could have removed "most visited" because it was full of stupid redirects/webmail addresses, then in a future version we change it to report typed uris, or use frecency, that it's a different "most visited" than what the user has removed.
In this case the change is that the new "most visited" does not show useless redirects anymore.

- smart bookmarks are managed and retrieved through annotations, if you use any extension that removes those annotation or does not sync them, there's nothing we can do to distiguish a smart bookmark from a saved search, so we can't detect if they still exist.
also notice any user can set SmartBookmarksVersion to -1 and smart bookmarks won't never be created again if removed.
removing blocking since bug 428690 is only on trunk
Flags: blocking-firefox3.5+
Whiteboard: [blocking for investigation]
Summary: Most visited, recently bookmarked and recent tags folder re-added/duplicated with the 20090318 nightly → Most visited, recently bookmarked and recent tags folder re-added when they are upgraded
OS: Windows Vista → All
Hardware: x86 → All
Whiteboard: [see comment 12 to opt-out from smart bookmarks upgrades]
(In reply to comment #11)
> - smart bookmarks version has changed: this means we have updated/fix them, we
> have added/removed any of them. It's important those changes get caught from
> everybody, since for example i could have removed "most visited" because it was
> full of stupid redirects/webmail addresses, then in a future version we change
> it to report typed uris, or use frecency, that it's a different "most visited"
> than what the user has removed.
> In this case the change is that the new "most visited" does not show useless
> redirects anymore.

The problem is that you are tracking the set of smart bookmarks with a single
version. This is wrong. It is true that if you add a new smart bookmark to the
default set then it is right to roll it out to everyone when they upgrade. It
is not true that when you modify an existing smart bookmark, as you have done
here, that you should recreate it when people have deleted it. If people have
deleted something then it should stay gone.

Proposing a preferences hack to allow people to keep the things they don't want to see from being shoved in their faces time and time again is not the right approach.
it's not a hack, see bug 399984, i agree we miss an UI and that bug covers that missing, but the right choice to opt-out from smart bookmarks upgrade is to set that pref, alternatively you only say "remove this specific query but upgrade me to future versions".

So what you say is that a removed smart bookmark should stay removed, but a new one should instead be added, so removing "most visited" and adding "most visited no redirects" is fine based on that logic, i would find that even more annoying.
For sure we can stop restoring a removed smart bookmark (posted that no add-on has played with annotations), users who removed a smart bookmark because was BOGUS won't even notice a new fixed version do exist.
ops sorry i just noticed now that bug was wontfixed, i would have preferred not.
(In reply to comment #15)
> So what you say is that a removed smart bookmark should stay removed, but a new
> one should instead be added, so removing "most visited" and adding "most
> visited no redirects" is fine based on that logic, i would find that even more
> annoying.

No, because in that case the two smart bookmarks are essentially the same (as far as the user is concerned). They are both just Most Visited. Even both called the same.
"essentially" is the key, but a working "most visited" would be "quite" different from our actual "most visited" (that is atm completely useless to me as to many other users).
Btw, i'll bring this topic to today's Places Status Meeting, and we'll discuss pro and cons, feel free to take part of it.
(In reply to comment #18)
> "essentially" is the key, but a working "most visited" would be "quite"
> different from our actual "most visited" (that is atm completely useless to me
> as to many other users).

Yet I'm assuming the existing upgrade code does replace the previous Most Visited folder with the new version? Or is the process to simply delete all existing smart bookmarks and re-create them, presumably not in the same place that a user may have moved them to?
they are all replaced in-place, that means if annotations have not been destroyed by anyone, they will not be moved from their actual location, but all of them are replaced.
We could add a version property to every smart bookmark to set which version has introduced that query, so we would only replace/recreate the changed ones, and this is my first idea that would solve the fact we update A and recreate B, would not solve if we upgrade A and recreate A, unless we want to simply not provide upgrade of queries to users who have removed an outdated smart bookmark.
taking this, provided that this won't fix bogus profiles who have their annotations stripped away, we won't restore a smart bookmark if it has been created on a previous version and is no more there, after that the only way for a user who has removed a smart bookmark to obtain the new version will be to play with about:config.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
FWIW, I already raised the issue that Foxmarks decided against annotations way back in bug 421483 comment 51. Back then every synced client re-added the smart bookmarks, while other versions were synced over as "new bookmarks".

Of course I don't know what - if anything - has changed since then...
(In reply to comment #22)
> FWIW, I already raised the issue that Foxmarks decided against annotations

we are using them for some useful task, i think you could at least provide a sort of annotations whitelist, to sync at least the most important one. we could provide a list of our internally used on a wiki page eventually.
Attached patch patch v1.0 (obsolete) — Splinter Review
Attachment #369298 - Flags: review?(dietrich)
Flags: in-testsuite?
Target Milestone: --- → Firefox 3.6a1
Comment on attachment 369298 [details] [diff] [review]
patch v1.0


>@@ -853,17 +853,18 @@ BrowserGlue.prototype = {
>                      itemId: null,
>                      title: placesBundle.GetStringFromName("mostVisitedTitle"),
>                      uri: this._uri("place:redirectsMode=" +
>                                     Ci.nsINavHistoryQueryOptions.REDIRECTS_MODE_TARGET +
>                                     "&sort=" +
>                                     Ci.nsINavHistoryQueryOptions.SORT_BY_VISITCOUNT_DESCENDING +
>                                     "&maxResults=" + MAX_RESULTS),
>                      parent: bmsvc.toolbarFolder,
>-                     position: bookmarksToolbarIndex++};
>+                     position: bookmarksToolbarIndex++,
>+                     newInVersion: 1 };
>         smartBookmarks.push(smart);

please document how newInVersion works in the comments for this method

>+// Initialize browserGlue.
>+let bg = Cc["@mozilla.org/browser/browserglue;1"].
>+         getService(Ci.nsIBrowserGlue);
>+
>+// Initialize Places.
>+var hs = Cc["@mozilla.org/browser/nav-history-service;1"].
>+         getService(Ci.nsINavHistoryService);
>+let bs = Cc["@mozilla.org/browser/nav-bookmarks-service;1"].
>+         getService(Ci.nsINavBookmarksService);
>+
>+// Get other services.
>+let ps = Cc["@mozilla.org/preferences-service;1"].
>+         getService(Ci.nsIPrefBranch);
>+let os = Cc["@mozilla.org/observer-service;1"].
>+         getService(Ci.nsIObserverService);

nit: why the mixed use of let and var? and let doesn't really seem to serve a purpose here since there's no obvious scope distinctions needed for these.

>+tests.push({
>+  description: "An existing smart bookmark is replaced when version changes.",
>+  exec: function() {
>+    // Sanity check: we have a smart bookmark on the toolbar.
>+    let itemId = bs.getIdForItemAt(bs.toolbarFolder, 0);
>+    do_check_neq(itemId, -1);
>+    // Change its title.
>+    bs.setItemTitle(itemId, "new title");
>+    do_check_eq(bs.getItemTitle(itemId), "new title");

i'd prefer a more robust check to ensure it's a smart bookmark

>+
>+    // Set preferences.
>+    ps.setIntPref(PREF_SMART_BOOKMARKS_VERSION, 1);
>+    // Force nsBrowserGlue::_initPlaces().
>+    os.notifyObservers(null, TOPIC_PLACES_INIT_COMPLETE, null);
>+
>+    // Count items on toolbar.
>+    do_check_eq(countFolderChildren(bs.toolbarFolder), SMART_BOOKMARKS_ON_TOOLBAR);
>+    // Count items on menu (+1 for the separator).
>+    do_check_eq(countFolderChildren(bs.bookmarksMenuFolder), SMART_BOOKMARKS_ON_MENU + 1);
>+
>+    // check smart bookmark has been replaced, itemId has changed.
>+    itemId = bs.getIdForItemAt(bs.toolbarFolder, 0);
>+    do_check_neq(bs.getItemTitle(itemId), "new title");
>+
>+    // Check version has been updated.
>+    do_check_true(ps.getIntPref(PREF_SMART_BOOKMARKS_VERSION) > 0);

why not check for the exact value that it's supposed to be?

(goes for the other tests as well)

>+
>+    next_test();
>+  }
>+});
>+
>+//------------------------------------------------------------------------------
>+
>+tests.push({
>+  description: "An explicitely removed smart bookmark should not be recreated.",

nit: explicitly

>+  exec: function() {   
>+    // Set preferences.
>+    ps.setIntPref(PREF_SMART_BOOKMARKS_VERSION, 1);
>+    // Remove toolbar's smart bookmarks
>+    bs.removeFolderChildren(bs.toolbarFolder);
>+
>+    // Force nsBrowserGlue::_initPlaces().
>+    os.notifyObservers(null, TOPIC_PLACES_INIT_COMPLETE, null);
>+
>+    // Count items on toolbar, we should not have recreated the smart bookmark.
>+    do_check_eq(countFolderChildren(bs.toolbarFolder), 0);
>+    // Count items on menu (+1 for the separator).
>+    do_check_eq(countFolderChildren(bs.bookmarksMenuFolder), SMART_BOOKMARKS_ON_MENU + 1);
>+
>+    // Check version has been updated.
>+    do_check_true(ps.getIntPref(PREF_SMART_BOOKMARKS_VERSION) > 0);
>+
>+    next_test();
>+  }
>+});
>+
>+//------------------------------------------------------------------------------
>+
>+tests.push({
>+  description: "Even if a a mart bookmark has been removed recreate it if version is 0.",

nit: s/a mart/smart/

r=me with these fixed
Attachment #369298 - Flags: review?(dietrich) → review+
(In reply to comment #23)
> (In reply to comment #22)
> > FWIW, I already raised the issue that Foxmarks decided against annotations
> 
> we are using them for some useful task, i think you could at least provide a
> sort of annotations whitelist, to sync at least the most important one. we
> could provide a list of our internally used on a wiki page eventually.

I hope you did not get the impression that I'm connected to Foxmarks, beside being a user. I would hope the Places team could establish and maintain some working relationships to popular Addon developers (nee API consumers), like Xmarks/Foxmarks, for the benefit of Firefox users.
Attached patch patch v1.1Splinter Review
Attachment #369298 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/4c16e2029a2a
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite? → in-testsuite+
Resolution: --- → FIXED
Comment on attachment 369392 [details] [diff] [review]
patch v1.1

asking approval, this avoids to recreate smart bookmarks on upgrade if they have been deleted by the user. no add-ons impact, low risk, has test.
Attachment #369392 - Flags: approval1.9.1?
Attachment #369392 - Flags: approval1.9.1?
Comment on attachment 369392 [details] [diff] [review]
patch v1.1

no need to rush on 1.9.1 for now, since we won't upgrade smart bookmarks in a minor release.
Depends on: 493557
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.