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

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[u88484]

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.

Comment 1

[WildcatRay]

(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.

Comment 2

[-fullmetaljacket-]

also fwiw, i can also reproduce this on xp and windows seven.

Comment 4

[Dietrich Ayala (:dietrich)]

blocking for investigation.

Comment 5

[Matthew Turnbull [Bluefang]]

Also saw this on Linux

Comment 6

[Marco Bonardo [:mak]]

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.

Comment 7

[Marco Bonardo [:mak]]

(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.

Comment 8

[Marco Bonardo [:mak]]

(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.

Comment 9

[(not currently active) Ted Mielczarek]

(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.

Comment 10

[Dave Townsend [:mossop]]

This will impact everyone who upgrades from 3.0 to 3.5 right?

Comment 11

[Marco Bonardo [:mak]]

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.

Comment 12

[Marco Bonardo [:mak]]

also notice any user can set SmartBookmarksVersion to -1 and smart bookmarks won't never be created again if removed.

Comment 13

[Marco Bonardo [:mak]]

removing blocking since bug 428690 is only on trunk

Comment 14

[Dave Townsend [:mossop]]

(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.

Comment 15

[Marco Bonardo [:mak]]

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.

Comment 16

[Marco Bonardo [:mak]]

ops sorry i just noticed now that bug was wontfixed, i would have preferred not.

Comment 17

[Dave Townsend [:mossop]]

(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.

Comment 18

[Marco Bonardo [:mak]]

"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.

Comment 19

[Dave Townsend [:mossop]]

(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?

Comment 20

[Marco Bonardo [:mak]]

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.

Comment 21

[Marco Bonardo [:mak]]

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.

Comment 22

[Thomas Stache]

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...

Comment 23

[Marco Bonardo [:mak]]

(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.

Comment 24

[Marco Bonardo [:mak]]

Attachment: patch v1.0

Comment 25

[Dietrich Ayala (:dietrich)]

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

Comment 26

[Thomas Stache]

(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.

Comment 27

[Marco Bonardo [:mak]]

Attachment: patch v1.1

Comment 28

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/4c16e2029a2a

Comment 29

[Marco Bonardo [:mak]]

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.

Comment 30

[Marco Bonardo [:mak]]

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.

Comment 31

[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