Closed
Bug 242673
Opened 20 years ago
Closed 20 years ago
Bookmark importer should recognize Opera
Categories
(Camino Graveyard :: Bookmarks, enhancement)
Tracking
(Not tracked)
RESOLVED
FIXED
Camino0.9
People
(Reporter: stuart.morgan+bugzilla, Assigned: jaas)
Details
Attachments
(1 file, 2 obsolete files)
10.32 KB,
patch
|
me
:
review+
mikepinkerton
:
superreview+
|
Details | Diff | Splinter Review |
Opera is not currently supported by our bookmark import, while almost everything else is.
Comment 2•20 years ago
|
||
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.
As long as we have a patch, marking 0.9.
Target Milestone: Future → Camino0.9
Comment 5•20 years ago
|
||
missed a line. sorry about that.
Attachment #157992 -
Attachment is obsolete: true
Comment 6•20 years ago
|
||
ping
Attachment #158026 -
Flags: review?(joshmoz)
+// 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 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-
Addressed my own comments in order to get this landed fast.
Attachment #158026 -
Attachment is obsolete: true
Attachment #160253 -
Flags: review?(me)
Comment 10•20 years ago
|
||
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
Updated•20 years ago
|
Attachment #160253 -
Flags: review?(me) → review+
Comment 11•20 years ago
|
||
(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 :)
Attachment #160253 -
Flags: superreview?(pinkerton)
Attachment #160253 -
Flags: review?
Attachment #160253 -
Flags: review?
Assignee | ||
Comment 12•20 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 13•20 years ago
|
||
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•20 years ago
|
||
landed on trunk with some whitespace cleanup
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•