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

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

Merged
merged 1 commit into from
Jun 14, 2021

Conversation

Flowdalic
Copy link
Contributor

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

@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

@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.

@codecov
Copy link

codecov bot commented Aug 16, 2020

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

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

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.

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
Copy link
Contributor Author

@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
@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 added a commit that referenced this pull request Jun 16, 2021
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
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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