Skip to content

Remove attached sessions limitation to not detect multiple attached clients #342

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

Merged

Conversation

Timoses
Copy link
Contributor

@Timoses Timoses commented Oct 23, 2021

Technically this is a breaking change. Previously one could expect to detect that only one client is attached to the sessions via this property.

However, since previously Server.attached_sessions was broken (see #180 and PR to fix #341) I do not expect anybody to use the property currently.

Builds on https://github.com/tmux-python/libtmux/pull/341/files (<- Merge first)

@tony
Copy link
Member

tony commented Oct 30, 2021

@Timoses Can you rebase this and #341 so tests run? Thank you!

Timoses added 2 commits October 30, 2021 17:23
Removes limitation of Server.attached_sessions to not being
able to detect multiple attached clients
@Timoses Timoses force-pushed the feature/remove-attached-sessions-limitation branch from 2280a37 to 851856e Compare October 30, 2021 15:24
@Timoses
Copy link
Contributor Author

Timoses commented Oct 30, 2021

@tony : Done ; )

@tony
Copy link
Member

tony commented Oct 30, 2021

@Timoses Thank you!

@codecov
Copy link

codecov bot commented Oct 30, 2021

Codecov Report

Merging #342 (851856e) into master (714261b) will not change coverage.
The diff coverage is 0.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #342   +/-   ##
=======================================
  Coverage   87.06%   87.06%           
=======================================
  Files          16       16           
  Lines        1515     1515           
=======================================
  Hits         1319     1319           
  Misses        196      196           
Impacted Files Coverage Δ
libtmux/server.py 79.19% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 714261b...851856e. Read the comment docs.

@@ -319,8 +319,6 @@ def attached_sessions(self):
"""
Return active :class:`Session` objects.

This will not work where multiple tmux sessions are attached.
Copy link
Member

Choose a reason for hiding this comment

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

Hm, back in history, I'm wondering what I will thinking back then.

tmux-python/tmuxp@1519dee

I guess there really wasn't a good rationale back then.

@tony tony merged commit 399a829 into tmux-python:master Oct 30, 2021
@tony
Copy link
Member

tony commented Oct 30, 2021

@Timoses Thank you!

Would you like me to cut a new release of libtmux for this now?

tmuxp?

@tony
Copy link
Member

tony commented Oct 30, 2021

Also does this look good? I merged #342 directly but preserved #341's commit.

@tony
Copy link
Member

tony commented Oct 30, 2021

@Timoses This is live in 0.10.2 if you'd like to try!

@Timoses
Copy link
Contributor Author

Timoses commented Nov 1, 2021

Also does this look good? I merged #342 directly but preserved #341's commit.

Looks good.

@Timoses This is live in 0.10.2 if you'd like to try!

Thanks, already tried it and it works : ).

What I noticed though is that using more tmux introspection causes more "lag". As I am using libtmux in a real-time application I am now circumventing calling it too often: https://codeberg.org/timoses/caster-timoses/src/branch/master/caster_timoses/tmux/__init__.py#L19-L26

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