improve export of menu spacer / bookmark separator

RESOLVED FIXED in Camino1.0

Status

defect
RESOLVED FIXED
14 years ago
14 years ago

People

(Reporter: der.ozean, Assigned: mikepinkerton)

Tracking

({fixed1.8})

unspecified
Camino1.0
PowerPC
macOS
Dependency tree / graph

Details

Attachments

(2 obsolete attachments)

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.
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
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+
Attachment #192329 - Flags: review+ → review-
(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.
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.
Attachment #192329 - Attachment is obsolete: true
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 :-)
Attachment #192411 - Attachment is obsolete: true
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: 14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.