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 all 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
43 changes: 27 additions & 16 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,15 @@ 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) {
/* 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);
if (window == null ||
options.lookup ("new-window", "b", out new_window) && new_window) {
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 ();

unowned string[] commands;
unowned string command;
bool new_tab, minimized;
Expand All @@ -237,15 +235,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 (can happen if trying to execute invalid commands)
add_default_tab (window, false);

if (options.lookup ("minimized", "b", out minimized) && minimized) {
window.iconify ();
} else {
Expand All @@ -264,7 +267,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
106 changes: 41 additions & 65 deletions src/MainWindow.vala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,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 @@ -41,7 +42,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 @@ -105,30 +105,19 @@ namespace Terminal {
{ ACTION_OPEN_IN_BROWSER, action_open_in_browser }
};

public MainWindow (Terminal.Application app, bool recreate_tabs = true) {
public MainWindow (Terminal.Application app) {
Object (
app: app,
recreate_tabs: recreate_tabs
app: app
);

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

public MainWindow.with_coords (Terminal.Application app, int x, int y,
bool recreate_tabs, bool ensure_tab) {
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 Down Expand Up @@ -259,28 +248,17 @@ namespace Terminal {

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

if (recreate_tabs) {
open_tabs ();
if (app.get_windows ().length () == 1) {
open_saved_tabs ();
}
}

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 @@ -295,7 +273,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 @@ -550,7 +528,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 @@ -659,9 +638,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 @@ -822,7 +799,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 @@ -831,12 +808,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 @@ -854,9 +831,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 @@ -869,10 +843,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 @@ -896,7 +866,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 @@ -920,8 +890,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 @@ -1189,7 +1161,11 @@ namespace Terminal {

private void action_new_tab () {
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 @@ -1348,8 +1324,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 @@ -1362,22 +1338,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
Loading