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

Prefer tmux splits and tabs over containing terminal #1116

Merged
merged 1 commit into from
Sep 9, 2023

Conversation

EdJoJob
Copy link
Contributor

@EdJoJob EdJoJob commented Feb 13, 2023

Done by having the context for matching tmux be a more restrictive match than that of the terminals.

This was modeled after the matches for the in-browser Slack and Teams apps.

apps/tmux/tmux.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@splondike splondike left a comment

Choose a reason for hiding this comment

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

Don't think this will work in general as implemented.

@splondike
Copy link
Collaborator

@EdJoJob Are you still interested in updating this and merging it in?

@EdJoJob
Copy link
Contributor Author

EdJoJob commented May 28, 2023

I'll work on getting it fixed. Been off GH for a while. Thanks for the patience.

@EdJoJob EdJoJob force-pushed the tmux-actions branch 2 times, most recently from f2bf95a to 767cc9b Compare May 30, 2023 10:01
@EdJoJob EdJoJob requested a review from splondike June 2, 2023 08:23
apps/tmux/tmux.py Outdated Show resolved Hide resolved
@EdJoJob EdJoJob force-pushed the tmux-actions branch 2 times, most recently from fa570fb to ed5badb Compare July 1, 2023 02:29
@EdJoJob EdJoJob requested a review from splondike July 1, 2023 02:29
@splondike
Copy link
Collaborator

Looks good. I reckon just remove the apps.tmux = "app.name: tmux" code block on line 8 of tmux.py since I don't think it's doing anything.

@splondike
Copy link
Collaborator

Not sure if you intended to push a fix for my previous comment in the last force push, but it hasn't changed the thing I was pointing to. (Just want to make sure we're not both waiting on the other person!).

@EdJoJob EdJoJob force-pushed the tmux-actions branch 2 times, most recently from f0c039d to d1decfe Compare August 24, 2023 04:41
@EdJoJob
Copy link
Contributor Author

EdJoJob commented Aug 24, 2023

I'm in the habit of force-pushing a rebase, and then force-pushing the new changes. Clearly this time I just missed doing the second push. Should be up-to-date now

@splondike
Copy link
Collaborator

One more thing I noticed while testing, the context used by the tab commands doesn't have any matchers on it, so it's not very specific. You probably want to add in a line like this so that Tmux's tab implementation overrides the default one:

ctx.matches = "app: tmux"

Done by having the context for matching tmux be a more restrictive match than that of the terminals
Copy link
Collaborator

@splondike splondike left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for your persistence!

@splondike splondike merged commit 84d427d into talonhub:main Sep 9, 2023
2 checks passed
@EdJoJob EdJoJob deleted the tmux-actions branch September 10, 2023 11:26
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.

2 participants