Closed Bug 304118 Opened 18 years ago Closed 18 years ago
improve export of menu spacer / bookmark separator
User-Agent: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b4) Gecko/20050809 Camino/0.9a2+ Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8b4) Gecko/20050809 Camino/0.9a2+ Camino uses its own format for separators when exporting bookmarks into an html file, it would save a lot of work on import into other browsers if the same format as in other browsers of the Mozilla family would be used: the <hr> element Reproducible: Always Steps to Reproduce: 1. Export Camino Bookmarks containing a separator as HTML 2. Import ExportedBookmarkFile.html into other browser Actual Results: Menu separators are displayed as bookmarks with the name "Menu Spacer" not separators. The entry in the exported file reads: <DT><A HREF=""><Menu Spacer></A> Expected Results: Menu separators are displayed as such (if available in the browser). Firefox, Mozilla and iCab use <hr> elements for separators. Opera is at least clever enough to ignore <hr> elements on import.
Sounds like a solid request. Seems like it would be a very welcome and proper thing to have for 1.0.
Target Milestone: --- → Camino1.0
OK, I have a working patch for this, but I need someone to walk me through the bugzilla patch submission process. Any volunteers? :) cl
Looks like I figured the patching system out ;) cl
Attachment #192329 - Flags: review+
Comment on attachment 192329 [details] [diff] [review] Patch to check if item is separator and export an HR if it is Not quite ;) ? to ask for a review (with an email address if you have someone in mind)
Attachment #192329 - Flags: review+ → review?
Oops, adding Chris to the cc so he'll see my last comment :-\ (I'm assuming the fumbled + meant you really thought the patch was ready for review...if not, clear the ? flag by editing the attachment)
Yep, that's it. Woops. Thanks, Smokey I really have no idea who'd be best to review this...Mike? Simon? Anyone? Bueller? I don't suppose anyone's considered writing a Bugzilla first-timers FAQ, have they...? cl
(In reply to comment #6) > I don't suppose anyone's considered writing a Bugzilla first-timers FAQ, have > they...? http://www.caminobrowser.org/development/dev_dev.html#DevelopmentCycle is what we have now. In the old days, there used to be something with more detail (i.e., Mike doesn't do reviews, just sr, etc.)...at any rate, discussion of this belongs in its own bug or at docs.cb.o somewhere.
Comment on attachment 192329 [details] [diff] [review] Patch to check if item is separator and export an HR if it is Looks good to me. r=me
Attachment #192329 - Flags: review? → review+
(In reply to comment #8) > (From update of attachment 192329 [details] [diff] [review] ) > Looks good to me. r=me > Er, wait. No sense in assigning formatString twice. It should probably read like this: else if ([self isSeparator]) formatString = @"%@<HR>\n"; else formatString = @"%@<DT><A HREF=\"%@\">%@</A>\n"; Change that, and *then* r=me. Sorry 'bout that.
On third thought, [self isSeparator] should probably be the first thing checked in the group of ifs, just in case isSeparator is true but mDescription > 0. Sorry for the quasi-spam.
Here you go, Jeff. cl
Attachment #192411 - Flags: review?
Attachment #192411 - Flags: review? → review?(mozilla)
Comment on attachment 192411 [details] [diff] [review] Updated patch per Jeff's suggestions Yay! r=me. P.S. Who is Jeff? ;)
Attachment #192411 - Flags: review?(mozilla) → review+
Though I almost wonder if it wouldn't be better to have |if (![self isSeparator])| first, with a description check inside, since that's the most common case. I shall ask.
Nevermind; it probably doesn't matter. Sorry I'm so indecisive today.
Comment on attachment 192411 [details] [diff] [review] Updated patch per Jeff's suggestions OK, so next step is super-review, right? I read through the sr docs but neither Simon nor Mike (the two most likely candidates for sr on this one, methinks) are listed there. Setting sr to Mike for now, apologies in advance if I'm jumping the gun on this one. cl
Attachment #192411 - Flags: superreview?(pinkerton)
Comment on attachment 192411 [details] [diff] [review] Updated patch per Jeff's suggestions sr=pink
Attachment #192411 - Flags: superreview?(pinkerton) → superreview+
This should get checked in before we all forget about it :-)
This will be fixed by the patch to bug 307743. Feel free to mark this bug as dependent on that one. There's probably no point in checking this patch in now that it's going to be immediately superseded. cl
Bug 307743 landed, so this should be fixed now per comment 18.
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.