Camino initial bookmark import APPEARS to hang and stall w/ large bookmark imports

RESOLVED FIXED in Camino1.5

Status

Camino Graveyard
Bookmarks
P2
normal
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: John Spooner, Assigned: David Haas (gone, not reading mail))

Tracking

({fixed1.8.1, perf})

unspecified
Camino1.5
PowerPC
Mac OS X
fixed1.8.1, perf

Details

Attachments

(2 attachments, 2 obsolete attachments)

21.35 KB, patch
Håkan Waara
: review+
Mike Pinkerton (not reading bugmail)
: superreview+
Details | Diff | Splinter Review
5.85 KB, application/octet-stream
froodian (Ian Leue)
: review+
Details
(Reporter)

Description

12 years ago
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.
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 ;)

Updated

12 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: perf

Comment 2

12 years ago
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

12 years ago
(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

12 years ago
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.
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.

Updated

12 years ago
Priority: -- → P2
Target Milestone: --- → Camino1.1
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
Assignee: mikepinkerton → nobody
Keywords: helpwanted
QA Contact: bookmarks
(Assignee)

Comment 7

11 years ago
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
(Assignee)

Comment 8

11 years ago
Created attachment 234538 [details]
nib file for patch

Modified Nib file needed by patch
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 ;)
Attachment #234537 - Flags: review?
Attachment #234537 - Flags: review? → review?(hwaara)
Keywords: helpwanted

Comment 10

11 years ago
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?
Attachment #234537 - Flags: review?(hwaara) → review-
(Assignee)

Comment 11

11 years ago
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.
Attachment #234537 - Attachment is obsolete: true
(Assignee)

Comment 12

11 years ago
Created attachment 236187 [details]
import dialog nib

I changed the string to have real ellipses.
Attachment #234538 - Attachment is obsolete: true
Attachment #236186 - Flags: review?(hwaara)
Attachment #236187 - Flags: review?(hwaara)

Comment 13

11 years ago
Comment on attachment 236187 [details]
import dialog nib

r=me on this nib.
Attachment #236187 - Flags: review?(hwaara) → review+

Comment 14

11 years ago
Comment on attachment 236186 [details] [diff] [review]
new patch

Looks great!

David, maybe you'd also be interested in bug 351351.
Attachment #236186 - Flags: superreview?(mikepinkerton)
Attachment #236186 - Flags: review?(hwaara)
Attachment #236186 - Flags: review+

Comment 15

11 years ago
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 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
Attachment #236186 - Flags: superreview?(mikepinkerton) → superreview+

Updated

11 years ago
Assignee: nobody → david.haas

Comment 17

11 years ago
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. ;)
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.