Fix extra tab #737

jeremypw · 2023-08-21T12:00:15Z

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 · 2023-08-22T12:23:02Z

Probably needs splitting into two PRs.

jeremypw · 2023-08-23T17:24:38Z

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

Marukesu · 2023-08-25T17:46:29Z

src/Application.vala

@@ -201,7 +201,11 @@ public class Terminal.Application : Gtk.Application {
        );

        var new_window_action = new SimpleAction ("new-window", null);
-        new_window_action.activate.connect (new_window);
+        new_window_action.activate.connect (() => {
+            var win = new MainWindow (this, false); // Do not restore tabs


this will break having the first window be created by the action when we enable DBus activation (the only thing that was stopping us of doing that was the duplicate tab). so the active_window == null is necessary here.

Not sure what this means 😞 as I am not sure how DBus activation works. Could you give more details or suggested code?

src/Application.vala

src/MainWindow.vala

src/tests/Application.vala

Marukesu · 2023-08-27T20:08:24Z

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.

src/Application.vala

Marukesu · 2023-08-27T20:34:50Z

src/Application.vala

+        // Only if no other tabs, add default tab
+        add_default_tab (window, false);
+


this won't respect the current working directory as the others.

Whether or not it respects the current working directory depends on the "follow-last-tab" setting (that is what the default tab does) since it uses the action-new-tab. This is for consistency with other situations where there is no specified tab.

This also replaces functionality removed from MainWindow so I want to keep the behaviour the same.

this is already being inconsistent with the others command line options handling, where the current working directory is always respected. the code in the action-new-tab won't respect it either, it will either use the last tab directory or the current working directory of the first invocation.

src/MainWindow.vala

Marukesu · 2023-08-27T20:45:37Z

src/MainWindow.vala

+                } else {
+                    new_tab (Environment.get_current_dir ());
+                }


it's better to keep any "current directory" concept exclusive to the application class. because of the local/primary instance logic.

So what do you suggest?

Note: Environment.get_current_dir () is already in use in TerminalWidget

So what do you suggest?

Only the Application class known the real working directory for the current invocation, so either action-new-tab is updated to take a path, or we don't try to use the action in the command line context, where is the only place that a concept of working directory exists.

Note: Environment.get_current_dir () is already in use in TerminalWidget

It actually isn't, the only place that we call activate_shell() without a location is in the case that we call new_tab() with a empty location, what isn't possible.

Marukesu · 2023-08-27T20:46:43Z

src/tests/Application.vala

@@ -145,7 +145,7 @@ namespace Terminal.Test.Application {
                unowned var window = (MainWindow) application.active_window;
                assert_nonnull (window);
                var n_tabs = (int) window.terminals.length ();
-                assert_cmpint (n_tabs, CompareOperator.EQ, 2);
+                assert_cmpint (n_tabs, CompareOperator.EQ, 1); // No default tab added as well in this case


this test was made to assert that we always create a new tab when requested, so this change is wrong.

It is necessary for the test to pass. If the original test is correct then the app is wrong

If you run io.elementary.terminal -t manually without a Terminal window open you get one tab (as expected). If you run the same command with the primary window open (but no restored tabs) then you get 2 tabs - the original default one and the requested additional one.

I am not exactly sure what happens in the test environment - does a window get created before the options are processed or not?

After some investigation it seems like before processing the commandline in option () there is 1 window with 1 tab. So I would have expected -t true to create a new tab like when used manually. But it doesn't in the test environment for some reason.

OK, so it is race - the callback is executed before the window terminal list is updated in the on_tab_added signal handler. Need to get a more immediate count of true number of tabs in the notebook. I'll review whether the terminals list is needed or not.

so, what's happening here is that add_tab_with_working_directory() is matching the already existent tab (from the first launch), and add_default_tab() is not creating one because window.terminals.length () == 1.

Marukesu · 2023-08-27T20:46:54Z

src/tests/Application.vala


            // valid
            option ("{'execute':<[b'%s']>}".printf (string.joinv ("',b'", execute)), "@a{sv} {}", () => {
                unowned var window = (MainWindow) application.active_window;
                assert_nonnull (window);
                var n_tabs = (int) window.terminals.length ();
-                assert_cmpint (n_tabs, CompareOperator.EQ, 5); // include the guaranted extra tab
+                assert_cmpint (n_tabs, CompareOperator.EQ, 5); // Includes initial default tab added when no tabs restored.


now that we aren't creating the extra tab, this should be 4.

Hmm then it should be failing - will investigate.

So what I think is happening is that the test environment opens a window without restoring tabs before processing the option. So there is already a default tab in the window and the execute option creates 4 more, one for each command, giving a total of 5. However this does not seem to be the case for the -t option test so tbh I am not on what exactly happening in the test environment.

If you manually execute multiple commands before any window is opened e.g. with a commandline io.elementary.terminal -e "true" -e "echo Hi" then no extra default tab appears.

Further investigation indicates that 5 tabs are added during this test including one due to a restore-saved-tabs attempt that should not be happening.

simillary as the new-tab test, there's 1 tab from first launch, and 4 from the commands, the sixty one seems strange.

jeremypw · 2023-08-28T11:06:25Z

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 · 2023-08-28T11:19:37Z

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 · 2023-08-28T11:27:31Z

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 · 2023-08-28T11:38:58Z

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 · 2023-08-28T11:49:08Z

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 · 2023-08-28T15:54:12Z

@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 😞 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 · 2023-08-28T15:57:34Z

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

jeremypw · 2023-08-28T17:06:39Z

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 · 2023-08-28T17:24:11Z

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 · 2023-08-28T18:31:15Z

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 · 2023-08-28T18:33:07Z

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

jeremypw · 2023-10-30T17:50:32Z

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.

Jeremy Paul Wootten added 3 commits August 23, 2023 17:18

DO not restore tabs for new window, do not save tabs if not restored

19a8a56

Ensure no tabs restored for new window; ensure at least one tab

666807b

Add extra test and comments

6de245a

jeremypw force-pushed the fix-extra-tab branch from cd50a2d to 6de245a Compare August 23, 2023 17:13

Lose unneeded new property

f317a33

jeremypw marked this pull request as ready for review August 23, 2023 17:39

jeremypw requested a review from a team August 23, 2023 17:40

Marukesu reviewed Aug 25, 2023

View reviewed changes

Jeremy Paul Wootten added 4 commits August 27, 2023 09:26

Revert new_window action anonymous handler

0399db3

Behave more like Files

2695426

Fix -nw <path> option

9788196

Lose unneeded test

38c757b

jeremypw requested a review from Marukesu August 27, 2023 18:06

Marukesu reviewed Aug 27, 2023

View reviewed changes

Jeremy Paul Wootten added 4 commits August 28, 2023 16:59

Revert public action_new_tab ()

98ab0bd

Lose unneeded comment

d161f1c

Remove temporary assert

5d61b99

Lose unneeded properties

10112d4

jeremypw marked this pull request as draft August 28, 2023 16:55

Jeremy Paul Wootten added 2 commits August 28, 2023 19:13

Fix tests to choose whether to create a default window first

3f0ec4e

Small cleanups

4dd2b10

zeebok and others added 2 commits September 13, 2023 23:54

Merge branch 'master' into fix-extra-tab

3a99e7a

Merge branch 'master' into fix-extra-tab

56989a3

jeremypw closed this Oct 30, 2023

		// Only if no other tabs, add default tab
		add_default_tab (window, false);

Fix extra tab #737

Fix extra tab #737

Conversation

jeremypw commented Aug 21, 2023 • edited Loading

TESTING

With History/restore-tabs ON:

With History/restore-tabs OFF

Running Commands

jeremypw commented Aug 22, 2023

jeremypw commented Aug 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Marukesu commented Aug 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremypw Aug 28, 2023 • edited Loading

Choose a reason for hiding this comment

jeremypw Aug 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremypw commented Aug 28, 2023

jeremypw commented Aug 28, 2023

jeremypw commented Aug 28, 2023

jeremypw commented Aug 28, 2023

jeremypw commented Aug 28, 2023

jeremypw commented Aug 28, 2023 • edited Loading

jeremypw commented Aug 28, 2023

jeremypw commented Aug 28, 2023

Marukesu commented Aug 28, 2023 • edited Loading

jeremypw commented Aug 28, 2023

jeremypw commented Aug 28, 2023

jeremypw commented Oct 30, 2023

jeremypw commented Aug 21, 2023 •

edited

Loading

jeremypw Aug 28, 2023 •

edited

Loading

jeremypw Aug 28, 2023 •

edited

Loading

jeremypw commented Aug 28, 2023 •

edited

Loading

Marukesu commented Aug 28, 2023 •

edited

Loading