Destination should be consistently remembered when saving bookmarks

RESOLVED FIXED in Camino1.5

Status

Camino Graveyard
Bookmarks
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: James Wheare, Assigned: Chris Lawson (gone))

Tracking

({fixed1.8.1.1})

unspecified
Camino1.5
PowerPC
Mac OS X
fixed1.8.1.1
Bug Flags:
camino1.1a2 +

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

12 years ago
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.
We remember during the same session. No reason to remember from previous sessions.
Status: UNCONFIRMED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → WONTFIX
(Reporter)

Comment 2

11 years ago
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.
(Assignee)

Updated

11 years ago
Status: RESOLVED → VERIFIED
*** Bug 354685 has been marked as a duplicate of this bug. ***
Reopening for discussion. Carrying over flag request.
Status: VERIFIED → UNCONFIRMED
Flags: camino1.1?
Resolution: WONTFIX → ---

Comment 5

11 years ago
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.
Assignee: mikepinkerton → nobody
QA Contact: bookmarks
(Assignee)

Comment 6

11 years ago
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.
(Assignee)

Updated

11 years ago
Assignee: nobody → hwaara
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 7

11 years ago
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

11 years ago
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.
Attachment #242691 - Flags: review?(stuart.morgan)

Comment 9

11 years ago
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.
Attachment #242691 - Flags: review?(stuart.morgan) → review-

Updated

11 years ago
Target Milestone: --- → Camino1.1

Comment 10

11 years ago
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.
(Assignee)

Comment 11

11 years ago
Created attachment 246593 [details] [diff] [review]
this is the patch-fixing police

This is the patch-fixing police. Let's move.
Assignee: hwaara → bugzilla
Attachment #242691 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #246593 - Flags: review?(stuart.morgan)

Comment 12

11 years ago
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.
Attachment #246593 - Flags: review?(stuart.morgan) → review-
(Assignee)

Comment 13

11 years ago
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
Attachment #246593 - Attachment is obsolete: true
Attachment #247762 - Flags: review?(stuart.morgan)
(Assignee)

Comment 14

11 years ago
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.
Attachment #247762 - Attachment is obsolete: true
Attachment #247837 - Flags: review?(stuart.morgan)
Attachment #247762 - Flags: review?(stuart.morgan)

Comment 15

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

r=me
Attachment #247837 - Flags: superreview?(mikepinkerton)
Attachment #247837 - Flags: review?(stuart.morgan)
Attachment #247837 - Flags: review+
Comment on attachment 247837 [details] [diff] [review]
by which I really mean this one

sr=pink
Attachment #247837 - Flags: superreview?(mikepinkerton) → superreview+
(Assignee)

Comment 17

11 years ago
Created attachment 249026 [details] [diff] [review]
patch for checkin

Here ya go.

Comment 18

11 years ago
Checked in on trunk and MOZILLA_1_8_BRANCH.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago11 years ago
Flags: camino1.1? → camino1.1a2+
Keywords: fixed1.8.1.1
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.