Skip to content

Save tabs when directory changed, not on_terminal_title_changed (#1633) - #1786

Merged
gsemet merged 4 commits into
masterfrom
save-tabs-when-directory-changed
Oct 20, 2020
Merged

Save tabs when directory changed, not on_terminal_title_changed (#1633)#1786
gsemet merged 4 commits into
masterfrom
save-tabs-when-directory-changed

Conversation

@mlouielu

@mlouielu mlouielu commented Jul 21, 2020

Copy link
Copy Markdown
Collaborator

This should fix #1633.

This commit remove @save_tabs_when_changed on on_terminal_title_changed,
and put the terminal directory into ther instance. This is because
vte:current-directory-uri-changed is not worked, and current-directory-uri
always return None.

When on_terminal_title_changed called, it will check if the directory
changed for this terminal, if so, then save the tabs by the setting.

mlouielu added 2 commits July 21, 2020 22:55
@mlouielu
Update session.json when directory changed, not terminal title changed
3cbd534 
This should fix #1633.

This commit remove `@save_tabs_when_changed` on `on_terminal_title_changed`,
and put the terminal directory into ther instance. This is because
`vte:current-directory-uri-changed` is not worked, and `current-directory-uri`
always return `None`.

When `on_terminal_title_changed` called, it will check if the directory
changed for this terminal, if so, then save the tabs by the setting.
@mlouielu
Add SLUG
02a70be
@mlouielu mlouielu mentioned this pull request Jul 21, 2020
Closed
@mlouielu

mlouielu commented Jul 21, 2020

Copy link
Copy Markdown
Collaborator Author

This may not work for split window...

ulidtko
ulidtko requested changes Jul 21, 2020
View reviewed changes

@ulidtko ulidtko left a comment •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm somewhat dubious of the overall approach here. It seems to rest on the confusion that VTE titles are a representation of the CWD (current working directory). They aren't.

It just so happens that bash on many systems by default sends [{{cwd}}] as the VTE title, and many people don't customize that, neither interact with other programs which set VTE titles too. Still many other people do do that, and those will see why this PR misses the point.

So instead of adding more logic entrenching the confusion — some logic needs to be removed. Specifically, as the original issue reporter said:

there is no reason to save the "label" unless when "custom_label_set": true. It will anyway not be used when restoring the tabs.

Please follow this. When a program sends to Guake please name my tab "IAmFoobar" — there's no slightest need to persist "IAmFoobar" anywhere; the program will send it again. When the user however names a tab "MyAwesomeSpecialTab" — we want to ignore what the programs send, and we want to save this manual label into session.json and restore it when restarted.

@mlouielu

mlouielu commented Jul 22, 2020

Copy link
Copy Markdown
Collaborator Author

I'm somewhat dubious of the overall approach here. It seems to rest on the confusion that VTE titles are a representation of the CWD (current working directory). They aren't.

This PR didn't use VTE titles as their CWD, here we only use on_terminal_title_changed as a signal that "terminal directory may have changed"

It just so happens that bash on many systems by default sends [{{cwd}}] as the VTE title, and many people don't customize that, neither interact with other programs which set VTE titles too. Still many other people do do that, and those will see why this PR misses the point.

I've test with default bash shell, and Vte.Terminal.get_current_directory() will changed when the directory changed.

Or are there any cases that Vte.Terminal.get_current_directory() will failed even if directory changed? If so, then the save-tabs function will not work too. Because the path that save to the session.json also use Vte.Termianl.get_current_directory().

So instead of adding more logic entrenching the confusion — some logic needs to be removed. Specifically, as the original issue reporter said:

there is no reason to save the "label" unless when "custom_label_set": true. It will anyway not be used when restoring the tabs.

Please follow this. When a program sends to Guake please name my tab "IAmFoobar" — there's no slightest need to persist "IAmFoobar" anywhere; the program will send it again. When the user however names a tab "MyAwesomeSpecialTab" — we want to ignore what the programs send, and we want to save this manual label into session.json and restore it when restarted.

Again, this PR only use on_terminal_title_changed() signal as a signal that terminal directory may changed.

@gsemet
gsemet merged commit a11063f into master Oct 20, 2020
@einarjon einarjon mentioned this pull request Nov 10, 2020
Closed
@Davidy22
Davidy22 deleted the save-tabs-when-directory-changed branch May 12, 2023 02:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ulidtko ulidtko ulidtko requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

save_tabs function is way too aggressive

3 participants

@mlouielu @ulidtko @gsemet