Closed Bug 549179 Opened 14 years ago Closed 14 years ago

Spotlight bookmark search result doesn't open Camino bookmarks in Camino

Categories

(Camino Graveyard :: OS Integration, defect)

Product:
Camino Graveyard
Component:
OS Integration
Platform:
All
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

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

References

(
URL
)

Details

(Whiteboard: [camino-2.0.3] rdar://6270593 rdar://7751157)

Attachments

(1 file, 3 obsolete files)

Custom importer, v2
69.25 KB, patch
mikepinkerton
: superreview+
Details | Diff | Splinter Review 
To be honest, I'm not sure this ever worked (nothing in bug 292550 indicates it did), but I have a vague recollection of it working either on 10.4 or early in 10.5.  However…

When you run a Spotlight search and match a bookmark item, the OS tries to open the bookmark in Safari, since Safari declares the .webbookmark extension.

Worse, if you try to "Open With", we parse the .webbookmark plist as XML and display it.

The fix for the second issue is easy; just add .webbookmark to the list of extensions to parse in NSURL+Utils (note this won't fix Gecko, but who's going to drag a .webbookmark onto a Gecko view?); see attached patch.

The fix for the first issue should be "easy", too, in that if we declare .webbookmark in our .plist, the OS should know we can open them, and users can manually set the .webbookmark extension to open in Camino by default (I suppose we could write a creator code to our bookmarks metadata and eliminate the need to manually set, or to rely on LaunchServices black magic, the default on 10.4 and 10.5).

However, we'd have to proceed carefully to make sure we don't change anything about the .webbookmark registration that makes Bookmarks.mdimporter stop indexing our (or all) bookmarks because it doesn't think the type matches the type it's supposed to index any more.  Maybe as long as we let Safari always export the UTIs and we just declare we can open the extension?

An alternate fix for the first issue would be to write and ship our own importer and develop new extensions and whatnot, but that sounds less fun.

The patch attached here makes the NSURL+Utils change, which will allow anyone to manually open bookmarks-metadata files in Camino.  I think it's probably worth taking regardless of whether we can safely declare .webbookmark in our plist, but that's certainly debatable.
pink said he was pretty sure it worked when it landed and we should add .webbookmark to our .plist if we can open it.

Stuart said we may as well write our own importer, and maybe he'd do it.
URL: http://forums.mozillazine.org/viewtop...
(In reply to comment #1)
> pink said he was pretty sure it worked when it landed

http://mxr.mozilla.org/seamonkey/source/camino/src/bookmarks/BookmarkManager.mm#1236

"we piggyback the one that Safari uses which launches the default browser when selected."

So at some point that changed.
This patch is just the previous one with the addition of the Info.plist changes to let people manually Get Info and Change All to set us as the default.

As I mentioned in an earlier comment, that's kind-of sucky; you have to do a "Change All" for the association to stick meaningfully (since we delete the files), so on 10.5 and below writing a creator would help, but that's beyond me.

----

I also experimented with another approach, which was to (also) export the com.apple.safari.bookmark UTI but for a .cmbookmark filename extension (the OS importer only matches by UTI, not by filename extension, so that it could import the metadata while only we would open the files), but that didn't seem to work here; the files were given a dynamic UTI instead of com.apple.safari.bookmark.

I'm not sure if only one app can export a given UTI or I just needed to completely reset my Spotlight db and kick LaunchServices, or logout, or…, but I didn't feel like doing any of those bits of gymnastics tonight.
Attachment #429382 - Attachment is obsolete: true
These files are also Quick Look-able; they show the Title, URL, (size, last modified), and a pretty icon.
Attached patch Custom importer, v1 (obsolete) — — Splinter Review 
This is the more heavy-weight solution: using a custom importer.

A few notes:
1) In theory this should give us description and shortcut searchability. In practice that only worked once I manually ran mdimport, but my setup is probably thoroughly confused by all the changes I was making as I went.
2) This declares our bookmarks as a subtype of Safari's bookmarks. This is the only way I know of to actually show up in the Bookmarks/Webpages category in Spotlight results, since there's no generic UTI for that (I filed a Radar in 2008; no update yet). We need to test that this works on 10.4 and 10.5, because I think I had problems doing that with Precipitate.
3) Bookmarks with :'s will show -'s instead, and our metadata files become skankier (a file+directory for each bookmark). This is because I can't find any way to get Spotlight to use our kMDItemDisplayName they way they do for Safari bookmark types. I'm thinking there may be some hard-coded magic there. I filed a Radar, but for now I'm doing what Precipitate does: using the UUID for the folder, so we can give the file itself a non-crappy filename, since that's what Spotlight will show.

(Sorry for all the Xcode churn here; I had to add a framework, so I finally got rid of the pointless subdivision of the Frameworks directory that Xcode used to default to, re-ordered the frameworks, and moved the external project files into a new group.)

Holding off on review until someone can test on 10.4 and 10.5
Assignee: nobody → stuart.morgan+bugzilla
Status: NEW → ASSIGNED
This works well for me on 10.5, including description and shortcuts, and showing up in the Webpages category in the Spotlight menu search.  

Eiichi, can you test this patch on 10.4?

I mentioned what turned out to be a stray plist in the wrong place in the project; Stuart fixed that stuff locally.
For reference, the Radar bugs I referred to are now openradar'd:
http://openradar.appspot.com/6270593
http://openradar.appspot.com/7751157
Whiteboard: rdar://6270593 rdar://7751157
(In reply to comment #6)
> Eiichi, can you test this patch on 10.4?

I applied the patch:attachment (id=432340) and tested on 10.4.
The patch works fine as expected on 10.4.
Spotlight bookmark search result opens Camino bookmarks in Camino.
Moves scheme.xml to resources/, puts the Info.plist with the others at the top level, and removes the dead reference to the Xcode-created Info.plist.
Attachment #432340 - Attachment is obsolete: true
Attachment #432437 - Flags: superreview?(mikepinkerton)
Per the meeting, this is better than the current state of affairs, so even if there are issues, we'd like to get this in 2.0.x.
Blocks: 306052
Flags: camino2.0.3?
+  // This should not be possible, but since we are about to do a recursive
+  // delete we want to be very, very careful.
+  if ([uuid length] == 0)
+    return;

do you also want to test that we're not deleting "/" or "~" or something obvious like that?

+  NSDictionary *plist =
+      [[[NSDictionary alloc] initWithContentsOfFile:inFile] autorelease];

NSDictionary* plist =

+Boolean GetMetadataForFile(void* thisInterface,
+			   CFMutableDictionaryRef attributes,
+			   CFStringRef contentTypeUTI,
+			   CFStringRef pathToFile)

indent 1 more space

sr=pink
Attachment #432437 - Flags: superreview?(mikepinkerton) → superreview+
We don't need to get symbols on this, right?  If for whatever reason the importer starts crashing, it'll be crashing md*, never us?
(In reply to comment #11)
> do you also want to test that we're not deleting "/" or "~" or something
> obvious like that?

We append the UUID, and since I've checked that it's not empty, the worst-case scenario is /<uuid> or ~/<uuid>, both of which are highly unlikely to end up actually being real files/directories.

(In reply to comment #12)
> We don't need to get symbols on this, right?  If for whatever reason the
> importer starts crashing, it'll be crashing md*, never us?

Right.
Landed on CVS trunk with whitespace fixes. Smokey, can you land this on the branch? I don't have a branch tree, and didn't trust cross-landing the large project patch.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
And landed on the CAMINO_2_0_BRANCH (with a little merge fix-up) in advance of Camino 2.0.3.
Flags: camino2.0.3? → camino2.0.3+
Whiteboard: rdar://6270593 rdar://7751157 → [camino-2.0.3] rdar://6270593 rdar://7751157
Stuart, any idea why this caused a 3.8% Ts regression on minus, ~1.5% on cb-x4, 2.2% on cb-x4-branch, ~0.75% on cb-x1, and ~0.5% on cb-x1 branch?  

What new code are we running at startup? Or is it just that we now have to make UUID-folders and metadata files, whereas before we only made files?
Even worse, we didn't do a clobber build when testing :(  This started failing with nightly-generation: http://tinderbox.mozilla.org/showlog.cgi?log=Camino/1269759840.1269761137.18209.gz&fulltext=1

CompileC build/Camino.build/Deployment/CaminoStaticApp.build/Objects-normal/ppc/MainController.o /builds/tinderbox/Cm2.1-M1.9/Darwin_8.11.1_Depend/build/ppc/camino/src/application/MainController.mm normal ppc objective-c++ com.apple.compilers.gcc.4_0
[...]
-c /builds/tinderbox/Cm2.1-M1.9/Darwin_8.11.1_Depend/build/ppc/camino/src/application/MainController.mm -o /builds/tinderbox/Cm2.1-M1.9/Darwin_8.11.1_Depend/build/ppc/camino/build/Camino.build/Deployment/CaminoStaticApp.build/Objects-normal/ppc/MainController.o
In file included from /builds/tinderbox/Cm2.1-M1.9/Darwin_8.11.1_Depend/build/ppc/camino/src/application/MainController.mm:65:
/builds/tinderbox/Cm2.1-M1.9/Darwin_8.11.1_Depend/build/ppc/camino/src/application/GrowlController.h:41:24: error: Growl/Growl.h: No such file or directory
/builds/tinderbox/Cm2.1-M1.9/Darwin_8.11.1_Depend/build/ppc/camino/src/application/GrowlController.h:43: error: cannot find protocol declaration for 'GrowlApplicationBridgeDelegate'

Looking back through the log, Growl's not being built at all after the framework re-org. :(  We build the new importer, then relaunch tool, Sparkle,  breakpadUtilities, Inspector, crash_report_sender, Breakpad, and CaminoStaticApp.

I'll be able to look in a bit and see if I can fix this; otherwise and if Stuart hasn't come up with a fix, I'll back this out :-(
I checked in a bustage fix for this; Growl.framework had gotten lost from the dependencies of both app targets (I also moved the importer to be built after the external frameworks, since it seemed odd to build "Camino code" then a bunch of external code, and then Camino again...).
(In reply to comment #16)
> Stuart, any idea why this caused a 3.8% Ts regression on minus, ~1.5% on cb-x4,
> 2.2% on cb-x4-branch, ~0.75% on cb-x1, and ~0.5% on cb-x1 branch?  

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

Attachment

General

Creator:
Created:
Updated:
Size: