Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix extra tab #737

Closed
wants to merge 16 commits into from
Closed

Fix extra tab #737

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
19a8a56
DO not restore tabs for new window, do not save tabs if not restored
Aug 23, 2023
666807b
Ensure no tabs restored for new window; ensure at least one tab
Aug 23, 2023
6de245a
Add extra test and comments
Aug 23, 2023
f317a33
Lose unneeded new property
Aug 23, 2023
0399db3
Revert new_window action anonymous handler
Aug 27, 2023
2695426
Behave more like Files
Aug 27, 2023
9788196
Fix -nw <path> option
Aug 27, 2023
38c757b
Lose unneeded test
Aug 27, 2023
98ab0bd
Revert public action_new_tab ()
Aug 28, 2023
d161f1c
Lose unneeded comment
Aug 28, 2023
5d61b99
Remove temporary assert
Aug 28, 2023
10112d4
Lose unneeded properties
Aug 28, 2023
3f0ec4e
Fix tests to choose whether to create a default window first
Aug 28, 2023
4dd2b10
Small cleanups
Aug 28, 2023
3a99e7a
Merge branch 'master' into fix-extra-tab
zeebok Sep 14, 2023
56989a3
Merge branch 'master' into fix-extra-tab
Oct 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 29 additions & 15 deletions src/Application.vala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,9 +107,9 @@ public class Terminal.Application : Gtk.Application {
if (options.lookup ("working-directory", "^&ay", out working_directory)) {
if (working_directory != "\0") {
Environment.set_current_dir (working_directory); // will be sent via platform-data
// Need to know in commandline () whether this option was present and valid so do not remove
// options.remove ("working-directory");
}

options.remove ("working-directory");
}

if (options.lookup (OPTION_REMAINING, "^a&ay", out args)) {
Expand Down Expand Up @@ -217,17 +217,18 @@ public class Terminal.Application : Gtk.Application {
unowned var options = command_line.get_options_dict ();
var window = (MainWindow) active_window;
bool new_window;
if (window == null ||
options.lookup ("new-window", "b", out new_window) && new_window) {

if (window == null || options.lookup ("new-window", "b", out new_window) && new_window) {
/* Uncertain whether tabs should be restored when app is launched with working directory from commandline.
* Currently they are set to restore (subject to the restore-tabs setting).
* If it is desired that tabs should never be restored in these circimstances add another check below.
*/
bool restore_tabs = !("commandline" in options || "execute" in options) || window == null;
window = new MainWindow (this, restore_tabs);
window = new MainWindow (this);
}

unowned var working_directory = command_line.get_cwd ();
unowned string working_directory;
var wd_was_requested = options.lookup ("working-directory", "^&ay", out working_directory);
working_directory = command_line.get_cwd ();

assert (working_directory != "\0"); // Can we be sure of this? If not, need fallback

jeremypw marked this conversation as resolved.
Show resolved Hide resolved
unowned string[] commands;
unowned string command;
bool new_tab, minimized;
Expand All @@ -237,15 +238,20 @@ public class Terminal.Application : Gtk.Application {
if (options.lookup ("execute", "^a&ay", out commands)) {
for (var i = 0; commands[i] != null; i++) {
if (commands[i] != "\0") {
window.add_tab_with_working_directory (working_directory, commands[i], new_tab);
window.add_tab_with_working_directory (working_directory, commands[i]);
}
}
} else if (options.lookup ("commandline", "^&ay", out command) && command != "\0") {
window.add_tab_with_working_directory (working_directory, command, new_tab);
} else {
window.add_tab_with_working_directory (working_directory, null, new_tab);
window.add_tab_with_working_directory (working_directory, command);
} else if (wd_was_requested) {
window.add_tab_with_working_directory (working_directory, null);
} else if (new_tab) {
window.add_tab_with_working_directory (working_directory, null); //FIXME Should we honor "follow-last-tab" setting?
}

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (options.lookup ("minimized", "b", out minimized) && minimized) {
window.iconify ();
} else {
Expand All @@ -264,7 +270,15 @@ public class Terminal.Application : Gtk.Application {
}

private void new_window () {
new MainWindow (this, active_window == null).present ();
var window = new MainWindow (this);
add_default_tab (window, false);
window.present ();
}

private void add_default_tab (MainWindow window, bool force) {
if (force || window.terminals.length () == 0) {
window.actions.activate_action (window.ACTION_NEW_TAB, null);
}
}

private void close () {
Expand Down
120 changes: 54 additions & 66 deletions src/MainWindow.vala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ namespace Terminal {
private Gtk.ToggleButton search_button;
private Dialogs.ColorPreferences? color_preferences_dialog;
private Granite.AccelLabel open_in_browser_menuitem_label;
private bool tabs_were_restored = false;

private HashTable<string, TerminalWidget> restorable_terminals;
private bool is_fullscreen = false;
Expand All @@ -40,7 +41,6 @@ namespace Terminal {

public bool unsafe_ignored;
public bool focus_restored_tabs { get; construct; default = true; }
public bool recreate_tabs { get; construct; default = true; }
public bool restore_pos { get; construct; default = true; }
public uint focus_timeout { get; private set; default = 0;}
public Gtk.Menu menu { get; private set; }
Expand Down Expand Up @@ -104,30 +104,27 @@ namespace Terminal {
{ ACTION_OPEN_IN_BROWSER, action_open_in_browser }
};

public MainWindow (Terminal.Application app, bool recreate_tabs = true) {
Object (
app: app,
recreate_tabs: recreate_tabs
);
public uint window_number { get; construct; }

if (!recreate_tabs) {
new_tab ("");
public bool is_first_window {
get {
return (window_number == 0);
}
}

public MainWindow.with_coords (Terminal.Application app, int x, int y,
bool recreate_tabs, bool ensure_tab) {
public MainWindow (Terminal.Application app) {
Object (
app: app
);
}

public MainWindow.with_coords (Terminal.Application app, int x, int y) {
Object (
app: app,
restore_pos: false,
recreate_tabs: recreate_tabs
restore_pos: false
);

move (x, y);

if (!recreate_tabs && ensure_tab) {
new_tab ("");
}
}

static construct {
Expand All @@ -153,6 +150,7 @@ namespace Terminal {
}

construct {
window_number = app.get_windows ().length ();
actions = new SimpleActionGroup ();
actions.add_action_entries (ACTION_ENTRIES, this);
insert_action_group ("win", actions);
Expand Down Expand Up @@ -260,28 +258,20 @@ namespace Terminal {

restorable_terminals = new HashTable<string, TerminalWidget> (str_hash, str_equal);

if (recreate_tabs) {
open_tabs ();
if (is_first_window) {
jeremypw marked this conversation as resolved.
Show resolved Hide resolved
open_saved_tabs ();
}

// This no longer ensures at least one tab is opened.
// The creator of the window (Application.vala or on_tab_moved) must add a tab if necessary
jeremypw marked this conversation as resolved.
Show resolved Hide resolved
}

public void add_tab_with_working_directory (string? directory, string? command = null, bool create_new_tab = false) {
public void add_tab_with_working_directory (string location, string? command = null) {
/* This requires all restored tabs to be initialized first so that the shell location is available */
/* Do not add a new tab if location is already open in existing tab */
string? location = null;

if (directory == null || directory == "") {
if (notebook.tabs.first () == null || command != null || create_new_tab) { //Ensure at least one tab
new_tab ("", command);
}

return;
} else {
location = directory;
}
/* Does not add a new tab if location is already open in existing tab */

/* We can match existing tabs only if there is no command and create_new_tab == false */
if (command == null && !create_new_tab) {
if (command == null && location != null) {
var file = File.new_for_commandline_arg (location);
foreach (Granite.Widgets.Tab tab in notebook.tabs) {
var terminal_widget = get_term_widget (tab);
Expand All @@ -296,7 +286,7 @@ namespace Terminal {
}
}

new_tab (location, command);
new_tab (location, command); // Always execute command in new tab
}

/** Returns true if the code parameter matches the keycode of the keyval parameter for
Expand Down Expand Up @@ -540,7 +530,8 @@ namespace Terminal {
}

private void restore_saved_state (bool restore_pos = true) {
if (Granite.Services.System.history_is_enabled () &&
if (!tabs_were_restored && // Only restore tabs once
Granite.Services.System.history_is_enabled () &&
Application.settings.get_boolean ("remember-tabs")) {

saved_tabs = Terminal.Application.saved_state.get_strv ("tabs");
Expand Down Expand Up @@ -649,9 +640,7 @@ namespace Terminal {
var new_window = new MainWindow.with_coords (
app,
x,
y,
false,
false
y
);

var terminal_widget = get_term_widget (tab);
Expand Down Expand Up @@ -812,7 +801,7 @@ namespace Terminal {
current_terminal.grab_focus ();
}

private void open_tabs () {
private void open_saved_tabs () {
string[] tabs = {};
double[] zooms = {};
int focus = 0;
Expand All @@ -821,12 +810,12 @@ namespace Terminal {
if (Granite.Services.System.history_is_enabled () &&
Application.settings.get_boolean ("remember-tabs")) {

tabs_were_restored = true; // An attempt was made to restore.
tabs = saved_tabs;
var n_tabs = tabs.length;

if (n_tabs == 0) {
tabs += Environment.get_home_dir ();
zooms += default_zoom;
return;
} else {
foreach (unowned string zoom_s in saved_zooms) {
var zoom = double.parse (zoom_s); // Locale independent
Expand All @@ -844,9 +833,6 @@ namespace Terminal {
}

focus = Terminal.Application.saved_state.get_int ("focused-tab");
} else {
tabs += Environment.get_current_dir ();
zooms += default_zoom;
}

assert (zooms.length == tabs.length);
Expand All @@ -859,10 +845,6 @@ namespace Terminal {
null_dirs++;
tabs[i] = "";
}

if (null_dirs == tabs.length) {
tabs[0] = Environment.get_current_dir ();
}
}

Terminal.Application.saved_state.set_strv ("tabs", {});
Expand All @@ -886,7 +868,7 @@ namespace Terminal {
index++;
}

if (focus_restored_tabs) {
if (index > 0 && focus_restored_tabs) {
var tab = notebook.get_tab_by_index (focus.clamp (0, notebook.n_tabs - 1));
notebook.current = tab;
get_term_widget (tab).grab_focus ();
Expand All @@ -910,8 +892,10 @@ namespace Terminal {


var tab = create_tab (
location != null ? Path.get_basename (location) : TerminalWidget.DEFAULT_LABEL,
null, terminal_widget); //Set correct label now to avoid race when spawning shell
Path.get_basename (location),
null,
terminal_widget
); //Set correct label now to avoid race when spawning shell

terminal_widget.child_exited.connect (() => {
if (!terminal_widget.killed) {
Expand Down Expand Up @@ -1177,9 +1161,13 @@ namespace Terminal {
// Closing a tab will switch to another, which will trigger check for same names
}

private void action_new_tab () {
public void action_new_tab () {
jeremypw marked this conversation as resolved.
Show resolved Hide resolved
if (Application.settings.get_boolean ("follow-last-tab")) {
new_tab (current_terminal.get_shell_location ());
if (current_terminal != null) {
new_tab (current_terminal.get_shell_location ());
} else {
new_tab (Environment.get_current_dir ());
}
Comment on lines +1166 to +1168
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So what do you suggest?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

} else {
new_tab (Environment.get_home_dir ());
}
Expand Down Expand Up @@ -1337,8 +1325,8 @@ namespace Terminal {
string[] opened_tabs = {};
string[] zooms = {};

// The last closed window will override the settings for any previously closed window
Application.saved_state.set_double ("zoom", current_terminal.font_scale);

if (Granite.Services.System.history_is_enabled () &&
Application.settings.get_boolean ("remember-tabs")) {

Expand All @@ -1351,22 +1339,22 @@ namespace Terminal {
}
}
});
}

Terminal.Application.saved_state.set_strv (
"tabs",
opened_tabs
);
Terminal.Application.saved_state.set_strv (
"tabs",
opened_tabs
);

Terminal.Application.saved_state.set_strv (
"tab-zooms",
zooms
);
Terminal.Application.saved_state.set_strv (
"tab-zooms",
zooms
);

Terminal.Application.saved_state.set_int (
"focused-tab",
notebook.current != null ? notebook.get_tab_position (notebook.current) : 0
);
Terminal.Application.saved_state.set_int (
"focused-tab",
notebook.current != null ? notebook.get_tab_position (notebook.current) : 0
);
}
}

/** Return enough of @path to distinguish it from @conflict_path **/
Expand Down
6 changes: 3 additions & 3 deletions src/tests/Application.vala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

});

option ("{'new-tab':<false>}", "@a{sv} {}", () => {
Expand All @@ -169,14 +169,14 @@ namespace Terminal.Test.Application {
});

GLib.Test.add_func ("/application/command-line/execute", () => {
string[] execute = { "true", "echo test", "echo -e te\\tst", "false" };
string[] execute = { "true", "echo test", "echo -e te\\tst", "false" }; // 4 commands

// 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm then it should be failing - will investigate.

Copy link
Collaborator Author

@jeremypw jeremypw Aug 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@jeremypw jeremypw Aug 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

});

// invalid
Expand Down