User-Agent: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7) Gecko/20040428 Camino/0.8b Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7) Gecko/20040428 Camino/0.8b When importing Mozilla bookmarks, the separators are not imported. Reproducible: Always Steps to Reproduce: 1. Select "Import Bookmarks..." in the "Camino" menu 2. Select "Netscape/Mozilla" in the import dialog and click "Import" 3. Mozilla Bookmarks are correctly imported but without any separator Actual Results: Mozilla bookmarks separators are not imported. Expected Results: It would be cool if separators could be imported too. This bug was discovered when importing from Mozilla 1.6 : Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.6) Gecko/20040113

I had a look at what to do to solve this. I remember we had some trouble (crashes) with importing Mozilla bm files with separators. In Bookmark.h a separator is defined as follows: #define kBookmarkSpacerStatus 9 Which is a defenition based entirely on what a separator is in Camino. In our plist files a separator is defined as follows: <dict> <key>Status</key> <integer>9</integer> </dict> But in Mozilla a separator is defined in the xml file just as a <HR> tag. So there lies the problem. While we only check if the integer is 9 (as shown below) we are not looking if it perhaps is a <HR> tag. And thus we fail to import them from the xml file. -(BOOL) isSeparator { if ([self status]==kBookmarkSpacerStatus) return YES; return NO; } Hope this helps to solve this.

Looks like a pretty simple change to the html import parser. I'll try to take a look in the next couple of weeks, since I've seen other complaints about this. Anyone have a bug number for the crash on separators? I'm not able to find it.

We used to have crashers we don't any more. The original patch from David had trouble.

Recognizes <HR as a menu separator. Also cleans up some useless uppercase/lowercase checks.

welcome back david. YOu you care setting the review flags ?

Hey David, how wonderfull to see you around.

Comment on attachment 157994 [details] [diff] [review] import separators >Index: BookmarkManager.mm >=================================================================== >RCS file: /cvsroot/mozilla/camino/src/bookmarks/BookmarkManager.mm,v >retrieving revision 1.17 >diff -u -4 -r1.17 BookmarkManager.mm >--- BookmarkManager.mm 2 Sep 2004 20:21:40 -0000 1.17 >+++ BookmarkManager.mm 6 Sep 2004 06:49:36 -0000 >@@ -749,22 +758,23 @@ > unsigned long scanIndex = 0; > NSRange tempRange, keyRange; > BOOL justSetTitle = NO; > // Scan through file. As we find a token, do something useful with it. >+ [fileScanner setCaseSensitive:NO]; > while (![fileScanner isAtEnd]) { > [fileScanner scanUpToString:@"<" intoString:&tokenString]; > scanIndex = [fileScanner scanLocation]; > if ((scanIndex+3) < [fileAsString length]) { > tokenTag = [[NSString alloc] initWithString:[fileAsString substringWithRange:NSMakeRange(scanIndex,3)]]; > // now we pick out if it's something we want to save. > // check in a "most likely thing first" order >- if (([tokenTag isEqualToString:@"<DT "]) || ([tokenTag isEqualToString:@"<dt "])) { >+ if ([tokenTag isEqualToString:@"<DT "]) { This does not work and should not be modified. The tokenTag is not Case insensitive, only scanning to "<" is. >+ // Firefox menu separator >+ else if ([tokenTag isEqualToString:@"<HR"]) { >+ currentItem = [currentArray addBookmark]; >+ [(Bookmark *)currentItem setIsSeparator:TRUE]; >+ [fileScanner setScanLocation:(scanIndex+1)]; >+ } > else { //beats me. just close the tag out and continue. > [fileScanner scanUpToString:@">" intoString:NULL]; > } > [tokenTag release]; Will review this part once the upper part of the patch is cleaned up.

fixed

I'd r+ this patch because it improves the situation as it is meant to, but perhaps it could include a fix for this issue I found while reviewing it. Extremely scary: Import Firefox bookmarks. See how they appear in the bottom of the left-hand bookmark folder list. Select the new imported bookmarks folder. See Firefox bookmarks on the right. Now drag the Firefox bookmarks folder up and drop it on your Bookmark Menu folder. The Bookmark Menu folder is selected on the left, but the view never changes on the right, and it appears that you just overwrote all of your bookmarks with the imported ones. You didn't really, if you force the bookmark manager to update its right-hand view, but it was hella scary for a few seconds. If you want to make this problem a different bug then I'll r+ this and we should land it, followed shortly by a fix for the above issue.

let's file another bug for that

Comment on attachment 158031 [details] [diff] [review] fixed sr=pink will land shortly

landed.

landed on branch