Last Comment Bug 322093 - Camino initial bookmark import APPEARS to hang and stall w/ large bookmark imports
: Camino initial bookmark import APPEARS to hang and stall w/ large bookmark im...
Status: RESOLVED FIXED
: fixed1.8.1, perf
Product: Camino Graveyard
Classification: Graveyard
Component: Bookmarks (show other bugs)
: unspecified
: PowerPC Mac OS X
P2 normal (vote)
: Camino1.5
Assigned To: David Haas (gone, not reading mail)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2006-01-01 17:19 PST by John Spooner
Modified: 2006-09-11 13:21 PDT (History)
9 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
a patch (20.35 KB, patch)
2006-08-18 19:21 PDT, David Haas (gone, not reading mail)
hwaara: review-
Details | Diff | Splinter Review
nib file for patch (5.84 KB, application/octet-stream)
2006-08-18 19:23 PDT, David Haas (gone, not reading mail)
no flags Details
new patch (21.35 KB, patch)
2006-08-30 20:59 PDT, David Haas (gone, not reading mail)
hwaara: review+
mikepinkerton: superreview+
Details | Diff | Splinter Review
import dialog nib (5.85 KB, application/octet-stream)
2006-08-30 21:00 PDT, David Haas (gone, not reading mail)
froodian: review+
Details

Description User image John Spooner 2006-01-01 17:19:33 PST
User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7.8) Gecko/20050511 Firefox/1.0.4
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7.8) Gecko/20050511 Firefox/1.0.4

Per request, I am copying this over from Bug report:  to a less critical status. See explanation below:

3 reasons (minor bugs) I THOUGHT my Firefox bookmarks were not importing:

1) Blue "Import" button in window did not react after clicking -- i.e. button
did not flash, window did not close, no progress bar, nothing but spinning
beachball.

2) Beachball spun for a LONG time with no apparent progress.

3) Simultaneously the Command+Option+Esc, Force Quit Applications window showed
"Not responding" for Camino.

After multiple tries, I decided to try to import from Explorer where I have
about 3,000 bookmarks. Same apparent results but just as I was about to Force Quit
Explorer, all of a sudden, Camino's bookmarks window filled up.

So I decided to try Firefox again (where I have about 5,000 bookmarks) but just
let it run a while even though Mac says "Not responding".  In about 3 to 4
minutes of spinning beachball, all bookmarks successfully imported.

I believe that 1.-3. above should still be reported as "minor" to "medium"
level bugs because the Mac OS and Camino are not interfacing the way a Mac user
would expect.

However, a simple footnote to the effect that, "Note: When importing large
numbers of bookmarks, Camino bookmark window may not close, spinning beachball
may occur and Mac "Force Quit" window may say Camino is "Not responding",
however let Import run at least 5 minutes, as it is probably importing. 

Per Comment #13 From Smokey Ardisson (sporadic 16 Dec - 10 Jan)  2006-01-01 12:12 PST  [reply] -------

 It seems at the very least we should be showing our own wait
cursor there...

I agree with Smokey, some kind of status window or messaging to indicate Camino is actually working and not hanging would be a great improvement, especially since Command + Option + Esc window on Mac erroneously states "Not responding"

Thanks,  John


Reproducible: Always

Steps to Reproduce:
1. Launch Camino.
2. File -> Import bookmarks... -> Firefox
3.

Actual Results:  
Imports as it should, but interface is misleading -- see "details".   

Expected Results:  
Import bookmarks, but tell user bookmarks are importing, working show scroll bar or something so user understands it only appears that Camino is hanging.
Comment 1 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-01-01 19:25:54 PST
John, can you attach your Firefox bookmarks file so the we can have something to reproduce this with?  Most of us don't have 3-5K bookmark entries ;)
Comment 2 User image Jasper 2006-01-02 01:27:00 PST
This makes me think we should show some sort of progress dialog when importing bookmarks. I know that for small bookmark imports this wouldn't be necesarry, but it wouldn't bother to many people either.
Comment 3 User image Chris Lawson (gone) 2006-01-02 06:18:41 PST
(In reply to comment #2)
> This makes me think we should show some sort of progress dialog when importing
> bookmarks. I know that for small bookmark imports this wouldn't be necesarry,
> but it wouldn't bother to many people either.
 
And for small bookmark imports, it would probably disappear so fast that it wouldn't even be noticeable.

I agree with this approach.

cl
Comment 4 User image Jasper 2006-01-02 10:57:31 PST
The behaviour I'd expect to see would be:

1) Choose a file to import.
2) import window closes and we open a new tab or window showing the bookmark manager.
3) a sheet drops down showing the progress of importing.
4) the imported folder is selected on completion of the import.
Comment 5 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-01-31 02:09:25 PST
FWIW, I've added a note to the new (forthcoming) text for the bookmarks section of the website that large bookmarks files might take a LONG time to import, so it's sort-of documented until we get some UI/UE fixes here.
Comment 6 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-08-13 01:22:25 PDT
Some sort of progress notification would be nice for 1.1, but this is in danger of not making it.

Comment 5 is live now on cb.o
Comment 7 User image David Haas (gone, not reading mail) 2006-08-18 19:21:04 PDT
Created attachment 234537 [details] [diff] [review]
a patch

patch does 2 things:
1) flips up a little "import in progress" window 
2) does most of the import on a background thread
Comment 8 User image David Haas (gone, not reading mail) 2006-08-18 19:23:08 PDT
Created attachment 234538 [details]
nib file for patch

Modified Nib file needed by patch
Comment 9 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-08-19 13:56:20 PDT
Comment on attachment 234537 [details] [diff] [review]
a patch

Thanks for the patch, David; good to see you hacking again ;)

I tested this on the 800K HTML file generated from exporting the canonical huge .plist, and it made a noticeable difference in speed (in addition to the visual aid provided by the new nib).

I think the progress window looks too small (and should use the real elipsis), but that's just minor ;)
Comment 10 User image Håkan Waara 2006-08-28 12:23:48 PDT
Comment on attachment 234537 [details] [diff] [review]
a patch

Overall looks great. Here are some things I spotted.

>@@ -141,9 +143,9 @@
> - (void)copyBookmarksURLs:(NSArray*)bookmarkItems toPasteboard:(NSPasteboard*)aPasteboard;
> 
> // Importing bookmarks
> - (void)startImportBookmarks;
>-- (BOOL)importBookmarks:(NSString *)pathToFile intoFolder:(BookmarkFolder *)aFolder;
>+- (void)importBookmarksThreadEntry:(id)aDict;

Please use the explicit type |NSDictionary*| where ever possible (i.e., also the |importBookmarksThreadThreadReturn| one).

>@@ -74,8 +75,15 @@
> static NSString* const kRendezvousFolderIdentifier     = @"RendezvousFolder";   // aka Bonjour
> static NSString* const kAddressBookFolderIdentifier    = @"AddressBookFolder";
> static NSString* const kHistoryFolderIdentifier        = @"HistoryFolder";
> 
>+NSString* const kBookmarkImportPathIndentifier = @"path";
>+NSString* const kBookmarkImportNewFolderNameIdentifier = @"title";
>+
>+static NSString* const kBookmarkImportStatusIndentifier = @"flag";
>+static NSString* const kBookmarkImportNewFolderIdentifier = @"folder";
>+static NSString* const kBookmarkImportNewFolderIndexIdentifier = @"index";

Please line up these.

>+
> // these are suggested indices; we only use them to order the root folders, not to access them.
> enum {
>   kBookmarkMenuContainerIndex = 0,
>   kToolbarContainerIndex = 1,
>@@ -110,8 +118,9 @@
> // these are "import" methods that import into a subfolder.
> - (BOOL)importHTMLFile:(NSString *)pathToFile intoFolder:(BookmarkFolder *)aFolder;
> - (BOOL)importCaminoXMLFile:(NSString *)pathToFile intoFolder:(BookmarkFolder *)aFolder settingToolbarFolder:(BOOL)setToolbarFolder;
> - (BOOL)importPropertyListFile:(NSString *)pathToFile intoFolder:(BookmarkFolder *)aFolder;
>+- (void) importBookmarksThreadReturn:(id)aDict;

Please use the prevailing code style in this file (no space after (void)). There are a few other places in the patch where the new code doesn't match the prevailing style, can you please just have a quick look to see if you can fix them?

>@@ -1435,41 +1449,106 @@
>   [mImportDlgController buildAvailableFileList];
>   [mImportDlgController showWindow:nil];
> }
> 
>-- (BOOL)importBookmarks:(NSString *)pathToFile intoFolder:(BookmarkFolder *)aFolder
>+- (void)importBookmarksThreadEntry:(id)aDict
> {

I feel this method is becoming huge when the whole import loop goes into it. Would it be possible to keep the "import one bookmark" method, and have the loop call that on every iteration (with the right arguments)?

pseudo-code:

for-each boomark b,
  import(b)

Also, I propose to rename the importThread* methods to something more descriptive. How about:

startImportBookmarksThread: and endImportBookmarksThread:, or similar?
Comment 11 User image David Haas (gone, not reading mail) 2006-08-30 20:59:15 PDT
Created attachment 236186 [details] [diff] [review]
new patch

Changes from previous patch:

*explicitly declares NSDictionary instead of id
*lines up strings in source file
*fixes a few indents

that's it.

To address the 'method too big, try to keep single bookmark import method from before' - the old method imported a single file at once, not a single bookmark.  In most cases, that's exactly how this method will work.  It's only in the Omniweb 5 case that you'd get passed more than one file to import at a time (possibly up to 3).  Doing it this way kept things thread safe & undo-able, where I couldn't figure out how to do that for Omniweb with a single file per call. 

As for re-naming the methods, most of the import-bookmark methods start with import*.  If somebody else feels like changing them to match some other naming scheme, be my guest.
Comment 12 User image David Haas (gone, not reading mail) 2006-08-30 21:00:40 PDT
Created attachment 236187 [details]
import dialog nib

I changed the string to have real ellipses.
Comment 13 User image froodian (Ian Leue) 2006-09-02 00:27:21 PDT
Comment on attachment 236187 [details]
import dialog nib

r=me on this nib.
Comment 14 User image Håkan Waara 2006-09-04 16:33:34 PDT
Comment on attachment 236186 [details] [diff] [review]
new patch

Looks great!

David, maybe you'd also be interested in bug 351351.
Comment 15 User image Stuart Morgan 2006-09-04 22:06:11 PDT
Comment on attachment 236186 [details] [diff] [review]
new patch

>+#import <pthread.h>
...
>+  if (pthread_main_np())
>+      return mUndoManager;
>+  return nil;

It might be nice to use the NSThread+Utils inMainThread here instead, for clarity.
Comment 16 User image Mike Pinkerton (not reading bugmail) 2006-09-11 06:38:13 PDT
Comment on attachment 236186 [details] [diff] [review]
new patch

looks ok to me, we'll have to get a lot of testing on this.

why use the pthread api directly rather than NSThread?

sr=pink
Comment 17 User image froodian (Ian Leue) 2006-09-11 13:21:20 PDT
Fixed on trunk and 1.8branch (with checkin lovin' to use the NSThread method).  Thanks for the fix David!  Hope to see your face around here often. ;)

Note You need to log in before you can comment on or make changes to this bug.