328737 - Destination should be consistently remembered when saving bookmarks

Status: RESOLVED FIXED

(Camino Graveyard :: Bookmarks, defect)

Description

[James Wheare]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.0.1) Gecko/20060226 Camino/1.0.0+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.0.1) Gecko/20060226 Camino/1.0.0+

Currently, when you bookmark a page by invoking Cmd-d, the value of the "Create in" drop down is not consistently remembered.

As is stated in bug 287708 comment 6, the state is remembered for the active Camino session, but if you quit and relaunch the app, the destination is forgotten.

There should be a persistent memory for this option since many people don't use the default "Boomarks Menu" destination.

Reproducible: Always

Steps to Reproduce:
1. Invoke cmd-d (Add page to bookmarks...) for your current page.
2. Choose a non-default value for the "Create in" drop down.
3. Add
4. Repeat and notice that the drop down value is remembered
5. Quit and relaunch Camino and repeat step 1
Actual Results:  
Value of "Create in" drop-down menu is not remembered from previous Camino session.

Expected Results:  
The bookmark destination should be persistently remembered between Camino sessions.

Comment 1

[Samuel Sidler (old account; do not CC)]

We remember during the same session. No reason to remember from previous sessions.

Comment 2

[James Wheare]

I'd argue there is a reason. If someone consistently saves bookmarks to the same destination, it'd be nice if Camino allowed for this rather than getting in the way of repeat actions.

Browsers should just work, and every time you have to take one more step than is necessary to achieve a goal, the experience is slightly tarnished. Another example of this is downloading items. I don't want to be asked where I want to save something every time I option-click a file, the download folder should just be used by default (separate issue I know but it illustrates my point).

Enable sensible defaults rather than forcing configuration. That's very much in line with the Mac ethos.

Comment 3

[Samuel Sidler (old account; do not CC)]

*** Bug 354685 has been marked as a duplicate of this bug. ***

Comment 4

[Samuel Sidler (old account; do not CC)]

Reopening for discussion. Carrying over flag request.

Comment 5

[Håkan Waara]

Like I said in the duped bug, now that this behavior has changed (to always default to the toolbar) I think the last chosen location should persist throughout sessions. This should be an easy fix.

We already do the same with the last open history/bookmark folder, in those views, iirc.

Comment 6

[Chris Lawson (gone)]

This bug is basically requesting what Mac OS apps have always done with Save, and what Camino still does when saving files (rather than bookmarks).

For the sake of consistency, I think we should probably do this.

And for what it's worth, this would bring us into parity with Safari, too.

Comment 7

[Warren TenBrook]

I've voted for this bug.  I never use the bookmark bar so, in my case, it is not desired behavior to reset the bookmark bar as default between sessions (and I would have preferred leaving the bookmark menu as the default, but that's a different matter).

Comment 8

[Håkan Waara]

Attachment: Proposed fix

Here's a fix.

* Persist the UUID of the last used/selected bookmark folder in the prefs, otherwise, use the current behavior
* Removes the unnecessary caching of mParentFolderIndex

If a folder is removed, or we for some reason can't find the UUID, things will automatically fallback on the current behavior.

Comment 9

[Stuart Morgan]

Comment on attachment 242691 [details] [diff] [review]
Proposed fix

>+NSString* const kLastSelectedBookmarkFolder   = @"last-selected-bookmark-folder";

This should be in UserDefaults.h

>+    // if none is saved from before, we'll use the toolbar folder as the default (see buildBookmarkFoldersPopup).

This code doesn't make that decisions, so don't document it here.

> - (IBAction)parentFolderChanged:(id)sender
> {
>-  mInitialParentFolderIndex = -1;
>+  BookmarkItem *selectedItem = [[mParentFolderPopup selectedItem] representedObject];
>+  [[NSUserDefaults standardUserDefaults] setObject:[selectedItem UUID] forKey:kLastSelectedBookmarkFolder];
>+  [self setDefaultParentFolder:(BookmarkFolder*)selectedItem];
> }

Should this be cached here, or when the bookmark is added?  If I bring up the window, select a new folder, then cancel, do we really want the default to change?

>   BookmarkFolder* parentFolder = [[mParentFolderPopup selectedItem] representedObject];
>   NSString*       titleString  = [mTitleField stringValue];
> 
>   BookmarkItem* newItem = nil;
>-  unsigned int  folderPosition = (mInitialParentFolderIndex != -1) ? mInitialParentFolderIndex : [parentFolder count];
>+  unsigned int  folderPosition = [parentFolder count];
>+  if (mInitialParentFolder) {
>+    // find out the position of this folder
>+    folderPosition = [[mInitialParentFolder parent] indexOfObject:mInitialParentFolder];
>+  }
>   
>   if (mCreatingFolder)
>   {
>     newItem = [parentFolder addBookmarkFolder:titleString inPosition:folderPosition isGroup:NO];
>   }

inPosition: expects an index under the target folder telling where to inset the item, and this feeds it the index of the item in its own parent, which isn't going to work.

Given that, I'm wondering if you actually want to remove mInitialParentFolderIndex. It looks like you thought it was the index *of* mInitialParentFolder, not an index *in* mInitialParentFolder. Removing it will change behavior.  I'm still not entirely clear on what the end-user benefit of that index was, so maybe that's fine; I just want to be sure it's intended.

>--(void)drawRect:(NSRect)aRect
>-{
>-  [super drawRect:aRect];
>-}
>-

No code changes in completely unrelated files please.

Comment 10

[Stuart Morgan]

Are you still working on this?  It's pretty close, and we'd like it for 1.1, so if not it would be good if someone else could pick it up.

Comment 11

[Chris Lawson (gone)]

Attachment: this is the patch-fixing police

This is the patch-fixing police. Let's move.

Comment 12

[Stuart Morgan]

Comment on attachment 246593 [details] [diff] [review]
this is the patch-fixing police

>+NSString* const kLastSelectedBookmarkFolder   = @"last-selected-bookmark-folder";

Go ahead and give the value type here like the others have

> - (void)dealloc
> {
>   [mTabGroupCheckbox release];
>-
>+  [mInitialParentFolder release];
>   [super dealloc];
> }

I'd prefer you leave the blank line before the |[super dealloc]|

>   unsigned int  folderPosition = (mInitialParentFolderIndex != -1) ? mInitialParentFolderIndex : [parentFolder count];
>+  if (mInitialParentFolder) {
>+    // find out the position of this folder
>+    folderPosition = [[mInitialParentFolder parent] indexOfObject:mInitialParentFolder];
>+  }

This new code shouldn't be here it all; it replaces the correct position (the first line above) with the wrong value. |folderPosition| should probably be renamed |insertPosition| so it's clearer.

Comment 13

[Chris Lawson (gone)]

Attachment: diff -w patch for review (not checkin)

For review/superreview. I'll post a whitespace-changes patch once this one has r/sr+.

cl

Comment 14

[Chris Lawson (gone)]

Attachment: by which I really mean this one

Forgot to change the constant back, so this wouldn't have compiled.

Comment 15

[Stuart Morgan]

Comment on attachment 247837 [details] [diff] [review]
by which I really mean this one

r=me

Comment 16

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 247837 [details] [diff] [review]
by which I really mean this one

sr=pink

Comment 17

[Chris Lawson (gone)]

Attachment: patch for checkin

Here ya go.

Comment 18

[froodian (Ian Leue)]

Checked in on trunk and MOZILLA_1_8_BRANCH.