8000 Fix select_window() by providing the session ID as argument to -t by Flowdalic · Pull Request #271 · tmux-python/libtmux · GitHub
[go: up one dir, main page]
Skip to content

Fix select_window() by providing the session ID as argument to -t#271

Merged
tony merged 1 commit intotmux-python:masterfrom
Flowdalic:fix-select-window
Jun 14, 2021
Merged

Fix select_window() by providing the session ID as argument to -t#271
tony merged 1 commit intotmux-python:masterfrom
Flowdalic:fix-select-window

Conversation

@Flowdalic
Copy link
Contributor
@Flowdalic Flowdalic commented Mar 19, 2020

Previosly select_window() would not work in nested tmux scenarios, as
there was no tmux session ID in the final tmux command. This is because
select_window() already adds the '-t' argument, hence the check in
cmd() does not add the session ID.

Thanks to Fabian Lesniak and Andreas Fried for pointing this out.

Fixes #161

@tony
Copy link
Member
tony commented Jul 3, 2020 •
edited
Loading

@Flowdalic Hi there, thank you very much and sorry for the delay!

Is it possible you can rebase?

Is it possible for you to create a test as well?

@tony
Copy link
Member
tony commented Aug 16, 2020

@Flowdalic Hi there, can you please rebase this PR? (I want to be sure CI gets it!)

@Flowdalic
Copy link
Contributor Author
Flowdalic commented Aug 16, 2020

@tony rebased

@tony
Copy link
Member
tony commented Aug 16, 2020

@Flowdalic Thank you for the fast follow up!

Would you like to check out the flake8 for this?

(I'm not sure if a variable was lost in rebase or not)

./libtmux/session.py:363:31: F821 undefined name '_session_id'
##[error]Process completed with exit code 1.

@tony tony self-requested a review August 16, 2020 16:45
@codecov
Copy link
codecov bot commented Aug 16, 2020 •
edited
Loading

Codecov Report

Merging #271 (af94b48) into master (9752999) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #271   +/-   ##
=======================================
  Coverage   87.00%   87.00%           
=======================================
  Files          16       16           
  Lines        1516     1516           
=======================================
  Hits         1319     1319           
  Misses        197      197
Impacted Files Coverage Δ
libtmux/session.py 80.95% <100.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 9752999...af94b48. Read the comment docs.

@Flowdalic
Copy link
Contributor Author
Flowdalic commented Aug 16, 2020

Checked and fixed.

@tony
Copy link
Member
tony commented Oct 24, 2020

@Flowdalic Can you rebase, and update the changelog? If possible could you provide a test for #161 and this new change?

@Flowdalic
Copy link
Contributor Author
Flowdalic commented Nov 23, 2020

Rebased and updated the changelog

@tony
Copy link
Member
tony commented Jun 14, 2021

@Flowdalic This one had test issues, if you rebase do they still happen?

@tony
Copy link
Member
tony commented Jun 14, 2021

Sorry for the delay - we really need maintainers. I have a lot on my plate.

@Flowdalic Flowdalic force-pushed the fix-select-window branch from a0a77d2 to 8f5df99 Compare June 14, 2021 16:21
@Flowdalic
Fix select_window() by providing the session ID as argument to -t
af94b48 
Previosly select_window() would not work in nested tmux scenarios, as
there was no tmux session ID in the final tmux command. This is because
select_window() already adds the '-t' argument, hence the check in
cmd() does not add the session ID.

Thanks to Fabian Lesniak and Andreas Fried for pointing this out.

Fixes tmux-python#161
@Flowdalic Flowdalic force-pushed the fix-select-window branch from 8f5df99 to af94b48 Compare June 14, 2021 16:22
@Flowdalic
Copy link
Contributor Author
Flowdalic commented Jun 14, 2021

@Flowdalic This one had test issues, if you rebase do they still happen?

Rebased, you need to approve the workflow run.

@tony
Copy link
Member
tony commented Jun 14, 2021

you need to approve the workflow run.

That's something new. Interesting.

@tony tony merged commit 4a0aa07 into tmux-python:master Jun 14, 2021
tony added a commit that referenced this pull request Jun 14, 2021
@tony
CHANGES: Update for #271
acd5cd7
@Flowdalic Flowdalic deleted the fix-select-window branch June 14, 2021 17:19
@tony
Copy link
Member
tony commented Jun 16, 2021

Eek, this (af94b48) is raising an issue with tmuxp:

https://github.com/tmux-python/tmuxp/runs/2842564761

tests/test_cli.py ...................................................... [ 33%]
...................s..........                                           [ 51%]
tests/test_config.py .................                                   [ 62%]
tests/test_config_teamocil.py ........                                   [ 67%]
tests/test_config_tmuxinator.py ...                                      [ 69%]
tests/test_plugin.py ..........                                          [ 75%]
tests/test_shell.py ..                                                   [ 76%]
tests/test_util.py .....                                                 [ 79%]
tests/test_workspacebuilder.py ..RRRRRFs..........................       [ 98%]
tests/test_workspacefreezer.py .                                         [ 98%]
tests/tests/test_helpers.py ..                                           [100%]

=================================== FAILURES ===================================
____________________________ test_focus_pane_index _____________________________

session = Session($1 libtmux_9wq3r4wt)

    def test_focus_pane_index(session):
        yaml_config = loadfixture('workspacebuilder/focus_and_pane.yaml')
    
        sconfig = kaptan.Kaptan(handler='yaml')
        sconfig = sconfig.import_config(yaml_config).get()
        sconfig = config.expand(sconfig)
        sconfig = config.trickle(sconfig)
    
        builder = WorkspaceBuilder(sconf=sconfig)
    
>       builder.build(session=session)

tests/test_workspacebuilder.py:74: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
tmuxp/workspacebuilder.py:192: in build
    for w, wconf in self.iter_create_windows(session, append):
tmuxp/workspacebuilder.py:286: in iter_create_windows
    w.select_window()
.venv/lib/python3.9/site-packages/libtmux/window.py:351: in select_window
    return self.session.select_window(target)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = Session($1 libtmux_9wq3r4wt), target_window = ('$1:0',)

    def select_window(self, target_window):
        """
        Return :class:`Window` selected via ``$ tmux select-window``.
    
        Parameters
        ----------
        window : str
            ``target_window`` also 'last-window' (``-l``), 'next-window'
            (``-n``), or 'previous-window' (``-p``)
    
        Returns
        -------
        :class:`Window`
    
        Notes
        -----
        .. todo::
    
            assure ``-l``, ``-n``, ``-p`` work.
        """
    
        # Note that we also provide the session ID here, since cmd()
        # will not automatically add it as there is already a '-t'
        # argument provided.
        target = '-t%s:%s' % (self._session_id, target_window)
    
        proc = self.cmd('select-window', target)
    
        if proc.stderr:
>           raise exc.LibTmuxException(proc.stderr)
E           libtmux.exc.LibTmuxException: ["can't find window: ('$1:0',)"]

.venv/lib/python3.9/site-packages/libtmux/session.py:364: LibTmuxException

----------- coverage: platform linux, python 3.9.5-final-0 -----------
Coverage XML written to file coverage.xml

=========================== short test summary info ============================
FAILED tests/test_workspacebuilder.py::test_focus_pane_index - libtmux.exc.Li...
============== 1 failed, 159 passed, 2 skipped, 5 rerun in 30.97s ==============

tony added a commit that referenced this pull request Jun 16, 2021
@tony
Window.select_window(): Update for #271
1a93b57
tony added a commit that referenced this pull request Jun 16, 2021
@tony
Tag v0.10.1
5c6499a 
Hotfix for #271 extended to support Window.select_window()
tony added a commit to tmux-python/tmuxp that referenced this pull request Jun 16, 2021
@tony
Bump libtmux to at least 0.10.1
ce0f7e8 
See also: tmux-python/libtmux#271
@tony
Copy link
Member
tony commented Jun 16, 2021

@Flowdalic I pushed a fix that should get Window.select_window() in line with this change

If you'd like to check it out you can

Now we have a super accurate, session-aware, target passed there as well. (it's a passthrough to the one you updated anyway) :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tony tony Awaiting requested review from tony

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

session.select_window(...) from script in tmux window acts on wrong session

2 participants

@Flowdalic @tony
0