304118 - improve export of menu spacer / bookmark separator

Status: RESOLVED FIXED

(Camino Graveyard :: Bookmarks, defect)

Description

[Lars Frers]

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="">&lt;Menu Spacer&gt;</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.

Comment 1

[Jasper]

Sounds like a solid request. Seems like it would be a very welcome and proper
thing to have for 1.0.

Comment 2

[Chris Lawson (gone)]

OK, I have a working patch for this, but I need someone to walk me through the
bugzilla patch submission process.

Any volunteers? :)

cl

Comment 3

[Chris Lawson (gone)]

Attachment: Patch to check if item is separator and export an HR if it is

Looks like I figured the patching system out ;)

cl

Comment 4

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

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)

Comment 5

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

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)

Comment 6

[Chris Lawson (gone)]

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

Comment 7

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

(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 8

[Wevah (Nate Weaver)]

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

Comment 9

[Wevah (Nate Weaver)]

(In reply to comment #8)
> (From update of attachment 192329 [details] [diff] [review] [edit])
> 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.

Comment 10

[Wevah (Nate Weaver)]

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.

Comment 11

[Chris Lawson (gone)]

Attachment: Updated patch per Jeff's suggestions

Here you go, Jeff.

cl

Comment 12

[Wevah (Nate Weaver)]

Comment on attachment 192411 [details] [diff] [review]
Updated patch per Jeff's suggestions

Yay! r=me.

P.S. Who is Jeff? ;)

Comment 13

[Wevah (Nate Weaver)]

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.

Comment 14

[Wevah (Nate Weaver)]

Nevermind; it probably doesn't matter. Sorry I'm so indecisive today.

Comment 15

[Chris Lawson (gone)]

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

Comment 16

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 192411 [details] [diff] [review]
Updated patch per Jeff's suggestions

sr=pink

Comment 17

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

This should get checked in before we all forget about it :-)

Comment 18

[Chris Lawson (gone)]

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

Comment 19

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

Bug 307743 landed, so this should be fixed now per comment 18.