453237 - Support import of iCab 4 bookmarks

Status: RESOLVED FIXED

(Camino Graveyard :: Bookmarks, defect)

Description

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

We currently support the import of iCab 1-2 and iCab 3 bookmarks (same file format, different file names), but iCab 4 brings a new plist format and file location.

At some point, we should support the import of this new format (which lives in ~/Library/Preferences/iCab/iCab 4 Bookmarks).

Sample default iCab 4 bookmarks in the URL field.

Comment 2

[Chris Lawson (gone)]

I have the converter finished (Stuart wrote awesome Safari converter code which made this really easy), just need to plumb it into everything else.

Comment 3

[Chris Lawson (gone)]

Attachment: new AccessoryViews.nib

Turns out this requires a new AccessoryViews.nib file, so here it is.

Comment 4

[Chris Lawson (gone)]

Attachment: fix v1.0

Comment 5

[Chris Lawson (gone)]

For some odd reason, the Export dialog won't let you save the exported bookmarks with any extension other than "html". It writes them in proper plist format, and iCab won't deal with the file as a native bookmarks file unless all extensions are stripped anyway, so unless I missed something simple (entirely possible), it's probably not worth worrying about much.

I tested import of an iCab bookmarks file and exporting our default bookmarks to an iCab file, and both worked great.

Comment 6

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

(In reply to Chris Lawson from comment #5)
> For some odd reason, the Export dialog won't let you save the exported
> bookmarks with any extension other than "html". It writes them in proper
> plist format, and iCab won't deal with the file as a native bookmarks file
> unless all extensions are stripped anyway, so unless I missed something
> simple (entirely possible), it's probably not worth worrying about much.

The extension determination is handled in MainController's setFileExtension:

You'd have to patch that, too, to handle the iCab case.

However, as I noted on irc last night, I don't think we want to add support for *exporting* to some random file format just because we can (and iCab 4 support importing Camino bookmarks natively, as well as the standard Netscape HTML interchange format).  Even if we do, we don't want to support it in 2.1, because we're done juggling nib changes, unless we hit a super-critical bug.

Comment 7

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

Comment on attachment 597635 [details]
new AccessoryViews.nib

I can't review this as a nib, anyway, since you used IB 3.x on 10.6 to update it; IB 3.x on 10.6 writes an IB3-format nib that's incompatible with IB 3.x on 10.5 :(

Comment 8

[Chris Lawson (gone)]

Attachment: fix v1.1, no export

Per discussion above and on IRC, we don't care about supporting export right now. If someone decides we want it later, the obsolete patches on this bug have most of the necessary code.

This version of the patch is import-only (and as a bonus, doesn't require nib changes, so it's l10n-safe).

Comment 9

[philippe (part-time)]

Comment on attachment 598088 [details] [diff] [review]
fix v1.1, no export

I took this for a brief spin, and the import went well. As far as I can tell no bookmarks got lost or damaged (tested with bookmarklets mostly). Tabgroups came through intact.

One thing went missing though: iCab bookmarks have a 'description' field (labeled as 'description' when one does a Cmd-I on a BM in the manager, but strangely the column in the BM manager is labeled 'Remark'); the plist has a 'description' field. That field is equivalent to Camino's description field. Follow-up bug maybe ?

As a side note, iCabs 'Camino' bookmarks importer is rather week, at least on 10.7.3: it only imports bookmarks present on the bookmark toolbar, but not folder on the BM bar, and it misses everything under the Bookmarks menu and collections. That might add weight to add an exporter in the future. I have contacted Alexander Claus about this.

Comment 10

[Chris Lawson (gone)]

Attachment: fix v1.11, now imports descriptions

(In reply to philippe from comment #9)

> One thing went missing though: iCab bookmarks have a 'description' field
> (labeled as 'description' when one does a Cmd-I on a BM in the manager, but
> strangely the column in the BM manager is labeled 'Remark'); the plist has a
> 'description' field. That field is equivalent to Camino's description field.
> Follow-up bug maybe ?

Nah, that was an oversight on my part; I did indeed mean to import Description, too. This version fixes that.

Comment 11

[philippe (part-time)]

Comment on attachment 598132 [details] [diff] [review]
fix v1.11, now imports descriptions

(In reply to Chris Lawson from comment #10)
 
> Nah, that was an oversight on my part; I did indeed mean to import
> Description, too. This version fixes that.
Yes! The descriptions are imported correctly (and in case you're worried: French, English, Spanish and a test description with random unicode characters all imported nicely).

Comment 12

[philippe (part-time)]

(In reply to philippe from comment #9)
 
> As a side note, iCabs 'Camino' bookmarks importer is rather week, at least
> on 10.7.3: it only imports bookmarks present on the bookmark toolbar, but
> not folder on the BM bar, and it misses everything under the Bookmarks menu
> and collections. That might add weight to add an exporter in the future. I
> have contacted Alexander Claus about this.

Alexander Claus already fixed this; he send me a test build and it works correctly on my side. Thanks.

Comment 13

[philippe (part-time)]

I ran a test for the import of (old, extensionless) Opera bookmarks, using Opera 8.54 on 10.6:
1. having only old Opera + Camino installed --> OK
2. installing iCab, letting it create its folder --> Opera BMs/iCab BM's import OK
3. upgrade Opera to 11.61, bookmarks converted --> both BM sets import OK

While doing this, I noticed that the Opera importer doesn't work (it is not in the drop down) for *new* installs of Opera. I'll file a bug for that.

Comment 14

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

Comment on attachment 598132 [details] [diff] [review]
fix v1.11, now imports descriptions

Code looks good to me, aside from the typos.  I didn't test, but it looks like philippe has done so extensively. r=ardissone

Comment 15

[Chris Lawson (gone)]

Comment on attachment 598132 [details] [diff] [review]
fix v1.11, now imports descriptions

Over to smorgan for sr?. I can respin for the typo or Smokey can probably fix it on checkin, whatever you guys want.

Comment 16

[Stuart Morgan]

Comment on attachment 598132 [details] [diff] [review]
fix v1.11, now imports descriptions

>+    // The allergy to file extensions was transmitted from the Norwagians to ze
>+    // Germans, so try iCab first.

Nice.


>+  if ([[iCabDict objectForKey:kEntryTypeKey] isEqualToString:kBookmarkTypeFolder])
>+  {
>+    return [self bookmarkFolderForDictionary:iCabDict];
>+  }

No braces.

>+  BookmarkFolder* folder = [BookmarkFolder bookmarkFolderWithTitle:
>+                              [iCabDict objectForKey:kBookmarkFolderTitleKey]];

Just indent four from the start of the line.

Comment 17

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

Chicken-wrangling note: exorcise the "boomarks".

(In reply to comment #14)
> Code looks good to me, aside from the typos.

Comment 18

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

http://hg.mozilla.org/camino/rev/d6896241ab7b with the sr comments address and the boomarks exorcised.