250386 - (SearchEnginePrefs) Camino should look for user defined earchURLList.plist first before using the default list.

Status: VERIFIED FIXED

(Camino Graveyard :: Toolbars & Menus, enhancement)

Description

[David Fedoruk]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7) Gecko/20040707 Camino/0.8
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.7) Gecko/20040707 Camino/0.8

Search engine default by editing the SearchURLList.plist in
/Applications/Camino.app/Contents/Resources/English.lproj. However since this
file is in the Camino.app, it will be clobbered next time a new Camino.app upgraded.

Reproducible: Always
Steps to Reproduce:
1. Edit /Applications/Camino.app/Contents/Resources/English.lproj as described
in help menu.

2. Open Camino and see the results of your edits (new search engines!) 

3. Upgrade Camino to the latest nightly.



Actual Results:  
Changes to SearchURLList.plist have been lost becuase that file is in the
Camino.app itself and not in ~/Library/Applicantion/Support/Camino

Expected Results:  
FireFox has already implimented a way to change or add search engines from the
Search engine field in the Navigation Tool Bar. Camino should impliment this
feature.

Comment 1

[Mike Pinkerton (not reading bugmail)]

this is a dupe, i think i wontfix'd the other bug

Comment 2

[Jasper]

Huh, why would you WONTFIX this? It can do no harm and even has the benefit that
it can be used for localisation!

The toolbar search item should have more freedom accross upgrades. Especially as
we tell on our website that people can customize. We don't want to detroy their
customisations do we?

Comment 3

[Jasper]

Never mind the localistation part :) The rest still stands

Comment 4

[Bruce Davidson]

I have asked David to clarify his intent with this bug. It appears to ask for
the UI (which as Mike notes has been previously WONTFIXed as an enhancement
request). However the posting on the mailing list appeared to indicate this bug
should just cover reading the search engine list from the user's profile rather
than the app bundle.

I have a patch to do this which I will attach here on the assumption that this
is what David meant this bug to cover.

Comment 5

[Bruce Davidson]

Attachment: Initial patch to read search engine list from profile if available

This patch causes Camino to first read ~/Application
Support/Library/Camino/SearchURLList.plist for the list of search engines. If
there is no such file or this fails then it falls back on the search engine
list in the application bundle as currently. This should make this
customisation easier and less threatening for most users. (And possible for
those users with wise sys admins that have locked down access to
/Applications!)

Comments on how the fall back should work are welcome - should it just be a
fall back as implemented or (like the bookmarks code) should we copy the file
out of our application bundle into the profile so that the user has something
to modify in future?

Comment 6

[Ludovic Hirlimann [:Usul]]

Comment on attachment 152782 [details] [diff] [review]
Initial patch to read search engine list from profile if available

[snip]
>+  if (searchURLDictionary == nil) {
>+    nsCOMPtr<nsIFile> aDir;
>+    NS_GetSpecialDirectory(NS_APP_USER_PROFILE_50_DIR, getter_AddRefs(aDir));
>+    if (aDir) {
>+      nsCAutoString aDirPath;
>+      nsresult rv = aDir->GetNativePath(aDirPath);
>+      if (NS_SUCCEEDED(rv)) {
>+        NSString *profileDir = [NSString stringWithUTF8String:aDirPath.get()];  
>+
>+        NSString *bookmarkPath = [profileDir stringByAppendingPathComponent:@"SearchURLList.plist"];
>+        searchURLDictionary = [[NSDictionary alloc] initWithContentsOfFile:bookmarkPath];
>+      }
>+    }
>+  }
>+  
>+  //
>+  // If that failed then read the default search engine list from our application bundle
>+  //
>+  if ( searchURLDictionary == nil )
 Why don't you use an if-then-else structure instead of testing twice for
searchURLDictionnary being nil ?

Comment 7

[Jasper]

I think it would be best yto always copy a file to the support folder just like
we do with bookmarks. That should be cleareer to our users. 

It might be interesting to see if you guys could also move the original search
plist file from the resource to the lproj folder for localisation purposes. Then
every language could have it's won localised search engines.

Comment 8

[ender.mb]

It is in the lproj folder, always has been AFAIK.

Comment 9

[Jasper]

Hehe I knew I should have looked before I said that. You are right it's there
already.

Comment 10

[Bruce Davidson]

Attachment: Updated patch that copies the default to the user profile directory

This is an updated patch that addresses Jasper's comment. If there is no
SearchURLList.plist in the user's profile it will copy the default one from the
application bundle into the profile. This means we can simply always tell the
user to edit the file in their profile, which is less scary than copying (or
worse, editing) a file inside an application bundle.

There are still two checks for the search list being null. I believe this is
the neatest way to cope with all possible error conditions when reading from
the profile, without lots of "else" clauses or introducing error flags. The
other alternative is to do a return immediately if we successfully read from
the profile directory, and then remove the second if. I'm happy to do this if
its considered better style.

Seeking review on this patch.

Comment 11

[Jasper]

it might be handy to incorporate some kind of erroir handeling. To instances I
can emagine:

1) What happens if both files are missing? It migh be handy if we have an error
handler passing this info to the user. What happens now?
2) What happens if the url.plist in the profile folder is corrupt? Right now the
one from the app is used but it would be better I think if the user is told the
file is corrupt en then use the one in the app.

Other then that the new behaviour is much better.

Comment 12

[Bruce Davidson]

> 1) What happens if both files are missing? It migh be handy if we have an error
> handler passing this info to the user. What happens now?

I'm not sure I can imagine both files being missing. That would require someone
to deliberately open the application bundle and delete the file. If people start
doing that we've got all sorts of problems!

> 2) What happens if the url.plist in the profile folder is corrupt? Right now the
> one from the app is used but it would be better I think if the user is told the
> file is corrupt en then use the one in the app.

How would we tell the user? The bookmarks code displays an error dialog only if
it can't read any of the .plist, .xml, .html bookmarks files and it can't use
the default set in the application bundle. Do we want to annoy the user with a
dialog each time they start the browser, or just write it to the console?

Comment 13

[Ludovic Hirlimann [:Usul]]

Comment on attachment 152844 [details] [diff] [review]
Updated patch that copies the default to the user profile directory


>+        NSString *searchEngineListPath    = [profileDir stringByAppendingPathComponent:@"SearchURLList.plist"];
>+        NSString *defaultSearchEngineList = [[NSBundle mainBundle] pathForResource:@"SearchURLList" ofType:@"plist"];
>+        NSFileManager *fileManager = [NSFileManager defaultManager];
>+        if ( [fileManager isReadableFileAtPath:searchEngineListPath] ||
>+             [fileManager copyPath:defaultSearchEngineList toPath:searchEngineListPath handler:nil] )
>+          searchURLDictionary = [[NSDictionary alloc] initWithContentsOfFile:searchEngineListPath];
>+      }
>+    }
>+  }
>+  
>+  //
>+  // If reading from the profile directory failed for any reason
>+  // then read the default search engine list from our application bundle directly
>+  //
>+  if ( searchURLDictionary == nil )
>     searchURLDictionary = [[NSDictionary alloc] initWithContentsOfFile:
>       [[NSBundle mainBundle] pathForResource:@"SearchURLList" ofType:@"plist"]];
>   
>   return searchURLDictionary;
> }

I'de still prefer something like this :
+      }
+      else
+	NSLog(@"Unable to open Profile dir !!");
+    }
+    else
+      NSLog(@"Can't get the profile dir");
+  }
+ else
+  //
+  // If reading from the profile directory failed for any reason
+  // then read the default search engine list from our application bundle
directly
+  //
     searchURLDictionary = [[NSDictionary alloc] initWithContentsOfFile:
       [[NSBundle mainBundle] pathForResource:@"SearchURLList"
ofType:@"plist"]];
+
+

Comment 14

[Bruce Davidson]

Attachment: Updated patch addressing review comments

Revised patch that addresses the review comments in comment 13 by adding NSLog
calls in the face of error conditions. Requesting review on this revised patch.

Comment 15

[Bruce Davidson]

Comment on attachment 152844 [details] [diff] [review]
Updated patch that copies the default to the user profile directory

Clear review? on obsolete patch.

Comment 16

[Ludovic Hirlimann [:Usul]]

Comment on attachment 163203 [details] [diff] [review]
Updated patch addressing review comments

r+

Comment 17

[Wevah (Nate Weaver)]

Comment on attachment 163203 [details] [diff] [review]
Updated patch addressing review comments

Looks good; the static NSDictionary near the top threw me off for a few
seconds, thoug. ;)

Comment 18

[Ludovic Hirlimann [:Usul]]

Comment on attachment 163203 [details] [diff] [review]
Updated patch addressing review comments

asking for sr

Comment 19

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 163203 [details] [diff] [review]
Updated patch addressing review comments

+	 NSString *defaultSearchEngineList = [[NSBundle mainBundle]
pathForResource:@"SearchURLList" ofType:@"plist"];

rather than do this twice, pull this declaration out of the if statement so
that it is scoped to the entire method.

just attach another quick patch and it'll get sr.

Comment 20

[Bruce Davidson]

Attachment: Updated patch addressing pinkerton's sr comments

Updated patch that factors out the common code from Pinkerton's sr comment.
Request transfer of r= and sr= from previous reviews, and if someone could
check this in for me.

Comment 21

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 164337 [details] [diff] [review]
Updated patch addressing pinkerton's sr comments

sr=pink, but i re-did the patch with some tweaks, attaching new patch.

Comment 22

[Mike Pinkerton (not reading bugmail)]

Attachment: updated patch that was landed

Comment 23

[Mike Pinkerton (not reading bugmail)]

fixed. the problem here is that as soon as people start to use this, if we ever
change the default list, they'll never get it.

Comment 24

[David Fedoruk]

(In reply to comment #23)
> fixed. the problem here is that as soon as people start to use this, if we ever
> change the default list, they'll never get it.

It works! This build finds my SearchURLList.plist. Thanks coders!

DF

Comment 25

[Ludovic Hirlimann [:Usul]]

based on last comment