Fix extra tab

[jeremypw]

Fixes #736

• MainWindow: Do not add default tab - responsibility of Application now
• MainWindow: Save tabs from each window (settings permitting) - the last window closed will overwrite previous windows tabs
• Application: Leave it to MainWindow to restore tabs or not. The first window will always restore tabs if settings permit
• Application: Distinguish when a working directory was explicitly requested (-w option) when an instance is already running in order to allow different behaviour for -t option It is an open question whether the -t option should behave the same as the new-tab action (honor "follow-last-tab" setting). It also fixes a small issue in master where if Terminal is launched with io.elementary.terminal when an instance is already added then a new unrequested tab is added to the first window.

To clarify what is going on, MainWindow does not automatically add a default tab.

TESTING

Where is the path to a folder that is not amongst the saved tabs.

With History/restore-tabs ON:

Open first instance of Terminal from command line with io.elementary.terminal -nw <folder-path> Terminal opens containing one tab at the requested path plus restored tabs Close Terminal and reopen with io.elementary.terminal Terminal opens with restored tabs including the requested path

Now open second instance of Terminal from the command line with io.elementary.terminal -nw <folder-path> A new window opens containing only the requested tab (difference from master - fixes issue)

Close the first window then close the second window Reopen Terminal Terminal opens with only the tab from the second (last closed) window

Open second instance with `io.elementary.terminal -w The first window is focused with the tab selected (already exists)

Open second instance with `io.elementary.terminal -w The first window is focused with an additional tab at the new path - the new tab is selected

Open second instance with io.elementary.terminal -t The first window is focused with an additional tab at the current working directory.

Open second instance with io.elementary.terminal -nt A new window is opened with a single tab at the current working directory.

Open a second instance io.elementary.terminal The first window is focused and no extra tab added (difference from master)

With History/restore-tabs OFF

Open first instance of Terminal from command line with io.elementary.terminal -nw <folder-path> Terminal opens containing one tab at the requested path. Close Terminal and reopen with io.elementary.terminal Terminal opens with the default tab

Now open second instance of Terminal from the command line with io.elementary.terminal -nw <folder-path> A new window opens containing only the requested tab (difference from master - fixes issue)

Close the first window then close the second window Reopen Terminal Terminal opens with the default tab

Running Commands

The -e and -x options behave similarly. If they create the first window then tabs are restored (if settings permit) and then an additional tab at the current working directory is created for each command to run in. If used with the -n option to create an additional window then only tabs running the commands are created.

[jeremypw]

Probably needs splitting into two PRs.

[jeremypw]

Now only fixes the linked issue. A separate PR dealing with option handling will be prepared later.

[Marukesu]

Application: Distinguish when a working directory was explicitly requested (-w option) when an instance is already running in order to allow different behaviour for -t option It is an open question whether the -t option should behave the same as the new-tab action (honor "follow-last-tab" setting).

The idea with the rewrite is that working directory isn't a "real" option, so it shouldn't cause different behaviour by itself.

It also fixes a small issue in master where if Terminal is launched with io.elementary.terminal when an instance is already added then a new unrequested tab is added to the first window.

the behaviour of opening a new tab when invoked in a directory without a tab already open is also intentional. the rationale is that any command line invocation is done with the intention of open a tab or window, so it act as a "weakened" (it won't create a tab, if one in the current directory already exists) version of -t.

if we are going to change the behaviour here, i would prefer that it's to drop the weakened -t for assuming a real one.

[jeremypw]

The idea with the rewrite is that working directory isn't a "real" option,

Not sure what this means. The -w and -t options do differ in that the former requires a following URI and the latter does not (it just adds a new default tab).

[jeremypw]

the behaviour of opening a new tab when invoked in a directory without a tab already open is also intentional. the rationale is that any command line invocation is done with the intention of open a tab or window

Again, I do not understand this. The change (and the whole PR) is about not opening two tabs when the user would expect only one. The change from master being discussed here is about when Terminal is invoked without any options so the user is not requesting a new tab. Usually it is because the user has forgotten Terminal is already running so it should behave as if the user had just clicked on the dock icon again - i.e. it should just focus the existing window.

[jeremypw]

There is a case for reconsidering whether we need both -w and -t options or whether they should do the same or different things, bearing in mind we do not want to break existing scripts if we can help it. But I am not sure this PR is the place - I am just trying to make sure Terminal continues to follow the information that the -h option gives while fixing the issue of unexpected extra tabs.

[jeremypw]

Elementary Terminal is currently different from other terminals in that it has a "follow-last-tab" setting (unexposed) that is by default false so that additional tabs are by default opened at the user's home directory not the current directory. This complicates matters considerably. Might be worth discussing whether that setting should continue to be supported or whether we should just emulate other major terminals. But that is for another PR.

[jeremypw]

so it act as a "weakened" (it won't create a tab, if one in the current directory already exists) version of -t.

This would make commandline invocation different from invocation from the dock or App Menu which both just focus an existing primary window, so I disagree.

Interestingly gnome-terminal always opens a new window when a primary window exists even when invoked with no options. xterm does the same. Following that would probably simplify things but would need design team sign off.

[jeremypw]

@Marukesu I'm going to have to ask you to look at that race issue in the testing of adding tabs I am afraid. I tried a few things but I do not fully understand the code :disappointed: Even getting the n_tabs directly from notebook does not work. The callback is running before MainWindow even starts to add the tab. Looks like iterate_context is not blocking for long enough for MainWindow to complete everything needed to add a tab.

UPDATE: Wait a minute I think I have been misinterpreting the logs - there are two calls to option () within each test and I have been mistaking the output from one as belonging to the other. I think the problem is due to an unexpected tab being added in the Test environment due to an attempt to restore tabs

[jeremypw]

Not sure why we're getting the expected tab count in the --execute test but not the new-tab option test though :shrug:

[jeremypw]

So it seems that for each call to option () in the Test environment, the application command_line function gets called twice? Is this intended? It is not what happens when launching the application on the desktop. It results in unexpected numbers of tabs appearing (at least, different from real life).

[Marukesu]

yes it does, that is the only way to test the options and actions without having interference of the command line. the first invocation override the default handler, so for the tests, the second invocation appears as if it was the first one.

to be a little more clear:

   var app = new Terminal.Application ();
   ulong signal_id = app.command_line.connect (() => { // this override the command_line () implementation of Terminal.Application
        app.disconnect (signal_id); // make the next command_line() call go to the default handler
        // ...

EDIT: oh, sorry, now i see, you say the second line in the overriden launching, it's needed because some of the test can only be properly tested in a secondary launching (like the new-window test), so it tries to act if the command_line was called with a already existent instance.

[jeremypw]

I have found a fix for the tests by allowing the test to choose whether or not command_line is called with a null parameter before the test commandline. This defaults to "yes" but for the "new-tab" tests it needs to be "no" in order to get the "expected" result (i.e. the same as real life). I havent fully got to the bottom of it but there does seem to be a timing issue when adding a second tab to the existing window in the new tab test.

[jeremypw]

I'll have another look tomorrow - I've been down enough rabbit holes and followed enough red herrings today :stuck_out_tongue_closed_eyes:

[jeremypw]

Closing as this has become too complicated/contentious. I'll try and find a simpler, if more limited, solution to the immediate issue of unwanted tabs being added every time the terminal is launched from the dock.