242673 - Bookmark importer should recognize Opera

Status: RESOLVED FIXED

(Camino Graveyard :: Bookmarks, enhancement)

Description

[Stuart Morgan]

Opera is not currently supported by our bookmark import, while almost everything
else is.

Comment 1

[Mike Pinkerton (not reading bugmail)]

whatever.

Comment 2

[David Haas (gone, not reading mail)]

Attachment: Opera bookmark importer

Opera users need lovin' too.

The patch works, at least for the default Opera 7
bookmark set.  Beats me if something more complex
will make it choke.

Comment 3

[Josh Aas]

got it

Comment 4

[Josh Aas]

As long as we have a patch, marking 0.9.

Comment 5

[David Haas (gone, not reading mail)]

Attachment: fixed patch

missed a line.	sorry about that.

Comment 6

[David Haas (gone, not reading mail)]

ping

Comment 7

[Josh Aas]

+// Hijacked it for Opera file import purposes.  Probably should rename it to
+// "decodedFile"

This method really isn't specific to HTML. Instead of adding a comment, I would
take your own advice and rename the method (decodedTextFile?). Make sure to
update references to it of course, but also change the NSLogs inside it to
reflect the new method name. Maybe improve the method explanation to include
more helpful documentation.

Comment 8

[Josh Aas]

Comment on attachment 158026 [details] [diff] [review]
fixed patch

Additional comments...
-------------------------------------
While you are in there, please remove

  id parent = [item parent];

from - (NSMenu *)contextMenuForItem:(id)item fromView:(BookmarkOutlineView
*)outlineView target:(id)target in BookmarkManager.mm (near the bottom, above
the comment "if we're not in a smart collection (other than history)"

The variable is unused.
--------------------------------------
  while (aLine = [enumerator nextObject]) {

Shouldn't this be double parenthesized? Note:

/Users/josh/src/Camino_Project/camino_opt/mozilla/camino/src/bookmarks/Bookmark
Manager.mm: In function `BOOL -[BookmarkManager
readOperaFile:intoFolder:](BookmarkManager*, objc_selector*, NSString*,
objc_object*)':
/Users/josh/src/Camino_Project/camino_opt/mozilla/camino/src/bookmarks/Bookmark
Manager.mm:996: warning: suggest parentheses around assignment used as truth
value
---------------------------------------
cosmetic - add a newline between the last line in the patch that you changed
and the following comment
---------------------------------------

Thats all - if you could address these comments and the one above about the
method name, that would be great.

Comment 9

[Josh Aas]

Attachment: patch v3

Addressed my own comments in order to get this landed fast.

Comment 10

[Geoff "temporarily off bugmail" Beier]

This looks good. Two minor observations:

1. In the comment where we criticize their misspelling:
      // Perhaps a separator? This isn't how I'd spell it, but
      // then again, I'm not Norwagian, so what do I know.
      else if ([aLine hasPrefix:@"#SEPERATOR"]) {
we misspell Norwegian.

2. This patch uses a local variable named sRange. IIRC this is the same
convention we use in some other places to indicate a static variable. We may
want to choose another name.

Neither of these is probably worth re-spinning the patch, but if (1) is
unintentional or anyone else doesn't like (2) you may want to fix before committing.

r=[e-mail]

Comment 11

[David Haas (gone, not reading mail)]

(In reply to comment #10)
> 1. In the comment where we criticize their misspelling:
>       // Perhaps a separator? This isn't how I'd spell it, but
>       // then again, I'm not Norwagian, so what do I know.
>       else if ([aLine hasPrefix:@"#SEPERATOR"]) {
> we misspell Norwegian.

I think your joke detector is broken  :)

Comment 12

[Josh Aas]

There is some other importer work in the queue, and it would be helpful for this
to land before that continues. Me, Geoff, and Haas have all looked it over so it
should be an easy sr... just a ping for pink :)

Comment 13

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 160253 [details] [diff] [review]
patch v3

sure thing. josh, when you land this you get to change the indenting to 2-space
from 4-space :)

sr=pink

Comment 14

[Josh Aas]

landed on trunk with some whitespace cleanup