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

Status: VERIFIED FIXED

(Camino Graveyard :: Bookmarks, defect)

Description

[derek fong]

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

[Stuart Morgan]

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?).

Comment 2

[Ulrik Sverdrup]

I am trying to fix this one. I'll let BookmarkManager have a method
+defaultDockMenuFolder, and always fall back on that one. Sounds reasonable?

Comment 3

[Mike Pinkerton (not reading bugmail)]

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

[Jasper]

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

[Ulrik Sverdrup]

Attachment: 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

[Ulrik Sverdrup]

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

Comment 7

[Ulrik Sverdrup]

Attachment: Same patch updated to don't conflict with current source

Comment 8

[Ulrik Sverdrup]

Attachment: 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.

Comment 9

[Josh Aas]

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.

Comment 10

[Simon Fraser [no longer active]]

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.

Comment 11

[Simon Fraser [no longer active]]

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.

Comment 12

[Ulrik Sverdrup]

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

[Ulrik Sverdrup]

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.

Comment 14

[Ulrik Sverdrup]

Attachment: 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

Comment 15

[Ulrik Sverdrup]

Attachment: 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.

Comment 16

[Ulrik Sverdrup]

Attachment: 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.

Comment 17

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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

Comment 18

[Stuart Morgan]

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

Comment 19

[Stuart Morgan]

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

Comment 20

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

See also bug 314628 for improving the "Dock menu" experience once this patch gets in....

Comment 21

[Stuart Morgan]

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.

Comment 22

[Mike Pinkerton (not reading bugmail)]

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 23

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

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.

Comment 24

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

Ulrik, can you create a new patch based on the current tree? We'd love to get this in for 1.1. :)

Comment 25

[erkan]

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

Comment 26

[froodian (Ian Leue)]

Attachment: 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.

Comment 27

[Stuart Morgan]

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.

Comment 28

[Stuart Morgan]

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.

Comment 29

[froodian (Ian Leue)]

Attachment: Gets rid of defaultDockMenuFolder

Comment 30

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 230121 [details] [diff] [review]
Gets rid of defaultDockMenuFolder

sr=pink

Comment 31

[Nick Kreeger]

Fixed trunk and branch.

Comment 32

[HG]

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