Closed Bug 569258 Opened 11 years ago Closed 11 years ago

Deleting bookmarks from the Manager doesn't delete the metadata file (and other fun metadata sync issues)

Categories

(Camino Graveyard :: OS Integration, defect)

All
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: alqahira, Assigned: stuart.morgan+bugzilla)

References

Details

(Whiteboard: [camino-2.0.6])

Attachments

(1 file)

As originally reported in the forum, if you delete a bookmark via the BM Bar, the metadata file gets deleted.  However, if you delete a bookmark via the Manager, the .caminobookmark is still there.

Other sync issues:

2ab) Renaming a bookmark (in the Manager or Info Panel) doesn't delete the current metadata file; it just adds a new file with the new name inside the UUID folder.

3) Updating the Description field in the Panel doesn't update the metadata (updating the field via the Manager itself does)

I've checked adding/changing/deleting shortcuts in the Manager and the Panel, and changing the URL via the Manager and the Panel, and all of those actions do appear to trigger an update to the metadata file.  It appears only these three cases are broken.
Flags: camino2.0.4+
@Smokey: The "import" of bookmarks (from Safari, for example) do not create the metadata-files, too! (...but this is certainly the same like "adding" :-) )

Thanks and regards
Mehmet
Hmmmm… there is a very interesting behavior with the "import" of bookmarks:



Step 1: Import the Bookmarks from Safari and move them to your Bookmark-Bar within the Bookmark-Manager

Result --> No Meta-files are created



Step 2: Now click on some of the "new" Bookmarks in your Bookmark-Bar

Result --> No Meta-files are created



Step 3 (the interesting part): Quit (CMD+Q) and restart Camino. Now click again on some Bookmarks in your Bookmark-Bar

Result --> The meta-files are created, now !!!!!!



Thanks and Regards

Mehmet
This isn't going to block 2.0.4 now, but if Stuart or someone else happens to have a patch before the other blockers are done, we'll of course take it.
Flags: camino2.0.4+ → camino2.0.4-
The brokenness here is blocking my next step with the location bar autocomplete patch, so I'm starting on this now. I see the problems, but I'm also going to be doing a bunch of cleanup/improvement in the bookmark notification system in general as part of fixing this. I'll try to do it in a few parts.
Assignee: nobody → stuart.morgan+bugzilla
Status: NEW → ASSIGNED
That doesn't sound particularly branch-friendly (wide-ranging and possibly invasive) to me, or am I interpreting that incorrectly?  Should we just drop this for the branch, or will there be branch-able pieces?  (Not going to be in 2.0.5, regardless.)
Flags: camino2.0.5? → camino2.0.5-
Good point; I guess I should start with targeted fixes and then move on to general improvement after, so we can branch the most important stuff.
This fixes most of the problems. The fundamental issue is that all the notifications are designed around the tree structure changing, but what the bookmark manager really cares about is bookmarks being created and deleted, and there are mismatches. For example we suppress notifications when deleting in the bookmark manager, then send updates at the end about parents of deleted items having changed. That's fine for things that care about the tree (bookmark manager, bookmark menu, etc.) but is why deleting in the manager didn't delete the metadata. I think from code inspection that there were bugs nobody had noticed around what the manager though happened when a bookmark was moved from one folder to another, too.

Rather than try to duct-tape it all together, I added explicit calls to the manager when bookmarks are created or deleted, and moved metadata management and similar tasks there. This is a bit more work for the client code, but is much more understandable and as a result less fragile.

(Eventually I'm going to have those methods send notifications, which will help the new location bar, and will simplify some other observers, but that will be a follow-up bug.)

This should also fix the bug with duplicate files when renaming. I haven't looked at the panel description issue yet though. That doesn't sound related to this, so I can do another patch.
Attachment #483906 - Flags: superreview?(mikepinkerton)
One other thing I just thought of :( Does our AppleScript bookmark manipulation go through this code, or does it bypass too much and cause bookmark changes that never get noticed?

--

It looks like (with your patch applied) adding and deleting bookmarks via AppleScript never get noticed. :(

However, renaming works OK (deletes the UUID folder and recreates it with same UUID and file with the new name, just as renaming in the Manager does), as do changing the URL, description, and shortcut (these just update the file and modification time on the folder).

Moving makes no change to the metadata, which is what I'd expect.

tell application "Camino"
	make new bookmark with properties ¬
		{name:"Example", URL:"http://www.example.com/"} at end of ¬
		bookmarks of bookmark bar collection
	--comment out line above uncomment line below to test deletion
	--delete bookmark "Example" of bookmark bar collection
end tell
Oh, right. Maybe a follow-up bug for that? I'm willing to regress AS to fix the common cases (I'll fix at least removal quickly though; adding I'll need to play with to see if I can tell the difference between moving and creating).
(In reply to comment #9)
> Oh, right. Maybe a follow-up bug for that? I'm willing to regress AS to fix the
> common cases (I'll fix at least removal quickly though; adding I'll need to
> play with to see if I can tell the difference between moving and creating).

Sure: bug 605068.

(I wasn't even sure that either worked now and only cared about seeing what the results were with your patch since things were so broken pre-patch, but I checked now, and, amazingly, adding and deleting do work currently!)
Comment on attachment 483906 [details] [diff] [review]
fix add/remove and rename

sr=pink
Attachment #483906 - Flags: superreview?(mikepinkerton) → superreview+
Landed http://hg.mozilla.org/camino/rev/1b18cb5ce4f7

Smokey, can you do the honors for 2.0.6?
Flags: camino2.0.6? → camino2.0.6+
OS: Mac OS X → Windows XP
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
OS: Windows XP → Mac OS X
(In reply to comment #12)
> Smokey, can you do the honors for 2.0.6?

Done.  CAMINO_2_0_BRANCH in advance of 2.0.6.
Whiteboard: [camino-2.0.6]
Thanks for fixing it !!!

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