189930 - Allow a tab group to be set as the home page (multiple homepages)

Status: RESOLVED INCOMPLETE

(Camino Graveyard :: Preferences, enhancement)

Description

[Rusty Mitchell]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.0.1) Gecko/20030119 Chimera/0.6+
Build Identifier: Mozilla/5.0 (000000000; 0; 000 000 00 0 000000; 00000; 00000000) Gecko/20030119 Chimera/0.6+

I would like to be able to set my home page as a tab group. Since this is not a
direct option in Preferences, I thought that getting info and assigning a
keyword to the tab group might work. After setting the keyword to [home], typing
[home] in the url space brings up the tab group. But when I set the home page
setting in preferences to [home], it sends me to [http://www.home.com]. If I
change the home page setting in preferences to [keyword: home] it uses netscapes
search to search the word [home]. The user should either be able to set a tab
group directly as the home page in preferences or at least be able to use
keywords. The keyword thing seems a bit confusing right now.

Reproducible: Always

Steps to Reproduce:
1. Get info and assign a keyword to a tab group
2. Set home page in preferences to [keyword]
3. Hitting home button takes viewer to http://www.[keyword].com instead of tab group
4. Reassign home page in preferences to [keyword: {keyword}]
5. Hitting home button takes viewere to netscape search of assigned keyword
instead of tab group

Actual Results:  
See details and steps to reproduce.

Expected Results:  
The user should either be able to set a tab group directly as the home page in
preferences or at least be able to use keywords to set a tab group as home page.

Comment 1

[S Woodside]

Confirming as request for enhancement.

Comment 2

[Jason Bassford]

Tweaking summary to correct reverse logic.

I'd be interested in this also, but under Mozilla.  Should the product be
changed to Browser and All/All?  (I think that filing a separate, identical,
Browser bug would be redundant wouldn't it?)

Comment 3

[Mike Pinkerton (not reading bugmail)]

cc'ing dave haas in case this can be somehow worked into his new bookmarks rewrite.

Comment 4

[David Haas (gone, not reading mail)]

I tried this in Mozilla.  Here's what it adds the following lines to prefs.js:

user_pref("browser.startup.homepage", "http://www.mozilla.org/");
user_pref("browser.startup.homepage.1", "http://www.yahoo.com/");
user_pref("browser.startup.homepage.2", "http://www.google.com/");
user_pref("browser.startup.homepage.count", 3);

So to do this, you're just going have to poke around a little in 
PreferenceManager.mm &  in the preference pane "Navigation.mm".
Which I probably won't do for the rest of the bookmark stuff.

Comment 5

[Ulrik Sverdrup]

Does this bug have any relation to bug 152147, or should it have?
I'm not sure which is the better implementation and interface of this -- a home tab group or a home session. (The result should be the same, right?)

Comment 6

[Jason Bassford]

> Does this bug have any relation to bug 152147

Almost none - at least in concept. :)  One is entirely static, the other is entirely dynamic.

Comment 7

[Samuel Sidler (old account; do not CC)]

Couldn't this be done the same way Firefox does it? They specifically change the homepage location box (in prefs) to allow | in between them and open each URL in a new tab (but only when they see the |). So I can do the following and have it open three tabs:

http://www.google.com/|http://www.caminobrowser.org/|http://www.yahoo.com/

Comment 8

[Chris Lawson (gone)]

(In reply to comment #7)
> Couldn't this be done the same way Firefox does it? They specifically change
> the homepage location box (in prefs) to allow | in between them and open each
> URL in a new tab (but only when they see the |). So I can do the following and
> have it open three tabs:
> 
> http://www.google.com/|http://www.caminobrowser.org/|http://www.yahoo.com/

My gut says this is a much uglier and much more difficult means than what David Haas mentioned in comment 4. His proposed solution sounds fairly simple to implement.

cl

Comment 9

[Chris Lawson (gone)]

Adding dependency on bug 287790. We need to work out how that's going to work before we tackle this.

I can take this once 287790 gets fixed and lands.

cl

Comment 10

[Chris Lawson (gone)]

How do Seamonkey and Firefox handle this if you have tabs turned off and use multiple windows for your browsing?

cl

Comment 11

[Chris Lawson (gone)]

My proposed UI for this so far is basically what Seamonkey does: put a button in the pref pane allowing the user to select the current tabs as the home page set, and when that's set, put some sort of notification text in the home page field ("Multiple values" or something like that) to alert the user, like the "hint text" we put in the search field. The text would disappear when the field begins editing.

This avoids entirely the very sticky UI issue of how you let a user manually input multiple home page URLs. If people really need to input multiple URLs manually, they can do it with a text editor in their prefs.js file.

If anyone has any questions about how this will look, there's a proposed nib file in bug 287790.

cl

Comment 12

[Chris Lawson (gone)]

(In reply to comment #10)
> How do Seamonkey and Firefox handle this if you have tabs turned off and use
> multiple windows for your browsing?

From discussions on IRC, it appears that neither one has any means of loading multiple *windows* when the Core prefs for multiple homepages are set. I think worrying about that will unnecessarily complicate things, and I think we shouldn't worry about it either.

I would, however, defer to Simon and Mike's opinion here.

cl

Comment 13

[Chris Lawson (gone)]

Attachment: Adds support for multiple homepages

This definitely isn't ready for checkin, but requesting review anyway to get comments on the approach and behaviour. (Patch should apply, but may have some offsets due to a bunch of other crud in my tree. When this gets closer to checkin quality, I'll respin off a clean tree.

cl

Comment 14

[Chris Lawson (gone)]

Attachment: nib file for General (Navigation) pref pane

Here's the nib.

cl

Comment 15

[Chris Lawson (gone)]

Comment on attachment 223874 [details] [diff] [review]
Adds support for multiple homepages

Really setting r? flag this time.

Comment 16

[Chris Lawson (gone)]

Comment on attachment 223875 [details]
nib file for General (Navigation) pref pane

Fixing MIME type on attachment.

Comment 17

[Chris Lawson (gone)]

While I'm adding a cc and bugspamming everyone anyway, I'll take this opportunity to say I already found room for improvement in the patch: I consolidated the two IBActions on Navigation.mm to one, |setHomePage:|, and now check the sender to alter the behaviour there. It cuts down on redundancy a bit (there was quite a bit of code shared between the two methods).

cl

Comment 18

[Chris Lawson (gone)]

One more comment -- I have fixed the spacing of the various UI elements in the nib file to conform more closely with the AHIG. The nib currently attached here doesn't reflect that, but right now I'm more concerned with commentary on the functionality. Once the first round of reviews is done-ish, I'll upload new patches for both the code and the UI.

cl

Comment 19

[Nick Kreeger]

Comment on attachment 223874 [details] [diff] [review]
Adds support for multiple homepages

+// XXXcl-fixed fix this to work with tab groups as home page
+- (NSArray *) homePageUsingStartPage:(BOOL)checkStartupPagePref
 {
+  NSString* blankPage = @"about:blank";
+  NSArray* blankURL = [[NSArray alloc] initWithObjects:blankPage,nil];
+  //NSLog(@"blankURL is %@", blankURL);
   if (!mPrefs)
-    return @"about:blank";
+    return blankURL;

First, remove the comment and the NSLog. 
Next, I really don't like the name here. homePageUsingStartPage doesn't tell me that it is going to be an array.
Maybe a name like |homePageArrayWithStartPage:| or something.

Another thing, wrapping up @"about:blank" in a NSArray seems kinda heavy just to get a single string. 
Perhaps if the setting is not set to load an array for a homepage, call the old method to give out the single NSString value, and create a new method for getting the array of this list. This new method could get the single string value from the old method.

+    } else {
       homepagePref = [self getStringPref:"browser.startup.homepage" withSuccess:NULL];
+      [urlArray addObject:homepagePref];
+

+      for (int i = 1; i < count; ++i) {
+        NSString* prefName = [NSString stringWithFormat:@"browser.startup.homepage.%d", i];
+        //NSLog(@"getting pref for %@", prefName);
+        NSString* nextURL = [self getStringPref:[prefName UTF8String] withSuccess:NULL];
+        [urlArray addObject:nextURL];
+      }
     }

Remove the NSLog call here. Instead of calling these items random names like "prefName" and "nextURL", try something like "curPrefValue" and "curPrefURL" or something.

Please put your else statements on the line under the ending curly, not next to it.

+  // XXXcl-fixed fix this for multiple home pages (use array instead?)

Please remove this comment

+  NSArray* urls = mStartURL ? [[NSArray alloc] initWithObjects:mStartURL,nil] : [[PreferenceManager sharedInstance] homePageUsingStartPage:YES];

To make this block of code a bit more readable, put parens around the conditional.

+  BrowserWindowController* controller = [self openBrowserWindowWithURLs:urls
+                                                                 behind:nil
+                                                            allowPopups:NO];

Since this block of code is about half the size of the call above it, go ahead and keep it all on the same line

+  if ([MainController isBlankURL:[urls objectAtIndex:0]])

This can be a dangerous call if the array inits to nothing, perhaps init the NSArray to nil, then check for it in the conditional

   else
   {
     // follow the pref about what to load in a new tab (even though we're making a new window)
     int newTabPage = [[PreferenceManager sharedInstance] getIntPref:"browser.tabs.startPage" withSuccess:NULL];
     BOOL loadHomepage = (newTabPage == 1);
 
     NSString* urlToLoad = @"about:blank";
-    if (loadHomepage)
-      urlToLoad = [[PreferenceManager sharedInstance] homePageUsingStartPage:NO];
+    if (loadHomepage) {
+      // XXXcl what should we do here if we have a tab group as the home page?
+      NSArray* homepages = [[PreferenceManager sharedInstance] homePageUsingStartPage:NO];
+      if ([homepages count] == 1)
+        urlToLoad = [homepages objectAtIndex:0];
+    }
 
     [self openBrowserWindowWithURL:urlToLoad andReferrer:nil behind:nil allowPopups:NO];
   }


Since the else block puts the curly brace on the next line, go ahead and do the same for your if statment that has a curly.
Also, once again remove the comment.

Another thing to watch out here is that the [homepages count] call could be dangerous, you should init that array to nil, then populate it.

+  NSArray* homepages = [[PreferenceManager sharedInstance] homePageUsingStartPage:NO];
+  if ([homepages count] == 1)
+    [mBrowserView loadURI:[homepages objectAtIndex:0] referrer:nil flags:NSLoadFlagsNone activate:NO allowPopups:NO];
+  else {
+    // XXXcl fix this for tab group as home page
+    // Should we replace? Append? Disable?
+  }

Once again, a little nil action won't hurt, and comment removal.

     {
       // Focus the URL bar if we're opening a blank tab and the URL bar is visible.
       NSToolbar* toolbar = [[self window] toolbar];
       BOOL locationBarVisible = [toolbar isVisible] &&
                                 ([toolbar displayMode] == NSToolbarDisplayModeIconAndLabel ||
                                  [toolbar displayMode] == NSToolbarDisplayModeIconOnly);
                                 
       NSString* urlToLoad = @"about:blank";
-      if (loadHomepage)
-        urlToLoad = [[PreferenceManager sharedInstance] homePageUsingStartPage:NO];
+      if (loadHomepage) {
+        // XXXcl what should we do here if we have a tab group as the home page?
+        NSArray* homepages = [[PreferenceManager sharedInstance] homePageUsingStartPage:NO];
+        if ([homepages count] == 1)
+          urlToLoad = [homepages objectAtIndex:0];
+      }

Let's put our curly brace on the next line like the brace at the top. Also comments...


+#import "MainController.h"
+#import "BrowserWindowController.h"

Our pref panes shouldn't need to know about these classes. If you want them to update on an event, post events using hte NSNotificationCenter.

+  if ([self getIntPref:"browser.startup.homepage.count" withSuccess:&gotPref] <= 1) {
+    //NSLog(@"browser.startup.homepage.count <= 1; setting stringValue to %@", [[self getCurrentHomePage] objectAtIndex:0]);
+    [textFieldHomePage setStringValue:[[self getCurrentHomePage] objectAtIndex:0]];
+  } else {
+    //NSLog(@"browser.startup.homepage.count > 1; setting stringValue to 'Tab group set'.");
+    [textFieldHomePage setStringValue:NSLocalizedString(@"Tab group set", @"Tab group set")];
+    // disable "Load Homepage in New Tabs" checkbox to avoid confusion
+    [checkboxNewTabBlank setEnabled:NO];
+  }
+

Once again, make sure the else statment is on a new line under the previous if statment, and remove the NSLog calls/comments.
Make your NSLocalizedString call look like this: NSLocalizedString(@"Tab group set", nil) -> we really don't need the extra stuff in there.

+  // only set the pref based on the text field if a tab group is NOT the home page
+  // XXXcl will this work with l10n?
+  if (![[textFieldHomePage stringValue] isEqualToString:NSLocalizedString(@"Tab group set", @"Tab group set")]) {
+    [self setPref:"browser.startup.homepage" toString:[textFieldHomePage stringValue]];
+    [self setPref:"browser.startup.homepage.count" toInt:1];
+  }

See above on the NSLocalizedString call. This XXX comment seems fine, but you may want to find the answer before and landing of this feature.

+
+  [[NSNotificationCenter defaultCenter] removeObserver:self];
 }
 
Why are you unsubscribing here (because the item is no longer selected?)

-- (NSString*)getCurrentHomePage
+- (NSArray*)getCurrentHomePage
 {

The name of this method should be changed as well now that it represents an array instead of a single string. (getCurrentHomePageArray)

+  int count = [self getIntPref:"browser.startup.homepage.count" withSuccess:&gotPref];
+  //NSLog(@"getting pref for browser.startup.homepge.count: %d", count);
+  NSMutableArray* urlArray = [NSMutableArray arrayWithCapacity:count];

+  NSString* homepageURL = [self getStringPref:"browser.startup.homepage" withSuccess:&gotPref];
+  //NSLog(@"getting pref for browser.startup.homepage = %@", homepageURL);
+  [urlArray addObject:homepageURL];

+  for (int i = 1; i < count; ++i) {
+    NSString* prefName = [NSString stringWithFormat:@"browser.startup.homepage.%d", i];
+    NSString* nextURL = [self getStringPref:[prefName UTF8String] withSuccess:&gotPref];
+    //NSLog(@"getting pref for %@ = %@", prefName, nextURL);
+    [urlArray addObject:nextURL];
+  }
+

Remove the NSLog's here. Also I noticed this in your simular method above, its more of a nit, but I would set i at 0 and i++ in the for loop.
Also look at my comments above about the name of these variables

+  BrowserWindowController* bwc = [[(MainController*)[NSApp delegate] getFrontmostBrowserWindow] windowController];
+  if (!bwc) {
+    // for some reason we couldn't get a BWC, so disable button
+    [buttonSetCurrentPageAsHomePage setEnabled:NO];
+    return;
+  }
+  NSString* currentURL = [[bwc activeBrowserView] getCurrentURI];

+- (IBAction)setCurrentTabsAsHomePage:(id)sender
+{
+  BrowserWindowController* bwc = [[(MainController*)[NSApp delegate] getFrontmostBrowserWindow] windowController];
+  if (!bwc) {
+    // for some reason we couldn't get a BWC, so disable button
+    [buttonSetCurrentTabsAsHomePage setEnabled:NO];
+    return;
+  }

This seems like an OK route (with getting the current BWC), but I don't like it. Maybe we can find a cleaner approach to getting the current URI...

+  int numTabs = [[bwc getTabBrowser] numberOfTabViewItems];  
+  NSMutableArray* urlArray = [NSMutableArray arrayWithCapacity:numTabs];
+  for (int i = 0; i < numTabs; i++)
+  {
+    BrowserWrapper* browserWrapper = (BrowserWrapper*)[[[bwc getTabBrowser] tabViewItemAtIndex:i] view];
+    NSString* hrefString = [browserWrapper getCurrentURI];
+    [urlArray addObject:hrefString];
+  }

What happens if there are no tabs here?

+  // now we have the strings, so let's write them to the prefs file
+  [self setPref:"browser.startup.homepage" toString:[urlArray objectAtIndex:0]];
+  [self setPref:"browser.startup.homepage.count" toInt:[urlArray count]];
+  for (int i = 1; i < (int)[urlArray count]; ++i) {
+    NSString* prefName = [NSString stringWithFormat:@"browser.startup.homepage.%d", i];
+    [self setPref:[prefName UTF8String] toString:[urlArray objectAtIndex:i]];
+    //NSLog(@"setting pref for %@ = %@", prefName, [urlArray objectAtIndex:i]);
+  }

Again the NSLog comment/call.

+  for (int i = 1; i < (int)[urlArray count]; ++i) {

This is an ugly way of looping through the for loop. You should use a NSEnumerator here.

+  [textFieldHomePage setStringValue:NSLocalizedString(@"Tab group set", @"Tab group set")];

Another NSLocalizedString to fix up.

+- (void)validateHomePageButtons
+{
+  BrowserWindowController* bwc = [[(MainController*)[NSApp delegate] getFrontmostBrowserWindow] windowController];
+  [buttonSetCurrentPageAsHomePage setEnabled:(bwc != nil)];
+  
+  int numTabs = [[bwc getTabBrowser] numberOfTabViewItems];
+
+  // Disable "Use Current Tabs" unless we have multiple tabs (which implies a valid bwc)
+  [buttonSetCurrentTabsAsHomePage setEnabled:(numTabs > 1)];
+  
+  // Disable "Load Home Page in New Tabs" checkbox if multiple homepages set
+  if ([[self getCurrentHomePage] count] > 1)
+    [checkboxNewTabBlank setEnabled:NO];
+}
+

This method needs some work...

[buttonSetCurrentPageAsHomePage setEnabled:(bwc != nil)];

First, this is fancy, but what about the rest of the method? Your next call is to get the number of tabs, what if |bwc| is nil?

+  if ([[self getCurrentHomePage] count] > 1)

This is another good reason to change the name of |getCurrentHomePage|, why would the current home page have a count?

This patch needs some work, and maybe some decisions made, such as should this feature should be a seperate item from the default home page setting?

Also, do we try to keep the BrowserCode like BWC away from the prefs, so maybe we can find another way for the users to set the tab group (or just bite the bullet here)?

There seems to be alot of grey to this feature that needs resolvement before a strong fix can be made.

Comment 20

[Chris Lawson (gone)]

(In reply to comment #19)
> (From update of attachment 223874 [details] [diff] [review] [edit])

> +  NSArray* urls = mStartURL ? [[NSArray alloc] initWithObjects:mStartURL,nil]
> : [[PreferenceManager sharedInstance] homePageUsingStartPage:YES];

...

> +  if ([MainController isBlankURL:[urls objectAtIndex:0]])
> 
> This can be a dangerous call if the array inits to nothing, perhaps init the
> NSArray to nil, then check for it in the conditional

How could the above init ever be nil?

> +      NSArray* homepages = [[PreferenceManager sharedInstance]
> homePageUsingStartPage:NO];
> +      if ([homepages count] == 1)
> +        urlToLoad = [homepages objectAtIndex:0];
> +    }
> 
>      [self openBrowserWindowWithURL:urlToLoad andReferrer:nil behind:nil
> allowPopups:NO];
>    }

...

> Another thing to watch out here is that the [homepages count] call could be
> dangerous, you should init that array to nil, then populate it.

Again, how can that array be nil?

> +#import "MainController.h"
> +#import "BrowserWindowController.h"
> 
> Our pref panes shouldn't need to know about these classes. If you want them to
> update on an event, post events using hte NSNotificationCenter.

It's not for events, but see also pink's bug 287790 comment 10. I agree it's a little "meh" but if anyone has a better proposal I'm all ears.

> Make your NSLocalizedString call look like this: NSLocalizedString(@"Tab group
> set", nil) -> we really don't need the extra stuff in there.

Our standard style seems to be to keep the comment in place.

> +  [[NSNotificationCenter defaultCenter] removeObserver:self];
>  }
> 
> Why are you unsubscribing here (because the item is no longer selected?)

Yes. If it's not selected, we don't care about validating the buttons, and therefore we don't care about the notifications we registered for in |didSelect:|

> +  for (int i = 1; i < count; ++i) {
> +    NSString* prefName = [NSString
> stringWithFormat:@"browser.startup.homepage.%d", i];
> +    NSString* nextURL = [self getStringPref:[prefName UTF8String]
> withSuccess:&gotPref];
> +    //NSLog(@"getting pref for %@ = %@", prefName, nextURL);
> +    [urlArray addObject:nextURL];
> +  }
> +
> 
> Remove the NSLog's here. Also I noticed this in your simular method above, its
> more of a nit, but I would set i at 0 and i++ in the for loop.

The reason it's done that way is because the pref requires it. (I pretty much stole this straight from Seamonkey's code.) The way the pref works is it leaves the standard "browser.startup.homepage" pref in place and adds "browser.startup.homepage.1", "browser.startup.homepage.2", ... "browser.startup.homepage.n", along with "browser.startup.homepage.count" (where count = n + 1). Starting the for{} at 0 wouldn't make much sense in this case, since we use the iterator value as part of the pref.

> +  int numTabs = [[bwc getTabBrowser] numberOfTabViewItems];  
> +  NSMutableArray* urlArray = [NSMutableArray arrayWithCapacity:numTabs];
> +  for (int i = 0; i < numTabs; i++)
> +  {
> +    BrowserWrapper* browserWrapper = (BrowserWrapper*)[[[bwc getTabBrowser]
> tabViewItemAtIndex:i] view];
> +    NSString* hrefString = [browserWrapper getCurrentURI];
> +    [urlArray addObject:hrefString];
> +  }
> 
> What happens if there are no tabs here?

In theory, we skip the entire for{} loop, which we shouldn't get into in the first place, because that button was disabled, so that method can't get called. (See |validateHomePageButtons:| in Navigation.mm.) In my revised code, the two methods are combined, but executing the above code still requires that > 1 tabs are open in the frontmost browser window. So unless someone is deliberately altering the sender of the message at runtime, this seems fairly robust to me. (And if someone is doing that, we have bigger problems.)

> +  for (int i = 1; i < (int)[urlArray count]; ++i) {
> 
> This is an ugly way of looping through the for loop. You should use a
> NSEnumerator here.

Again, we need the iterator to set the pref properly. Otherwise I would have tossed all the for{} constructs out of here entirely.

> +- (void)validateHomePageButtons
> +{
> +  BrowserWindowController* bwc = [[(MainController*)[NSApp delegate]
> getFrontmostBrowserWindow] windowController];
> +  [buttonSetCurrentPageAsHomePage setEnabled:(bwc != nil)];
> +  
> +  int numTabs = [[bwc getTabBrowser] numberOfTabViewItems];
> +
> +  // Disable "Use Current Tabs" unless we have multiple tabs (which implies a
> valid bwc)
> +  [buttonSetCurrentTabsAsHomePage setEnabled:(numTabs > 1)];
> +  
> +  // Disable "Load Home Page in New Tabs" checkbox if multiple homepages set
> +  if ([[self getCurrentHomePage] count] > 1)
> +    [checkboxNewTabBlank setEnabled:NO];
> +}
> +
> 
> This method needs some work...
> 
> [buttonSetCurrentPageAsHomePage setEnabled:(bwc != nil)];
> 
> First, this is fancy, but what about the rest of the method? Your next call is
> to get the number of tabs, what if |bwc| is nil?

It should return nil as well, which still passes the comparison (nil is not greater than 1, so the "Use Current Tabs" button still gets disabled). Nothing bad happens here if there's no browser window open.

> +  if ([[self getCurrentHomePage] count] > 1)
> 
> This is another good reason to change the name of |getCurrentHomePage|, why
> would the current home page have a count?

Agreed.

> This patch needs some work, and maybe some decisions made, such as should this
> feature should be a seperate item from the default home page setting?

You mean should it not use the pref that Core already defined for this purpose? Maybe it's just me, but when Firefox decided to go that route, a giant mess ensued. (Separating URLs with a legal URL character? Whose brilliant idea was THAT?)

> Also, do we try to keep the BrowserCode like BWC away from the prefs, so maybe
> we can find another way for the users to set the tab group (or just bite the

As I said above, suggestions are welcomed. I don't see any particularly good way for prefs code to know about browser state *other* than simply telling the prefs about it. Pink didn't seem to think it was too troublesome, but maybe he meant to put up more of a stink in bug 287790 ;)

cl

Comment 21

[Stuart Morgan]

> +  if ([MainController isBlankURL:[urls objectAtIndex:0]])
> 
> This can be a dangerous call if the array inits to nothing, perhaps init the
> NSArray to nil, then check for it in the conditional

How is this dangerous?  The init returns either a valid NSArray, or nil (I'm not sure what you mean by it initing to "nothing"), and [nil objectAtIndex:0] will return nil, which is fine to pass to isBlankURL:

> +      NSArray* homepages = [[PreferenceManager sharedInstance]
> homePageUsingStartPage:NO];
> +      if ([homepages count] == 1)
> [snip]
> 
> Another thing to watch out here is that the [homepages count] call could be
> dangerous, you should init that array to nil, then populate it.

Again, it will be set to either an NSArray or nil; presetting to nil doesn't accomplish anything. And since [nil count] === 0, there's nothing dangerous.

Comment 22

[Stuart Morgan]

By the way...

+  for (int i = 1; i < (int)[urlArray count]; ++i) {

The correct way to fix your warning is to declare |i| unsigned, not to cast |count| to a signed int.

Comment 23

[Chris Lawson (gone)]

(In reply to comment #22)
> By the way...
> 
> +  for (int i = 1; i < (int)[urlArray count]; ++i) {
> 
> The correct way to fix your warning is to declare |i| unsigned, not to cast
> |count| to a signed int.
> 

Yeah, I realised that at some point when looking over my code. ;)

cl

Comment 24

[Simon Fraser [no longer active]]

Screenshot?

Comment 25

[Chris Lawson (gone)]

Attachment: screenshot of General prefs

As requested.

Comment 26

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

For the text that fills the "Home page" field when multiple homepages are set, we should probably use something like "Multiple homepages set" or "Multiple tabs" rather than "Tab group set", as the latter might confuse people into thinking they can set any tab group directly from their bookmarks.  Since that's not possible, we shouldn't use the same language.

Comment 27

[Chris Lawson (gone)]

(In reply to comment #26)
> For the text that fills the "Home page" field when multiple homepages are set,
> we should probably use something like "Multiple homepages set" or "Multiple
> tabs" rather than "Tab group set", as the latter might confuse people into
> thinking they can set any tab group directly from their bookmarks.  Since
> that's not possible, we shouldn't use the same language.

* not possible _yet_ :)

But point taken.

cl

Comment 28

[Simon Fraser [no longer active]]

I think it will be confusing to see something like "Multiple tabs" in the homepage field. You're hiding what the user wants to see (what page(s) will open for their homepage) and not giving them any way to change them, other than to reset the entire set. I think to do this properly you have to tie into bookmarks.

Comment 29

[Warren TenBrook]

How about a scrolling list?  If the homepage is a single url, only one item appears.  If it is a tab group, all the urls are listed.

Comment 30

[Chris Lawson (gone)]

(In reply to comment #29)
> How about a scrolling list?  If the homepage is a single url, only one item
> appears.  If it is a tab group, all the urls are listed.

I would prefer to avoid the issue of manually inputting multiple URLs entirely, because there's no good UI for it. (Anyone have a proposal that doesn't suck for this?)

A "make this my home page" action on a bookmark folder or tab group would be nice, though. Then we could put the folder/tab group icon and the bookmark name in the text field.

cl

Comment 31

[Warren TenBrook]

Sorry if my recommendation was misinterpreted, CL.  I think your button text is fine.  If the user has a single page open and selects the "use current page" button, the scroll list should be autofilled with the url of their current page.  If the user has a tab group open, they can select the "use current tabs" button and the scroll list would be autofilled with the tab group urls.  I think this combines one-click simplicity with smfr's desire to show the user what they need to see.

Comment 32

[Chris Lawson (gone)]

(In reply to comment #31)
> Sorry if my recommendation was misinterpreted, CL.  I think your button text is
> fine.  If the user has a single page open and selects the "use current page"
> button, the scroll list should be autofilled with the url of their current
> page.  If the user has a tab group open, they can select the "use current tabs"
> button and the scroll list would be autofilled with the tab group urls.

Ah, OK, I see what you're saying. Are you talking about a scrolling box sort of like what's used for the CC field on this bug, or are you referring to something else?

cl

Comment 33

[Warren TenBrook]

Yes, the bugzilla cc list control is about what I had in mind; however, smfr mentioned having a way to change the urls and hook into bookmarks.  When the 'Use Current Tabs' button is selected, a Bookmarks view could also be opened in a tab and a 'Home Tab Group' bookmark collection created for easy editing.

Comment 34

[Simon Fraser [no longer active]]

A scrolling list in this prefs panel is way too heavy. I think if we have a list, it needs to go into a "Details" sheet. I'm not really in favor of a scrolling list either, because you force the user to deal with a list of hard-to-type urls. If we have such a list, it should act as a drag target for urls, bookmarks, and bookmark folders/tab groups.

Comment 35

[Nick Kreeger]

Comment on attachment 223875 [details]
nib file for General (Navigation) pref pane

Removing review flag until we agree on the way to do this.

Comment 36

[Stuart Morgan]

*** Bug 356715 has been marked as a duplicate of this bug. ***

Comment 37

[Chris Lawson (gone)]

We'll need to make sure the issues raised by bug 343999 aren't a problem for us when this is fixed.

Comment 38

[Chris Lawson (gone)]

Attachment: screenshot of "Details…" sheet

Here's a screenie of proposed UI for this.

Comment 39

[Chris Lawson (gone)]

Attachment: Screenshot of General prefs pane

And the prefs pane itself.

Comment 40

[Stuart Morgan]

That's an awful lot of buttons--and therefore cognitive load when opening the prefs--for one feature.

Comment 41

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

Comment on attachment 264552 [details]
Screenshot of General prefs pane

cl asked for a UI review ;) so recording the salient points of the irc discussion here.

The buttons add too much noise, and since (in this new world) the sheet is where setting homepages is done, we can perhaps store the "Use foo" buttons there.

The challenge will be making the button that opens the sheet 1) not look odd all by itself and 2) not take up lots of space or eat into the URL text field.

Comment 42

[Håkan Waara]

Another idea, that would avoid the sheet and extra buttons, would be to simply use the address textfield as a drop target for tab folders. 

When it's not active, you can show a gray text in it (like the google search does) saying "Drop bookmarks here to make it your start page".

Comment 43

[Stuart Morgan]

In that model, if you drop 10 bookmarks on it, how would you see what they were later, or remove one of them?

Comment 44

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

Mass un-setting milestone per 1.6 roadmap.

Filter on RemoveRedonkulousBuglist to remove bugspam.

Developers: if you have a patch in hand for one of these bugs, you may pull the bug back to 1.6 *at that point*.

Comment 45

[cannedbrain+mozilla]

How about using a small button with drop-down menu (like the ones you find in the bookmark manager)? When the user click on it, they'll see these menu commands:

Add current page
Add current tabs
----------------
Manage... (opens up the manager Chris posted in https://bugzilla.mozilla.org/show_bug.cgi?id=189930#c38)

Comment 46

[cannedbrain+mozilla]

Of course, add should not replace the current homepage. "Use" should replace but I thought it'd be too much to have 5 commands to set a homepage.

Anyways, I was also thinking about using the bookmark manager to set home page. Sort of like having a special collection for homepage. But it'd be confusing since people are used to opening the prefs to set it.

Comment 47

[Chris Lawson (gone)]

Attachment: possible button image

(In reply to comment #41)

> The challenge will be making the button that opens the sheet 1) not look odd
> all by itself and 2) not take up lots of space or eat into the URL text field.

Can we get one of our resident artiste-types to have a go at this? Hicks, maybe? Something about 16x16 ought to be sufficient. I was thinking maybe a the letter "i" inside a circle with the same general color scheme as our close-tab widget.

Something like this.

Comment 48

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

(In reply to comment #46)
> Anyways, I was also thinking about using the bookmark manager to set home page.
> Sort of like having a special collection for homepage. But it'd be confusing
> since people are used to opening the prefs to set it.

I really like the kernel of this idea: if we have a "Home Page" collection, it would be easy for people to add pages to their "home page" in the Manager itself and by using the "Add Bookmark" sheet.

I think we have to keep something like the UI we're working on here for the prefs, but this seems like a unique and useful alternate way--but we do need to watch how many alternate ways we have to do things (and would be another bug, anyway).

Comment 49

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

> > The challenge will be making the button that opens the sheet 1) not look odd
> > all by itself and 2) not take up lots of space or eat into the URL text field.
> 
> Can we get one of our resident artiste-types to have a go at this? Hicks,
> maybe? Something about 16x16 ought to be sufficient. I was thinking maybe a the
> letter "i" inside a circle with the same general color scheme as our close-tab
> widget.

This doesn't scream "click me to modify your home page" to me; it seems more like "if you can't figure out how to set a home page, in last gasp of desperation, click every button and then this 'i' thing that doesn't really look like a button" ;)

I'm trying to think of some way to use the "Home" icon and something to indicate "modify" (since we'll have add, set, delete, etc., there) that won't look like a useless blob at 16x16 (or 26x21, which is the size of the action button).

Comment 50

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

Adjusting priority and milestone per meet-up notes.

Comment 51

[Chris Lawson (gone)]

A few more button/icon ideas, just sort of brainstorming here: a house under construction, multiple houses staggered diagonally on top of each other, a house with a document on it, a document with a pencil writing on it, a document with a hand writing on it (sort of like the old generic System 7 application icon), the "Customize" icon overlaid on our Home icon, the Home icon with a small + overlaid on it somewhere, the Finder's "Documents" icon with a pencil or a hand writing on it.

Comment 52

[Samuel Sidler (old account; do not CC)]

Since we still don't have a full consensus here, this won't make Camino 1.6.

Comment 53

[Chris Lawson (gone)]

Philippe, can you maybe come up with an icon for us here? See comment 41 et seq. for details and possible ideas if you're interested.

Comment 54

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

Attachment: Safari 4's "Choose tabs folder…" sheet

Safari 4 uses this chooser sheet when you set "New windows open with:" to "Choose tabs folder…"