Closed Bug 429926 Opened 16 years ago Closed 16 years ago

Make Cmd+T open a new window when no browser windows are open

Categories

(SeaMonkey :: General, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.0a1

People

(Reporter: stefanh, Assigned: stefanh)

Details

Attachments

(2 files)

The firefox eqivalent is bug 425486 where the Safari behavior was copied (just open a blank window). However, I think we should honor the loadOnNewTab prefs here. Camino actually honor the the "Load homepage when opening a new tab" pref even when you hit Cmd+T when no windows are open.
Attachment #316692 - Flags: superreview?(neil)
Attachment #316692 - Flags: review?
Attachment #316692 - Flags: review? → review?(jag)
Status: NEW → ASSIGNED
Attachment #316692 - Flags: superreview?(neil) → superreview+
Comment on attachment 316692 [details] [diff] [review]
BrowserOpenTab() even when no windows are open

>Index: suite/browser/navigator.js
>===================================================================
>         case 2:
>-          uriToLoad = getWebNavigation().currentURI.spec;
>+          uriToLoad = Components.classes["@mozilla.org/browser/global-history;2"]
>+                                .getService(Components.interfaces.nsIBrowserHistory).lastPageVisited;
>           break;

Let's say in tab 1 you go to A and in tab 2 you go to B, in that order. Then switch back to tab 1.

|getWebNavigation.currentURI.spec| is A, |C.c[...].gS(...).lastPageVisited| is B.

Maybe something like:

  uriToLoad = gBrowser ? getWebNavigation().currentURI.spec
                       : Components.classes["@mozilla.org/browser/global-history;2"]
                                   .getService(Components.interfaces.nsIBrowserHistory).lastPageVisited;
Attachment #316692 - Flags: review?(jag) → review-
Attached patch New versionSplinter Review
> Let's say in tab 1 you go to A and in tab 2 you go to B, in that order. Then
> switch back to tab 1.
> 
> |getWebNavigation.currentURI.spec| is A, |C.c[...].gS(...).lastPageVisited| is
> B.

Ugh, I should have thought about that :-/ This does what you suggested. If you feel that this is an overkill, I guess we can just open an empty tab. But it'd be nice to honor the prefs, I think (I tend to think that the Safari behavior is just a symptom of "uh, let's just do it this way since we don't know how to do it the right way").
Attachment #317598 - Flags: review?(jag)
Personally I don't really see a use case where you always want the most recently loaded page (i.s.o. the most recently viewed page) to be loaded when opening a new window or tab, or on startup.

For startup I can see the usefulness of the more generic case of "restore whatever pages were open last time the browser shut down (or crashed)" (a.k.a. Session Restore).

I can also see wanting to open a copy of the currently shown page in a new tab. Of course, with no tab/window to duplicate from, the best we can do is "most recently loaded", though I'm kinda leaning towards just loading a blank tab, or adding the code to remember the most recently viewed page, either somewhere new (could we get this for free through Session Restore?), or by updating the value of lastPageVisited when switching tabs.

Just thinking out loud. Discuss ;-)
(In reply to comment #4)
> Personally I don't really see a use case where you always want the most
> recently loaded page (i.s.o. the most recently viewed page) to be loaded when
> opening a new window or tab, or on startup.
> 
> For startup I can see the usefulness of the more generic case of "restore
> whatever pages were open last time the browser shut down (or crashed)" (a.k.a.
> Session Restore).
> 
> I can also see wanting to open a copy of the currently shown page in a new tab.
> Of course, with no tab/window to duplicate from, the best we can do is "most
> recently loaded", though I'm kinda leaning towards just loading a blank tab, or
> adding the code to remember the most recently viewed page, either somewhere new
> (could we get this for free through Session Restore?), or by updating the value
> of lastPageVisited when switching tabs.
> 
> Just thinking out loud. Discuss ;-)
> 

Hmm, I wonder if it doesn't makes more sense to use the unmodified lastPageVisited rather than updating it when switching tabs (on startup and when no browse window is open). After all, that is the last visited page, isn't it? Or maybe users don't think like that (difference between last page you looked at and last page you loaded)?
Well, that's what I'm wondering. Is switching tabs seen as a visit?

Does lastPageVisited get updated when you go back in history? E.g. I search for something, click one of the links, which takes me to a two page article, I go to the second page, then go back twice and then I open a new window. Assuming the new window pref is "Last page visited", would I expect the article's second page to open, or the search results? How about when I open the article in a second tab, go to the second page, then close the tab, which takes me back to the Google search results?

The answer is "the article's second page" to both, which is not what I would've expected (for the first, at least), though I can reason about it and understand why it's doing it, and maybe in certain cases I can make use of it ("Oh, I closed that tab I just opened, what was it again? Let me open a new window...") as a shortcut for checking the top of my history.

On a side note, reload doesn't seem to update lastPageVisited. Hrm.

For this bug perhaps we should just take the second patch, and let's deal with lastPageVisited in a new bug.
Another side-note: In the "Preferences..." --> "Browser" pane you have the choice of Blank Page, Home Page and Last page visited on Browser startup, New window and New tab.. huh.
 
> For this bug perhaps we should just take the second patch, and let's deal with
> lastPageVisited in a new bug.

Sounds good to me. Want to r+ the patch? :-)

Comment on attachment 317598 [details] [diff] [review]
New version

I was thinking we could just check gBrowser at the beginning of this function and call OpenBrowserWindow(), but that would use the "new window" pref setting instead of the "new tab" pref setting, which is probably not what we want.

>Index: suite/browser/navigator.js
>===================================================================
>+          uriToLoad = gBrowser ? getWebNavigation().currentURI.spec :
>+                      Components.classes["@mozilla.org/browser/global-history;2"]
>+                                .getService(Components.interfaces.nsIBrowserHistory).lastPageVisited;

Please indent like this:

          uriToLoad = gBrowser ? getWebNavigation().currentURI.spec 
                               : Components.classes["@mozilla.org/browser/global-history;2"]
                                           .getService(Components.interfaces.nsIBrowserHistory)
                                           .lastPageVisited;

Or with no wrapping on 80:

          const HISTORY_CONTRACTID = "@mozilla.org/browser/global-history;2";
          const nsIBrowserHistory = Components.interfaces.nsIBrowserHistory;
          uriToLoad = gBrowser ? getWebNavigation().currentURI.spec 
                               : Components.classes[HISTORY_CONTRACTID]
                                           .getService(nsIBrowserHistory)
                                           .lastPageVisited;

r=jag with at least the indentation nit fixed.
Attachment #317598 - Flags: superreview?(neil)
Attachment #317598 - Flags: review?(jag)
Attachment #317598 - Flags: review+
Btw, when testing this (before & after the patch) I noticed that if you hit cmd+t a couple of times and the previous tab hasn't finished loading you'll get a tab with about:blank. Happens on branch too. Hmm.
Attachment #317598 - Flags: superreview?(neil) → superreview+
Landed with fixed indentation:

Checking in suite/browser/navigator.js;
/cvsroot/mozilla/suite/browser/navigator.js,v  <--  navigator.js
new revision: 1.622; previous revision: 1.621
done
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.0alpha
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: