366358 - Bookmarks import from Firefox chose the wrong profile (doesn't let me choose profile)

Status: RESOLVED FIXED

(Camino Graveyard :: Bookmarks, defect)

Description

[Håkan Waara]

I have my life -- my bookmarks - in Firefox right now, and when I wanted to try the latest Camino builds, it just imports the default profile's bookmarks.

The UI should somehow let me choose which profile to import from.

Comment 1

[Håkan Waara]

Hmm, actually it didn't even import the default profile. 

Looking at the code, I'm having trouble figuring out how it could import anything at all, given that it looks for bookmarks in ~/Application Support/Firefox/Profiles/default, and all my profiles are in .../Profiles/<random junk>/

Comment 2

[Stuart Morgan]

No, it uses:
  [self getSaltedBookmarkPathForProfile:@"~/Library/Application Support/Firefox/Profiles/"];
The others are to cover older versions.

So we are picking whichever profile happens to be first, I guess on the assumption that there is just one. We should be able to handle the multiple profile case better if there's a reasonably straight-forward way to get a profile name from something in the folder (just showing the salt strings definitely wouldn't help).

Comment 3

[Chris Lawson (gone)]

Would last-modified date be a reasonable way of guessing which file was "most desired?" It doesn't solve the choosing issue, but it might be a better default behaviour without choosing.

cl

Comment 4

[Håkan Waara]

(In reply to comment #3)
> Would last-modified date be a reasonable way of guessing which file was "most
> desired?" It doesn't solve the choosing issue, but it might be a better default
> behaviour without choosing.

Yeah that sounds as a better default.

Comment 5

[Håkan Waara]

Working on a patch for the suggestion in comment 3.

Comment 6

[Håkan Waara]

Attachment: Patch v1

Here's a patch that uses the most recently modified profile.

* Almost all work is taken care of the by a new NSFileManager category method to find the last modified subfolder of a folder.

* Fixed a bug I accidently stumbled upon. buildAvailableFileList is called twice every time the import bookmarks window is shown. That could've lead to a real bug later. Once in the main startImportBookmarks method, and once later when the window nib instance was created in windowDidLoad.

Comment 7

[Chris Lawson (gone)]

Comment on attachment 251003 [details] [diff] [review]
Patch v1

>Index: src/bookmarks/BookmarkManager.mm
>===================================================================

> - (void)startImportBookmarks
> {
>   if (!mImportDlgController)
>     mImportDlgController = [[BookmarkImportDlgController alloc] initWithWindowNibName:@"BookmarkImportDlg"];
>-
>-  [mImportDlgController buildAvailableFileList];
>+    

No whitespace on blank lines, please.

>Index: src/extensions/NSFileManager+Utils.h
>===================================================================

>+// finds the latest modified subdirectory at the specified path. not recursive. returns nil on failure.
>+- (NSString *)latestModifiedSubDirectoryAtPath:(NSString *)path;

s/latest/last, since that's the standard Mac OS terminology. This requires changes to the method name throughout the patch as well as the comments.

>Index: src/extensions/NSFileManager+Utils.m
>===================================================================

>+  // find the latest modified profile
>+  NSArray *subdirs = [fm directoryContentsAtPath:fullPath];
>+  NSDate *latestModifiedSubDirDate = [[NSDate distantPast] retain];
>+  NSString *latestModifiedSubDir = nil;

s/latest/last again.

The style for the rest of this file is NSFoo* foo, rather than NSFoo *foo, so let's keep it the way it is rather than mixing styles. We have enough of that in the tree as it is.

Comment 8

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

I talked to Håkan and he said it'd be OK if someone else brought this to completion; he doesn't have a tree or time atm.

Comment 9

[froodian (Ian Leue)]

Attachment: Patch v2

Addresses review comments.

Comment 10

[Sean Murphy]

Comment on attachment 257053 [details] [diff] [review]
Patch v2


The r- concerns the patch's NSFileManager category method.

Since |lastModifiedSubDirectoryAtPath has been added via a category and is available for use throughout Camino, we need to take a few more steps to make sure it's robust enough and will do what it promises for all uses, not just when dealing with Firefox profile directory layouts.

The method's name indicates it will return a directory, but no steps are taken in code to ensure this in the case and right now it will simply return anything (file or directory) with the latest mod date.  Adding a few calls with the more explicit NSFileManager method |fileExistsAtPath:isDirectory:| can help determine that.

+  NSDate* lastModifiedSubDirDate = [[NSDate distantPast] retain];
..
+      [lastModifiedSubDirDate release];
+      lastModifiedSubDirDate = [currentDirModDate retain];
..
+  [lastModifiedSubDirDate release];

I'd prefer to avoid retaining/releasing objects if it's not really required.  While this may work, it's error (leak) prone and there are easier techniques to accomplish the task at hand without directly dealing with our objects' life-cycle.

+  NSFileManager* fm = [NSFileManager defaultManager];
...
+  NSArray* subdirs = [fm directoryContentsAtPath:fullPath];
+  NSDate* lastModifiedSubDirDate = [[NSDate distantPast] retain];
+  NSString* lastModifiedSubDir = nil;

I know I'm starting to nit pick (sorry!), but I think abbreviations (unless they're commonly accepted in Cocoa) aren't worth the keystrokes they save.  I think we might want to rename some variables here anyway, highlighting their purpose even more unequivocally.

+- (NSString *)lastModifiedSubDirectoryAtPath:(NSString *)inPath

Subdirectory is actually one word, so I'd suggest lowering the case of 'D' making the method |lastModifiedSubdirectoryAtPath:|.

The overall approach and the remaining code looks great.  Since Håkan is busy atm, I went ahead and reworked up a new patch based on everything I mentioned above.  It's late here, so I'm going wait until I look over my code in the morning and then submit those changes tomorrow.

Comment 11

[Sean Murphy]

Transferring this over to myself, since Håkan said it'd be OK if someone else took care of finishing it.

I don't mean to jump in and steal the show froodian; I spent so much time looking at lastModifiedSubDirectoryAtPath: that I wanted to incorporate ideas which came into my head concerning the code/technique of it.

I'm addressing mostly the comments I made above, and will submit a new patch shortly.

Comment 12

[Sean Murphy]

Attachment: Patch v3

Re-implements the NSFileManager category method.

> +    if ([subpath hasPrefix:@"."])
> +      continue;

I have it ignore hidden directories at the moment; the original profile searching code (before it was even a NSFileManager extension) performed this check and the previous patches had also done the same.

Comment 13

[froodian (Ian Leue)]

Comment on attachment 257623 [details] [diff] [review]
Patch v3

r=me

Comment 14

[Håkan Waara]

Comment on attachment 257623 [details] [diff] [review]
Patch v3

Looks good to me too.

Comment 15

[Stuart Morgan]

Comment on attachment 257623 [details] [diff] [review]
Patch v3

>-  [mImportDlgController buildAvailableFileList];

Can someone justify this change please? Comment 6 says it could lead to a bug, but I don't see what it is, and there's a comment in buildAvailableFileList explicitly saying why it should be called again (namely, profiles might change over the course of Camino being open).

Comment 16

[Håkan Waara]

(In reply to comment #15)
> (From update of attachment 257623 [details] [diff] [review])
> >-  [mImportDlgController buildAvailableFileList];
> 
> Can someone justify this change please? Comment 6 says it could lead to a bug,
> but I don't see what it is, and there's a comment in buildAvailableFileList
> explicitly saying why it should be called again (namely, profiles might change
> over the course of Camino being open).

I think you're right, but there's still no need for the double call to buildAvailableFileList, AFAICS.

Every time "Import bookmarks..." is chosen, startImportBookmarks: is called (http://mxr.mozilla.org/seamonkey/source/camino/src/application/MainController.mm#1186) which will call buildAvailableFileList.

The first time startImportBookmarks: is called, it will load the window nib, which will invoke windowDidLoad: which will call buildAvailableFileList.

So the correct thing is to remove the extra call from windowDidLoad: instead because it will already be done a bit later. Does that sound good?

Comment 17

[Sean Murphy]

Attachment: Patch v3.1

I think the best scenario here would involve not forcing BookmarkManager to have any knowledge that buildAvailableFileList needs to be performed before showing the import dialog's window.  This prerequisite seems more like an implementation detail of BookmarkImportDlgController and thus its own responsibility.  BookmarkManager should be allowed to simply show the dialog and not be expected to handle any of its UI initialization.  Otherwise, the initial dialog setup would be spread out and involve buildAvailableFileList in BookmarkManager and showImportView BookmarkImportDlgController.

Håkan was originally doing this back in Patch v1, moving buildAvailableFileList out of the BookmarkImportDlgController header and into the private method category.

Also, as mentioned in the previous comment, windowDidLoad isn't a good place either if we want to encapsulate the UI initialization into BookmarkImportDlgController and still allow buildAvailableFileList to execute each time the window is shown.  The import window is kept around in memory after it has been closed and any subsequent showWindow: calls do not result in the windowDidLoad method being called.

One approach is to override the -[NSWindowController showWindow:] in BookmarkImportDlgController.  This seemed like the best alternative to me, since there is no "perfect" NSWindow delegate method or other notification for this task (e.g., windowDidBecomeKey would be called more often than needed).  The updated patch follows this technique.

I also thought about keeping the UI setup procedure in windowDidLoad and instead ensuring the window is released when closed, thus forcing it to load each time.  Despite setting the isReleasedWhenClosed property on the window, it wouldn't release or prompt further windowDidLoad calls.  If this is a better approach I can figure out what the deal is.

Comment 18

[Håkan Waara]

(In reply to comment #17)

Unless there are other public methods to bring up this, other than startImportBookmarks, I still think the best place for any initialization (i.e. buildAvailableFilelist) makes most sense in startImportBookmarks. Overriding showWindow: would be complicating the matter unnecessarily, imho.

Also, can you add a comment to the buildAvailableFilelist call why it's important? 

Comment 19

[Håkan Waara]

Oh, and while you're at it, I'm sure there are more currently public methods that could be moved to the private category. For example, the finishThreadedImport: sounds kind of private?

Comment 20

[Stuart Morgan]

(In reply to comment #19)
> Oh, and while you're at it, I'm sure there are more currently public methods
> that could be moved to the private category. For example, the
> finishThreadedImport: sounds kind of private?

Let's not mix unrelated issues into this bug please.

Comment 21

[Sean Murphy]

Attachment: Patch v3.2

Updated patch, does what Håkan suggested in comment #16:
> So the correct thing is to remove the extra call from windowDidLoad: instead
> because it will already be done a bit later.

(In reply to comment #18)
Yeah, the more I look at the situation, you've got a good point.  Additionally, showWindow: isn't a typically overridden method anyway, and it felt kinda unnatural doing that.

> Also, can you add a comment to the buildAvailableFilelist call why it's
> important? 

There were no method comments in the header, so for now I just rephrased the existing comment above the implementation.  I wanted to indicate that calling it is a prerequisite for any external code (BookmarkManager) showing the dialog.

Comment 22

[Sean Murphy]

I should have noticed this before submitting the last patch, but it turns out that calling buildAvailableFileList before showWindow: will not populate the UI.  The window has not yet initialized at this point and BookmarkImportDlgController's outlets are nil.

So, without going back to finding a way for BookmarkImportDlgController to handle all of its initialization internally, the workaround is to call buildAvailableFileList after the dialog is on screen:
-  [mImportDlgController buildAvailableFileList];
   [mImportDlgController showWindow:nil];
+  [mImportDlgController buildAvailableFileList];

This can result in the popup button's selected item changing from "Select a file…" to a browser name in front of the user, albeit very quickly.

Before I resubmit another patch with the above change, does this seem reasonable?  If not, maybe the best approach would be to force the import window to release when closed and just utilize windowDidLoad after all.

Comment 23

[Stuart Morgan]

In the spirit of comment 20: roughly half of this bug is now discussion about something completely unrelated to the bug itself. How about we get a patch here that fixes the bug this was filed about, and people can file new bugs to do cleanup that has no connection to the changes that need to be made to fix the problem.

Comment 24

[Sean Murphy]

Attachment: Patch v3 minus unrelated fixes

Sorry for going off track.  This is Patch v3, without the few lines irrelevant in choosing the correct Firefox profile.  I'll submit a bug report for the buildAvailableFileList issues when I have time tonight.

Comment 25

[Stuart Morgan]

Comment on attachment 261562 [details] [diff] [review]
Patch v3 minus unrelated fixes

>+// Given a Mozilla-like profile, returns the bookmarks.html file in the salt directory for the last modified profile,
>+// or nil on error

Break this comment after "directory" to keep the line length down.


>+// finds the last modified subdirectory at the specified path. not recursive. 
>+// ignores hidden directories and will return nil on failure.
>+- (NSString *)lastModifiedSubdirectoryAtPath:(NSString *)path;

Since these are (more or less) sentences, go ahead and capitalize them accordingly.

>+    NSDate* currentFileModificationDate = [[fileManager fileAttributesAtPath:subpath traverseLink:NO] fileModificationDate];

Again, just for line length, break this as

    NSDate* currentFileModificationDate = [[fileManager fileAttributesAtPath:subpath
                                                                traverseLink:NO] fileModificationDate];

sr=smorgan with those changes.

Comment 26

[froodian (Ian Leue)]

Checked in on trunk and MOZILLA_1_8_BRANCH with the above changes as checkin loving.

Comment 27

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

Do we want to file a new bug on Håkan's original issue (the inability to choose if there are multiple profiles), or is choosing the most recently changed profile good enough?