Last Comment Bug 328737 - Destination should be consistently remembered when saving bookmarks
: Destination should be consistently remembered when saving bookmarks
Status: RESOLVED FIXED
: fixed1.8.1.1
Product: Camino Graveyard
Classification: Graveyard
Component: Bookmarks (show other bugs)
: unspecified
: PowerPC Mac OS X
-- normal with 1 vote (vote)
: Camino1.5
Assigned To: Chris Lawson (gone)
:
:
Mentors:
: 354685 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-02-27 08:34 PST by James Wheare
Modified: 2006-12-18 12:03 PST (History)
6 users (show)
froodian: camino1.1a2+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Proposed fix (8.72 KB, patch)
2006-10-18 13:45 PDT, Håkan Waara
stuart.morgan+bugzilla: review-
Details | Diff | Splinter Review
this is the patch-fixing police (4.74 KB, patch)
2006-11-25 22:27 PST, Chris Lawson (gone)
stuart.morgan+bugzilla: review-
Details | Diff | Splinter Review
diff -w patch for review (not checkin) (5.90 KB, patch)
2006-12-06 18:10 PST, Chris Lawson (gone)
no flags Details | Diff | Splinter Review
by which I really mean this one (5.92 KB, patch)
2006-12-07 08:07 PST, Chris Lawson (gone)
stuart.morgan+bugzilla: review+
mikepinkerton: superreview+
Details | Diff | Splinter Review
patch for checkin (6.56 KB, patch)
2006-12-18 11:50 PST, Chris Lawson (gone)
no flags Details | Diff | Splinter Review

Description User image James Wheare 2006-02-27 08:34:25 PST
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 User image Samuel Sidler (old account; do not CC) 2006-03-27 22:04:44 PST
We remember during the same session. No reason to remember from previous sessions.
Comment 2 User image James Wheare 2006-03-28 01:29:12 PST
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 User image Samuel Sidler (old account; do not CC) 2006-09-28 16:11:34 PDT
*** Bug 354685 has been marked as a duplicate of this bug. ***
Comment 4 User image Samuel Sidler (old account; do not CC) 2006-09-28 16:12:02 PDT
Reopening for discussion. Carrying over flag request.
Comment 5 User image Håkan Waara 2006-09-28 16:14:11 PDT
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 User image Chris Lawson (gone) 2006-10-17 19:59:48 PDT
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 User image Warren TenBrook 2006-10-18 07:50:08 PDT
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 User image Håkan Waara 2006-10-18 13:45:50 PDT
Created attachment 242691 [details] [diff] [review]
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 User image Stuart Morgan 2006-10-22 10:26:21 PDT
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 User image Stuart Morgan 2006-11-25 13:14:01 PST
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 User image Chris Lawson (gone) 2006-11-25 22:27:36 PST
Created attachment 246593 [details] [diff] [review]
this is the patch-fixing police

This is the patch-fixing police. Let's move.
Comment 12 User image Stuart Morgan 2006-11-30 21:42:13 PST
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 User image Chris Lawson (gone) 2006-12-06 18:10:16 PST
Created attachment 247762 [details] [diff] [review]
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 User image Chris Lawson (gone) 2006-12-07 08:07:08 PST
Created attachment 247837 [details] [diff] [review]
by which I really mean this one

Forgot to change the constant back, so this wouldn't have compiled.
Comment 15 User image Stuart Morgan 2006-12-10 09:47:46 PST
Comment on attachment 247837 [details] [diff] [review]
by which I really mean this one

r=me
Comment 16 User image Mike Pinkerton (not reading bugmail) 2006-12-18 09:26:35 PST
Comment on attachment 247837 [details] [diff] [review]
by which I really mean this one

sr=pink
Comment 17 User image Chris Lawson (gone) 2006-12-18 11:50:42 PST
Created attachment 249026 [details] [diff] [review]
patch for checkin

Here ya go.
Comment 18 User image froodian (Ian Leue) 2006-12-18 12:03:26 PST
Checked in on trunk and MOZILLA_1_8_BRANCH.

Note You need to log in before you can comment on or make changes to this bug.