Special collections don't persist as Dock menu selections across sessions

RESOLVED WONTFIX

Status

defect
RESOLVED WONTFIX
11 years ago
5 years ago

People

(Reporter: alqahira, Assigned: bugzilla-graveyard)

Tracking

Details

Attachments

(1 attachment)

STR:

1) Launch Camino
2) Set the Dock menu to Address Book
3) Verify by Dock inspection that Address Book is being used
4) Quit Camino
5) Restart Camino
6) Inspect the Dock menu

ER: Address Book names
AR: Top 10 bookmark names

There's a possible chicken-egg situation here with bug 314628 comment 10; the checkmark isn't persisting for any of the collections, so even if you "set" your Dock menu to Top 10, it's possible that we're not even persisting that and it only appears to work on next launch because it is the default we're falling back to.
(Assignee)

Updated

11 years ago
Hardware: Macintosh → All
(Assignee)

Comment 1

11 years ago
It looks to me like |setupSmartCollections| in BookmarkManager.mm

http://mxr.mozilla.org/mozilla/source/camino/src/bookmarks/BookmarkManager.mm#426

is totally rebuilding them every time, and thus discarding any |isDockMenu| state that was previously set.
(Assignee)

Comment 2

11 years ago
And in BookmarkFolder.mm's |writeNativeDictionary:|, we bail immediately for smart folders without ever writing any information about flags that might be set:

http://mxr.mozilla.org/mozilla/source/camino/src/bookmarks/BookmarkFolder.mm#1207

We obviously don't need to store the whole smart folder since we're rebuilding it anyway, but we probably ought to at least write out the flag(s) so we can use them again.
(Assignee)

Comment 3

11 years ago
Posted patch fix v1.0Splinter Review
Murph said he could mebbe take a look at this, so let's do it.

I officially hate reading and riting roperty lists.
Assignee: nobody → cl-bugs-new
Status: NEW → ASSIGNED
Attachment #342390 - Flags: review?
(Assignee)

Updated

11 years ago
Attachment #342390 - Flags: review? → review?(murph)
(Assignee)

Comment 4

11 years ago
Comment on attachment 342390 [details] [diff] [review]
fix v1.0

Really setting r? flag this time.
(Assignee)

Comment 5

11 years ago
A heads-up just in case it's something we want to be concerned about: bookmarks files that are modified by this patch will be semi-broken (no bookmarks menu or bookmarks toolbar folder will be identified properly) in earlier versions of the browser. The bookmarks file can be imported manually and it'll work fine, but it's something to think about (and not to be alarmed about if you're one of the people who tests this and finds their toolbar bookmarks missing next them you boot a regular build).

Comment 6

11 years ago
Nice work here solving this problem cl!  As for the fix though, while it does make a special folder dock menu persist, I wonder if we could perhaps take an alternative approach.   The reason I say that is since, to make this work, I feel the patch is leaning towards being considered somewhat of a hack, in the sense that we're making the bookmarks code do too many special things to accomodate the storage of an empty smart folder.  The stored smart folder is not the actual populated one and is nothing more than an alternative placeholder, special cases are required in many places to handle it, and the fact that a kSmartFolderDummyChild object needs to be present is more indication of an approach trying to fight our current system.

I was wondering if it would work if we instead store the UUID of the current dock menu BookmarkFolder in NSUserDefaults, or somewhere in the Bookmarks.plist or similar (to keep the setting portable), and then just fetch the dock folder by searching by UUID instead of the whole special flag thing.

If we did this though, special folders do not have a constant UUID, a different one is returned during each Camino session.  What we could do for that is, in BookmarkFolder's initWithIdentifier: initializer, which is only used for special folders, set the UUID to the unique identifier of the special folder:

 - (id)initWithIdentifier:(NSString*)inIdentifier
 {
   if ([self init]) {
     mIdentifier = [inIdentifier retain];
>    [self setUUID:inIdentifier];
   }
   return self;
 }

The above is just a quick idea that came to mind, and I didn't have time to code up the entire approach, so maybe there's a good reason why we can't do it that way.  See what your thoughts are, and, if it doesn't sounds like a good plan, I'll definitely look over the code in your patch some more and post a formal review.
We also need to think about what to do when Dan adds hooks for remote third-party bookmark collections.

That said, it sounds like Sean just needs to r- this patch to get it out of the queue and then we should use his suggested approach instead.

[11:26pm] hendy: was just about to say that

Comment 8

10 years ago
Comment on attachment 342390 [details] [diff] [review]
fix v1.0

(In reply to comment #7)
> We also need to think about what to do when Dan adds hooks for remote
> third-party bookmark collections.
> 
> That said, it sounds like Sean just needs to r- this patch to get it out of the
> queue and then we should use his suggested approach instead.
> 
> [11:26pm] hendy: was just about to say that

I'm not actually sure why I didn't just go ahead with the r- when I made those suggestions, maybe because I was worried about coming too close to just taking over the bug myself...

Anyway, sorry about that, I do realize now that's the best approach for us here.
Attachment #342390 - Flags: review?(murph) → review-
This bug has been buried in the graveyard and has not been updated in over 5 years. It is probably safe to assume that it will never be fixed, so resolving as WONTFIX.

[Mass-change filter: graveyard-wontfix-2014-09-24]
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.