If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Bookmark importer should recognize Opera

RESOLVED FIXED in Camino0.9

Status

Camino Graveyard
Bookmarks
--
enhancement
RESOLVED FIXED
14 years ago
13 years ago

People

(Reporter: Stuart Morgan, Assigned: Josh Aas)

Tracking

unspecified
Camino0.9
PowerPC
Mac OS X

Details

Attachments

(1 attachment, 2 obsolete attachments)

10.32 KB, patch
Geoff "temporarily off bugmail" Beier
: review+
Mike Pinkerton (not reading bugmail)
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

14 years ago
Opera is not currently supported by our bookmark import, while almost everything
else is.
whatever.
Target Milestone: --- → Future
Created attachment 157992 [details] [diff] [review]
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.
(Assignee)

Comment 3

13 years ago
got it
Assignee: pinkerton → joshmoz
(Assignee)

Comment 4

13 years ago
As long as we have a patch, marking 0.9.
Target Milestone: Future → Camino0.9
Created attachment 158026 [details] [diff] [review]
fixed patch

missed a line.	sorry about that.
Attachment #157992 - Attachment is obsolete: true
ping
(Assignee)

Updated

13 years ago
Attachment #158026 - Flags: review?(joshmoz)
(Assignee)

Comment 7

13 years ago
+// 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.
(Assignee)

Comment 8

13 years ago
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.
Attachment #158026 - Flags: review?(joshmoz) → review-
(Assignee)

Comment 9

13 years ago
Created attachment 160253 [details] [diff] [review]
patch v3

Addressed my own comments in order to get this landed fast.
Attachment #158026 - Attachment is obsolete: true
(Assignee)

Updated

13 years ago
Attachment #160253 - Flags: review?(me)
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=me@mollyandgeoff.com
Attachment #160253 - Flags: review?(me) → review+
(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  :)
(Assignee)

Updated

13 years ago
Attachment #160253 - Flags: superreview?(pinkerton)
Attachment #160253 - Flags: review?
(Assignee)

Updated

13 years ago
Attachment #160253 - Flags: review?
(Assignee)

Comment 12

13 years ago
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 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
Attachment #160253 - Flags: superreview?(pinkerton) → superreview+
(Assignee)

Comment 14

13 years ago
landed on trunk with some whitespace cleanup
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.