Unable to delete bookmark folder that is flagged as Dock Menu + cannot remove from Dock

VERIFIED FIXED in Camino1.5

Status

Camino Graveyard
Bookmarks
VERIFIED FIXED
13 years ago
11 years ago

People

(Reporter: derek fong, Assigned: froodian (Ian Leue))

Tracking

({verified1.8.1})

unspecified
Camino1.5
PowerPC
Mac OS X
verified1.8.1

Details

Attachments

(1 attachment, 7 obsolete attachments)

(Reporter)

Description

13 years ago
User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7) Gecko/20040517 Camino/0.8b
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7) Gecko/20040517 Camino/0.8b

There appears to be a problem manipulating bookmark folders that are used in the
Dock.  Specifically, you are unable to "unmark" a bookmark folder as being in
the Dock and you are unable to actually delete the bookmark folder.

Reproducible: Always
Steps to Reproduce:
1. Open the Bookmarks pane.
2. Create a new bookmark folder.
3. Get Info on the new bookmark folder you just created.
4. Check the Dock Menu checkbox.

Immediately, the "Dock Menu" item is disabled.  There is no way to remove this
folder from the Dock.

Additionally, to reproduce the second part of this bug:

5. Back in the Bookmarks pane, right-click the bookmark folder you created, and
select Delete.

Actual Results:  
The bookmark folder remains.  Sometimes the bookmark folder disappears (i.e., if
you created a bookmark folder for use in the Dock in the Toolbar), but upon
opening a new window, the bookmark folder reappears and there is no way to
remove it.

Expected Results:  
The bookmark folder should be deleted when you choose to delete it, and you
should be able to toggle the Dock status on and off at will.

Comment 1

13 years ago
Yes, the current behavior is definitely bad.

There's a work-around (select a different folder as the dock folder, which will
unmark the old dock folder), but it's not at all intuitive. We should allow no
folder to be selected, or default to something like the Toolbar that should
always be there when a folder is un-checked (which do we do for a new profile?).
Status: UNCONFIRMED → NEW
Ever confirmed: true
Target Milestone: --- → Camino1.1

Comment 2

13 years ago
I am trying to fix this one. I'll let BookmarkManager have a method
+defaultDockMenuFolder, and always fall back on that one. Sounds reasonable?
should we prompt the user first and let them know what's going on and what we'll
be setting the dock menu to?

Comment 4

13 years ago
Sounds like a good plan to me, default dock menu on a fresh install is the
topten bm folder so you could use that I guess.

Comment 5

13 years ago
Created attachment 182783 [details] [diff] [review]
Patch to allow to unceck "Dock menu" as well as deleting the dock menu folder

Modifies behavior to not guard the dock menu folder and lets it be deleted and
unset. Reverts to default dock menu folder (Given by a BookmarksManager method)
when it's needed.

Also fixes a bug in connection to this: When copying the dock menu bookmark
folder, the dock menu folder flag was transferred.

Does not give a warning upon deleting the dock menu folder

Comment 6

13 years ago
(In reply to comment #5)
The small bug I mentioned is Bug 293147

Updated

12 years ago
Attachment #182783 - Flags: review+

Comment 7

12 years ago
Created attachment 184962 [details] [diff] [review]
Same patch updated to don't conflict with current source

Updated

12 years ago
Attachment #182783 - Attachment is obsolete: true
Attachment #184962 - Flags: review+

Comment 8

12 years ago
Created attachment 184963 [details] [diff] [review]
Patch to allow unchecking and transferring the dock flag, without the allow-delete changes

This patch lets the "dock menu" checkbox always be enabled, and lets the user
uncheck the current dock menu (camino falls back on a default dock menu,
currently top ten, which can't be unchecked (no info box on that one))

Also, you can check "dock menu" for any folder, and it will be set and the
previous one unset.
Attachment #184963 - Flags: review+

Updated

12 years ago
Attachment #184963 - Flags: review+ → review?(joshmoz)

Updated

12 years ago
Attachment #184962 - Flags: review+ → review?(joshmoz)

Comment 9

12 years ago
Comment on attachment 184962 [details] [diff] [review]
Same patch updated to don't conflict with current source

These are cool patches, but I'm not going to get to reviewing them soon so
asking smfr for review.
Attachment #184962 - Flags: review?(joshmoz) → review?(sfraser_bugs)

Updated

12 years ago
Attachment #184963 - Flags: review?(joshmoz) → review?(sfraser_bugs)

Comment 10

12 years ago
Comment on attachment 184962 [details] [diff] [review]
Same patch updated to don't conflict with current source

Patch is stale copyWithZone: changed a while back.
Attachment #184962 - Flags: review?(sfraser_bugs) → review-

Comment 11

12 years ago
Comment on attachment 184963 [details] [diff] [review]
Patch to allow unchecking and transferring the dock flag, without the allow-delete changes

Stale patch. And do you wan this one or the last one or both? Don't make
overlapping patches.
Attachment #184963 - Flags: review?(sfraser_bugs) → review-

Comment 12

12 years ago
Sorry, I didn't understand in which form the patch was wanted - with the dock
flag fix or not. Now that the dock flag is checked in in another bug, that's
solved. 

I'm sorry but I can't build camino right now, and can't work at it for a short
while more. I'll come back to this bug in two-three weeks I hope.

Comment 13

12 years ago
Comment on attachment 184962 [details] [diff] [review]
Same patch updated to don't conflict with current source

Obsoleting this patch. Includes dock flag changes already checked in
anotherplace.
Attachment #184962 - Attachment is obsolete: true

Comment 14

12 years ago
Created attachment 194267 [details] [diff] [review]
Patch to allow checking/unchecking of dock menu checkbox for bookmark folders

A patch from current source

Addresses: Allows the user to delselect the enabled "dock menu" checkbox for
bookmark folders. Handles fallback to default folder (top10). Handles updating
of checkbox if the dock menu changes while it is displayed.

Handles a minor thing outside it's scope: sets mDirty for the dock menu after
setBookmarksFolder: is called on it.

The patch includes no changes to allow deletion of the dock menu folder
Attachment #184963 - Attachment is obsolete: true
Attachment #194267 - Flags: review?(sfraser_bugs)

Comment 15

12 years ago
Created attachment 194269 [details] [diff] [review]
Increment to do the full fix; allow deletion of dock menu folders

With the above patch in place, the step to allow and handle deletion of the
dock menu folder is trivial. This is an increment of the previous patch.

The following patch changes one line and adds two. isDockMenu does not make a
bookmarks folder special anymore.
Attachment #194269 - Flags: review?(sfraser_bugs)

Comment 16

12 years ago
Created attachment 194272 [details] [diff] [review]
Increment to do the full fix; allow deletion of dock menu folders (fixed edition)

The previous patch was reversed by mistake. This one is diffed in the right
direction.

 I'm sorry, and yes, I'm going to sleep now to not make more silly mistakes.
Attachment #194269 - Attachment is obsolete: true
Attachment #194272 - Flags: review?(sfraser_bugs)

Updated

12 years ago
Attachment #194269 - Flags: review?(sfraser_bugs)
*** Bug 314629 has been marked as a duplicate of this bug. ***

Comment 18

12 years ago
*** Bug 319297 has been marked as a duplicate of this bug. ***

Comment 19

12 years ago
*** Bug 327995 has been marked as a duplicate of this bug. ***
See also bug 314628 for improving the "Dock menu" experience once this patch gets in....

Comment 21

12 years ago
Comment on attachment 194267 [details] [diff] [review]
Patch to allow checking/unchecking of dock menu checkbox for bookmark folders

Unfortunately, this patch has suffered some bitrot.  If someone has time to clean it up to apply to the current tree, I'll review it soon.
Attachment #194267 - Flags: review?(sfraser_bugs) → review-
so what does the dock menu get reset to after you delete it? don't you have to manually set it back to something else?
Comment on attachment 194272 [details] [diff] [review]
Increment to do the full fix; allow deletion of dock menu folders (fixed edition)

Clearing review request for bitrotten patch.
Attachment #194272 - Flags: review?(sfraser_bugs)
Ulrik, can you create a new patch based on the current tree? We'd love to get this in for 1.1. :)
QA Contact: bookmarks
Assignee: mikepinkerton → englabenny
OS: Mac OS X 10.2 → Mac OS X 10.3

Comment 25

11 years ago
This was a problem 2 years ago and  still a problem in the new release? That sucks..
(Assignee)

Comment 26

11 years ago
Created attachment 228030 [details] [diff] [review]
Brings Ulrik's patches up to date

This applies cleanly and works as expected.  I'm not intimately acquainted with the code, but I did apply it all by hand and read it as I did, and I think I have a pretty decent understanding of what's going on, so I'm gonna take responsibility for it from now on.  Ulrik made this as two patches, to land incrementally.  Since this isn't really that big a landing and we don't do that very much, I rolled it up into one patch.
Assignee: englabenny → stridey
Attachment #194267 - Attachment is obsolete: true
Attachment #194272 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #228030 - Flags: review?(stuart.morgan)

Comment 27

11 years ago
Comment on attachment 228030 [details] [diff] [review]
Brings Ulrik's patches up to date

r=me. Looks good, and provides a much better user experience than the current system.

pink, in answer to your question: there's no need to explicitly mark the fallback (top-10) as the dock menu because there's no way to get info on a collection, so from a user perspective there's no difference between having no explicit dock menu and having top-10 explicitly marked as the dock menu. Code-wise, all access to the dock menu is through a function that transparently substitutes in the fallback if there isn't an explicit one.
Attachment #228030 - Flags: superreview?(mikepinkerton)
Attachment #228030 - Flags: review?(stuart.morgan)
Attachment #228030 - Flags: review+

Comment 28

11 years ago
Comment on attachment 228030 [details] [diff] [review]
Brings Ulrik's patches up to date

>+-(BookmarkFolder *) defaultDockMenuFolder;
...
>-    return [self top10Folder];
>+    return [self defaultDockMenuFolder];
>+}
>+
>+-(BookmarkFolder *) defaultDockMenuFolder {
>+  return [self top10Folder];

Actually... I had decided against requesting this change, but thinking about it more I really can't come up with any reason someone would need to be able to find out what the default dock menu folder is; only dockMenuFolder should know or care about the fallback.

So r=me with the above section of changes removed.
(Assignee)

Comment 29

11 years ago
Created attachment 230121 [details] [diff] [review]
Gets rid of defaultDockMenuFolder
Attachment #228030 - Attachment is obsolete: true
Attachment #230121 - Flags: superreview?(mikepinkerton)
Attachment #230121 - Flags: review?(stuart.morgan)
Attachment #228030 - Flags: superreview?(mikepinkerton)

Updated

11 years ago
Attachment #230121 - Flags: review?(stuart.morgan) → review+
Comment on attachment 230121 [details] [diff] [review]
Gets rid of defaultDockMenuFolder

sr=pink
Attachment #230121 - Flags: superreview?(mikepinkerton) → superreview+
Whiteboard: [needs checkin]

Comment 31

11 years ago
Fixed trunk and branch.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
Whiteboard: [needs checkin]
(Assignee)

Updated

11 years ago
Status: RESOLVED → VERIFIED
Keywords: fixed1.8.1 → verified1.8.1

Comment 32

11 years ago
*** Bug 357353 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.