294019 - File opened with File->Open always loads in the first tab

Status: RESOLVED FIXED

(Camino Graveyard :: Tabbed Browsing, defect)

Description

[Marcel Broeken]

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

When I've multiple tabs open and I select File->Open Camino opens the file in
the first tab. It doesn't matter which tab I had open when selecting File->Open.

Reproducible: Always

Steps to Reproduce:
1. Open multiple tabs
2. Choose any tab (except the first), e.g. the second tab
3. Choose File->Open and choose a file

Actual Results:  
The selected file was loaded in the first tab.

Expected Results:  
The selected file should load in the active tab, in the example this is the
second tab.

Comment 1

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

FWIW, this is a regression from 0.8.x, where File->Open opens in the active tab.

I think it's a fairly recent regression in the nightlies (I think it's been
about two weeks since I last did File->Open to check something...); I'll do some
checking when I get a chance.

Comment 2

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

OK, this regressed between 3-25 and 3-26.  I think that means the checkins from
bug 287339 regressed this.

Since this is a regression, can we get this for 0.9?

Comment 3

[Simon Fraser [no longer active]]

Probably regression part of my bookmarks/tabs changes (not specifically bug 287339).

Comment 4

[Nick Kreeger]

Attachment: Proposed patch fixes problem

I modified the method:
- (void)openURLArray:(NSArray*)urlArray
replaceExistingTabs:(BOOL)replaceExisting allowPopups:(BOOL)inAllowPopups

Just replace with the code in the attachment and build.

After opening a few tabs, selecting one in the middle, and opening files will
replace the tab you have open, and open new tabs for the rest of the files.

Comment 5

[Nick Kreeger]

Comment on attachment 188049 [details] [diff] [review]
Proposed patch fixes problem

// Sorry use this, first time posting a patch
Index: browser/BrowserWindowController.mm
===================================================================
RCS file: /cvsroot/mozilla/camino/src/browser/BrowserWindowController.mm,v
retrieving revision 1.190
diff -u -8 -r1.190 BrowserWindowController.mm
--- browser/BrowserWindowController.mm	29 Jun 2005 01:25:34 -0000	1.190
+++ browser/BrowserWindowController.mm	2 Jul 2005 18:30:41 -0000
@@ -2643,40 +2643,52 @@
   BrowserTabViewItem* newTab = [self openNewTab:aLoadInBG];
   [[[newTab view] getBrowserView] setPageDescriptor:aDesc
displayType:aDisplayType];
 }

 - (void)openURLArray:(NSArray*)urlArray
replaceExistingTabs:(BOOL)replaceExisting allowPopups:(BOOL)inAllowPopups
 {
   int curNumTabs	= [mTabBrowser numberOfTabViewItems];
   int numItems 		= (int)[urlArray count];
+  int selectedTabIndex  = 0;  // keeps where the selected tab starts
+  int i;

-  for (int i = 0; i < numItems; i++)
+  // find the current tab
+  BrowserTabViewItem* selectedTabViewItem = [mTabBrowser selectedTabViewItem];
+  for (i = 0; i < curNumTabs; i++)
+  {
+    BrowserTabViewItem* aTab = [mTabBrowser tabViewItemAtIndex:i];
+    
+    if (selectedTabViewItem == aTab)
+    {
+      selectedTabIndex = i;
+    }
+  }
+  
+  for (i = 0; i < numItems; i++)
   {
     NSString* thisURL = [urlArray objectAtIndex:i];
     BrowserTabViewItem* tabViewItem;

     if (replaceExisting && i < curNumTabs)
-    {
-      tabViewItem = [mTabBrowser tabViewItemAtIndex:i];
+    { 
+      tabViewItem = [mTabBrowser tabViewItemAtIndex:selectedTabIndex];
+      replaceExisting = FALSE;
     }
     else
     {
       tabViewItem = [self createNewTabItem];
       [tabViewItem setLabel: NSLocalizedString(@"UntitledPageTitle", @"")];
       [mTabBrowser addTabViewItem: tabViewItem];
     }
-
+    
     [[tabViewItem view] loadURI: thisURL referrer:nil
			 flags: NSLoadFlagsNone activate:(i == 0)
allowPopups:inAllowPopups];

   }
- 
-  // Select the first tab.
-  [mTabBrowser selectTabViewItemAtIndex:replaceExisting ? 0 : curNumTabs];
 }

 -(void) openURLArrayReplacingTabs:(NSArray*)urlArray
closeExtraTabs:(BOOL)closeExtra allowPopups:(BOOL)inAllowPopups
 {
   // if there are no urls to load (say, an empty tab group), just bail
outright. otherwise we'd be
   // left with no tabs at all and hell to pay when it came time to do
menu/toolbar item validation.
   if (![urlArray count])
     return;

Comment 6

[Nick Kreeger]

Attachment: This contains the diff, please use this one instead of the previous patch!

This attachment contains the diff from cvs. Sorry for the last two posts and
the incorrect patch file.

Comment 7

[Simon Fraser [no longer active]]

Comment on attachment 188051 [details] [diff] [review]
This contains the diff, please use this one instead of the previous patch!

How does this affect other callers of -openURLArray:... ?
Also, what happens when you open multiple files at once (shift-clicking in Open
dialog)?

Comment 8

[Nick Kreeger]

For my patch:
- Note: I compared my results on Version 2005062008 (0.9a1)

- When shift-clicking and selecting multiple files to open:
The first file opens in the current tab, then all the other files are opened to
the right of existing tabs in new tabs.

- I found calls to 'openURLArray' in these files:
** BrowserTabView.mm @ Line 398 (calls to open a group of URLs in tabs) - in
0.9a1 when you have tabs open, and select a group of URLs to open in tabs, your
current tabs are replaced with the calling URLs, with the patch, it will replace
the current tab you are looking at with the first URL, and create new tabs to
the right of the existing open tabs with the other URLs.

** MainWindowController.mm @ Line 1604 (opens URLs from the toolbar text box) -
this call tells the method not to replace existing tabs, which doesn't affect
changes the patch has.

** MainWindowController.mm @ Line 1606 (open URLs from the toolbar text box) -
could not actually find a way to use this call to the -openURLArray, it looks
like it is a call when more than one URL is entered in the URL text box in the
toolbar, is that possible?

** MainController.mm @ Line 975 (-openBrowserWindowWithURLs) - gets called on
request to open URLs, for instance from the open dialog when file->open files
method (-openPanelDidEnd), if there are multiple URLs called, the same applies
to what happens in BrowserTabView.mm, the current tab is replaced with the first
url, and n number of tabs are created to the right of the current open tabs.

- I have been using a build with this patch all weekend and can't find a way to
break -openURLArray.

Comment 9

[Simon Fraser [no longer active]]

Comment on attachment 188051 [details] [diff] [review]
This contains the diff, please use this one instead of the previous patch!

>Index: browser/BrowserWindowController.mm
>===================================================================

> - (void)openURLArray:(NSArray*)urlArray replaceExistingTabs:(BOOL)replaceExisting allowPopups:(BOOL)inAllowPopups
> {
>   int curNumTabs	= [mTabBrowser numberOfTabViewItems];
>   int numItems 		= (int)[urlArray count];
>+  int selectedTabIndex  = 0;  // keeps where the selected tab starts
>+  int i;
>   
>-  for (int i = 0; i < numItems; i++)
>+  // find the current tab
>+  BrowserTabViewItem* selectedTabViewItem = [mTabBrowser selectedTabViewItem];
>+  for (i = 0; i < curNumTabs; i++)
>+  {
>+    BrowserTabViewItem* aTab = [mTabBrowser tabViewItemAtIndex:i];
>+    
>+    if (selectedTabViewItem == aTab)
>+    {
>+      selectedTabIndex = i;
>+    }
>+  }

Maybe rewrite this as:

  selectedTabIndex = [[mTabBrowser tabViewItems] indexOfObject:[mTabBrowser
selectedTabViewItem]];

>+  for (i = 0; i < numItems; i++)
>   {
>     NSString* thisURL = [urlArray objectAtIndex:i];
>     BrowserTabViewItem* tabViewItem;
> 
>     if (replaceExisting && i < curNumTabs)
>-    {
>-      tabViewItem = [mTabBrowser tabViewItemAtIndex:i];
>+    { 
>+      tabViewItem = [mTabBrowser tabViewItemAtIndex:selectedTabIndex];
>+      replaceExisting = FALSE;

I don't like code that changes the value of 'in' parameters. Also, use NO for
false in Obj-C.

In general, I don't think we should change the behavior of opening a tab group
bookmark when several tabs are already open. With this patch, it appends tabs.
Without the patch, it replaces tabs (if I understand the comments correctly).

Comment 10

[Nick Kreeger]

To change... since we only want to change the behavior of opening files, and not
bookmark requests...i will work on changing the patch to look like this:

- (void)openURLArray:(NSArray*)urlArray
replaceExistingTabs:(BOOL)replaceExisting allowPopups:(BOOL)inAllowPopups

1. need to change "replaceExistingTabs:(BOOL) to a 3 value enum.
2. define enum in BrowserWindowController.h, follow example of ENewTabContents
3. check all the hooks to this method, and switch calls to the new enum

** Note: the call should only change when opening files, and it should replace
the current tab, and all the tabs to the right and create new ones if possible

Comment 11

[Nick Kreeger]

Attachment: Updated patch that meets smfrs requests..

Updated the changes that smfr requested as documented above in my post. Tab
behavior does not change unless a file(s) are opened from file->open. This
replaces the current selected tab and all the tabs to the right of the current
tab.

Comment 12

[Simon Fraser [no longer active]]

Comment on attachment 188813 [details] [diff] [review]
Updated patch that meets smfrs requests..


>+typedef enum
>+{
>+  eRetainTabs,
>+  eReplaceTabs,
>+  eOpenFileInTab
>+	  
>+} ETabOpenPolicy;

I don't like the naming here. These should be:

eAppendTabs
eReplaceTabs
eAppendFromCurrentTab


>   for (int i = 0; i < numItems; i++)
>   {
>     NSString* thisURL = [urlArray objectAtIndex:i];
>     BrowserTabViewItem* tabViewItem;
>-
>-    if (replaceExisting && i < curNumTabs)
>+    
>+    if (tabPolicy == eReplaceTabs && i < curNumTabs)
>+      tabViewItem = [mTabBrowser tabViewItemAtIndex: i];
>+    else if (tabPolicy == eOpenFileInTab && selectedTabIndex < curNumTabs)
>     {
>-      tabViewItem = [mTabBrowser tabViewItemAtIndex:i];
>+      if (selectedTabIndex < curNumTabs)
>+        tabViewItem = [mTabBrowser tabViewItemAtIndex: selectedTabIndex++];
>+      else
>+        tabPolicy = eRetainTabs;

Can you structure this code so that ti doesn't change the value of an in
parameter?

Comment 13

[Nick Kreeger]

Attachment: Updated and revised patch

Updated patch to meet smfrs requirements for revision...

Comment 14

[Simon Fraser [no longer active]]

Patch looks mostly good.

Comment 15

[Nick Kreeger]

Attachment: fixed a few things before hopefull checkin

Swapped pointers back into loop, fixed some fuzy logic

Comment 16

[Torben]

Index: browser/BrowserWindowController.mm
===================================================================

-  // Select the first tab.
-  [mTabBrowser selectTabViewItemAtIndex:replaceExisting ? 0 : curNumTabs];

Was this an intentional change (as opposed to replacing "replaceExisting" with
"tabPolicy == eReplaceTabs")? If so, why don't we want to select the first tab
when opening tabgroups anymore?

Comment 17

[Nick Kreeger]

(In reply to comment #16)
> Index: browser/BrowserWindowController.mm
> ===================================================================
> 
> -  // Select the first tab.
> -  [mTabBrowser selectTabViewItemAtIndex:replaceExisting ? 0 : curNumTabs];
> 
> Was this an intentional change (as opposed to replacing "replaceExisting" with
> "tabPolicy == eReplaceTabs")? If so, why don't we want to select the first tab
> when opening tabgroups anymore?
Torbin,

Please read above, we are only changing the way that a group of files are opened
via file->open. The way a group of tabs open are still the same as before.

Comment 18

[Torben]

Attachment: Restore selection of first tab when opening tabgroups

>> Was this an intentional change [...]?

> Please read above, we are only changing the way that a group of files are 
> opened via file->open. 

I take this as the change was unintentional :-)

> The way a group of tabs open are still the same as before.

No, it is not (I have tested)! The attached patch, however, restores the
previous behaviour, i.e. the first tab of a tabgroup is selected.

Comment 19

[Simon Fraser [no longer active]]

I checked in something in the spirit of the last patch. Thanks!