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.

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

Adds support for saving and restoring pane titles. #431

Merged
merged 1 commit into from
Apr 10, 2022

Conversation

Hologos
Copy link
Contributor

@Hologos Hologos commented Mar 28, 2022

This PR is a rebase of #342 with addressed PR comments, fixes for restore_shell_history() and dump_shell_history(). It fixes #330 as well.

I am currently testing it but pls test it thoroughly yourself.

It is a breaking change so I added a comment to the changelog.

@Hologos
Copy link
Contributor Author

Hologos commented Mar 28, 2022

TODOs:

  • add pane titles into test fixture files
  • BUG: fix setting pane title when pane already exists
  • BUG: doesn't restore pane title for first pane when doing complete restore

@Hologos
Copy link
Contributor Author

Hologos commented Mar 28, 2022

Instead of naming the pane with a standalone command, can we somehow set the correct pane_title when the pane is created?

Originally posted by @bruno- in #342 (comment)

Sadly it's not possible, I created a function that is called within new_pane() and restore_pane() (if pane already exists) with commit 4a3b59c. Let me know if I should revert to calling it like in the original PR.

@Hologos Hologos force-pushed the feature/save-pane-title branch from e0d8299 to a516947 Compare March 29, 2022 11:38
@Hologos
Copy link
Contributor Author

Hologos commented Mar 29, 2022

Last commit reverts to setting pane_title at the end of restore_pane(). Otherwise it has to be called in new_pane(), new_window() and new_session() and once again after the very first pane is killed. This covers all cases.

@@ -302,7 +312,7 @@ restore_window_properties() {
}

restore_shell_history() {
awk 'BEGIN { FS="\t"; OFS="\t" } /^pane/ { print $2, $3, $6, $9; }' $(last_resurrect_file) |
awk 'BEGIN { FS="\t"; OFS="\t" } /^pane/ { print $2, $3, $6, $10; }' $(last_resurrect_file) |
Copy link
Member

Choose a reason for hiding this comment

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

I'm trying to get rid of this feature for a long time

https://github.com/tmux-plugins/tmux-resurrect/blob/master/docs/restoring_shell_history.md

Please don't fix this, I'll remove "restore shell history" after we merge restoring pane titles feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does it mean I should revert all changes in restore_shell_history() as well as dump_shell_history()?

Copy link
Member

Choose a reason for hiding this comment

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

Yes please.

I'll remove the whole "shell history" feature after we merge this branch.

@@ -200,6 +209,7 @@ restore_pane() {
else
new_session "$session_name" "$window_number" "$dir" "$pane_index"
fi
set_pane_title "$session_name" "$window_number" "$pane_index" "$pane_title"
Copy link
Member

Choose a reason for hiding this comment

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

set_pane_title function effectively contains only a single line/single command, and it's called from just one place.

My first thought was why not just inline it? If you think the function name makes it obvious what the cryptic tmux command does, we can also add a comment.

But, if you want to keep the function, that's fine too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is a remnant of previous commits where I tried to call it when creating panes. I agree to just inline the command.

@bruno-
Copy link
Member

bruno- commented Apr 1, 2022

@Hologos thank you for working on this.

Let me know if I should revert to calling it like in the original PR.

I'd prefer if we inlined that function, but I'm ok if you really want to keep set_pane_title.

I left a couple small comments, but overall this looks good to me.
Just to confirm, you ran this branch on your machine and everything looks good?

Can you please ping me after you make the final tweaks? I'll then test this myself for a couple days, and then we can merge.

@Hologos
Copy link
Contributor Author

Hologos commented Apr 4, 2022

Just to confirm, you ran this branch on your machine and everything looks good?

Can you please ping me after you make the final tweaks? I'll then test this myself for a couple days, and then we can merge.

I've been using it for a week now, resurrected several times + tested it the day I made the changes. We can test it for longer, I am able to rebase it if any changes gets merged into master.

@bruno-
Copy link
Member

bruno- commented Apr 4, 2022

I've been using it for a week now, resurrected several times + tested it the day I made the changes.

Great. I'll start using it after you make the final tweaks. We can merge this after a couple days if all looks good.

@Hologos
Copy link
Contributor Author

Hologos commented Apr 4, 2022

@bruno- I've made requested changes. Thank you for your time.

@bruno-
Copy link
Member

bruno- commented Apr 4, 2022

Thanks.

I pulled this branch locally. Testing basic save/restore looks good. I'll continue working with it for the next couple days.

@bruno-
Copy link
Member

bruno- commented Apr 9, 2022

@Hologos this PR is ready for the merge.

Since this feature contains breaking changes I'll have to release a new major version. That's done by tagging the commit with breaking changes with a new major version v4.0.0. To do that, the feature would ideally be one commit.

So, can you please squash the commits from this branch in a single commit? Thanks

@Hologos Hologos force-pushed the feature/save-pane-title branch from 92042b7 to 1ad109d Compare April 9, 2022 11:53
@Hologos
Copy link
Contributor Author

Hologos commented Apr 9, 2022

@bruno- Hi, I squashed them into a single one. Cheers

@bruno- bruno- merged commit 5b5e6ca into tmux-plugins:master Apr 10, 2022
@ioogithub
Copy link

Thank you for working on this feature. Is this ready to test yet?

@ioogithub
Copy link

I would love to try this. I see version 4.0 was release 16 days ago. Does this version contain the change? I did not see a changelog on the release page.

@Hologos
Copy link
Contributor Author

Hologos commented Apr 26, 2022

@ioogithub The feature is part of v4.0 release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pane titles are not restored
3 participants