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

Retain spaces in quoted strings in window options #37

Merged
merged 3 commits into from Apr 28, 2017

Conversation

Projects
None yet
3 participants
@kaushalmodi
Contributor

kaushalmodi commented Apr 26, 2017

Fixes tmux-python/tmuxp#239

With the below in tmux config:

setw -g pane-border-format " #P "

Before the fix:

('pane-border-format', '"', '#P', '"')

After the fix:

('pane-border-format', ' #P ')

@kaushalmodi kaushalmodi referenced this pull request Apr 26, 2017

Closed

tmux freeze fails #239

Retain spaces in quoted strings in window options
Fixes tmux-python/tmuxp#239

With the below in tmux config:

    setw -g pane-border-format " #P "

Before the fix:

    ('pane-border-format', '"', '#P', '"')

After the fix:

    ('pane-border-format', ' #P ')

@tony tony force-pushed the kaushalmodi:fix-tmuxp-issue-239 branch from 2c98c07 to 8900f52 Apr 27, 2017

@tony

This comment has been minimized.

Member

tony commented Apr 27, 2017

I added a test you can work again, but shlex.split isn't storing the expected value.

_____________________________ test_set_show_window_options ______________________________

session = Session($1 libtmux_ziQLip)

    def test_set_show_window_options(session):
        """Set option then Window.show_window_options(key)."""
        window = session.new_window(window_name='test_window')

        window.set_window_option('main-pane-height', 20)
        assert window.show_window_options('main-pane-height') == 20

        window.set_window_option('pane-border-format', ' #P ')
>       assert window.show_window_options('pane-border-format') == [' #P ']
E       assert '"' == [' #P ']
E        +  where '"' = <bound method Window.show_window_options of Window(@2 2:test_window, Session($1 libtmux_ziQLip))>('pane-border-format')
E        +    where <bound method Window.show_window_options of Window(@2 2:test_window, Session($1 libtmux_ziQLip))> = Window(@2 2:test_window, Session($1 libtmux_ziQLip)).show_w
indow_options
@tony

This comment has been minimized.

Member

tony commented Apr 27, 2017

I also rebased it against the latest changes, and dropped python 2.6

@tony

This comment has been minimized.

Member

tony commented Apr 27, 2017

Can you give it a look @kaushalmodi?

@kaushalmodi

This comment has been minimized.

Contributor

kaushalmodi commented Apr 27, 2017

Will have a look.. could probably mean that set_window_option has somehow to be updated to support spaces in quotes.

@kaushalmodi

This comment has been minimized.

Contributor

kaushalmodi commented Apr 27, 2017

I am missing out something obvious here.. how do I run the test_set_show_window_options test locally?

I tried make test in the libtmux repo but the py.test referenced in the Makefile does not exist.

> make test
py.test
make: py.test: Command not found
make: *** [test] Error 127
@tony

This comment has been minimized.

Member

tony commented Apr 27, 2017

Did you try loading the project with tmuxp load path/to/libtmux? You can also manually invoke ./bootstrap_env.py and source into the virtual environment created.

Also you can do pip install requirements/test.txt

@kaushalmodi

This comment has been minimized.

Contributor

kaushalmodi commented Apr 27, 2017

I got this from the .travis.yml and that ran the tests: python3 setup.py test... though fails a different test:

_____________________________________________________________ test_set_height _____________________________________________________________

session = Session($1 libtmux_8kct1jov)

    def test_set_height(session):
        window = session.new_window(window_name='test_set_height')
        window.split_window()
        pane1 = window.attached_pane
        pane1_height = pane1['pane_height']

        pane1.set_height(4)
        assert pane1['pane_height'] != pane1_height
>       assert int(pane1['pane_height']) == 4
E       AssertionError: assert 3 == 4
E        +  where 3 = int('3')

tests/test_pane.py:36: AssertionError
================================================== 1 failed, 77 passed in 16.67 seconds ===================================================
@tony

This comment has been minimized.

Member

tony commented Apr 27, 2017

Rerun it and see if it keeps happening. Could be a fluke.

@kaushalmodi

This comment has been minimized.

Contributor

kaushalmodi commented Apr 27, 2017

Still getting the same error.. will tackle this tomorrow. Thanks for the guidance.

@tony

This comment has been minimized.

Member

tony commented Apr 27, 2017

Yes go ahead, and try to do it via tmuxp load path/to/libtmux and see what happens

@kaushalmodi

This comment has been minimized.

Contributor

kaushalmodi commented Apr 28, 2017

I still see that test_set_height error. But now I see the test_set_show_window_options error too.

image


For my own record, I can run:
(on tcsh)

tmuxp load path/to/libtmux # That installs the virtual env in .venv
source .venv/bin/activate.csh
make test='tests/test_window.py::test_set_show_window_options' 
@kaushalmodi

This comment has been minimized.

Contributor

kaushalmodi commented Apr 28, 2017

I'll need your help to understand this problem.. here is my debug attempt: kaushalmodi@5e94ad8

Running

make test='tests/test_window.py::test_set_show_window_options'

gives

        window.set_window_option('pane-border-format', 'foo')
        assert window.show_window_options('pane-border-format') == '"foo"'

        window.set_window_option('pane-border-format', '#P')
        assert window.show_window_options('pane-border-format') == '"#P"'

        # window.set_window_option('pane-border-format', '#P')
        # assert window.show_window_options('pane-border-format') == ['#P']

        window.set_window_option('pane-border-format', ' #P ')
>       assert window.show_window_options('pane-border-format') == '" #P "'
E       assert '"' == '" #P "'
E         - "
E         + " #P "

tests/test_window.py:187: AssertionError
---------------------------------------------------------- Captured stdout call -----------------------------------------------------------
=== Setting Window Option ===
set_w_opt_dbg: "20"
cmd_dbg1: set-window-option (u'-t$1:2', u'main-pane-height', 20) {}
cmd_dbg2: set-window-option (u'-t$1:2', u'main-pane-height', 20) {}
=== Showing Window Option ===
show_w_opt_dbg: here1
show_w_opt_dbg: here2
show_w_opt_dbg: here4 main-pane-height False
cmd_dbg1: show-window-options (u'main-pane-height',) {}
cmd_dbg2: show-window-options (u'-t', u'@2', u'main-pane-height') {}
=== Setting Window Option ===
set_w_opt_dbg: "foo"
cmd_dbg1: set-window-option (u'-t$1:2', u'pane-border-format', u'foo') {}
cmd_dbg2: set-window-option (u'-t$1:2', u'pane-border-format', u'foo') {}
=== Showing Window Option ===
show_w_opt_dbg: here1
show_w_opt_dbg: here2
show_w_opt_dbg: here4 pane-border-format False
cmd_dbg1: show-window-options (u'pane-border-format',) {}
cmd_dbg2: show-window-options (u'-t', u'@2', u'pane-border-format') {}
=== Setting Window Option ===
set_w_opt_dbg: "#P"
cmd_dbg1: set-window-option (u'-t$1:2', u'pane-border-format', u'#P') {}
cmd_dbg2: set-window-option (u'-t$1:2', u'pane-border-format', u'#P') {}
=== Showing Window Option ===
show_w_opt_dbg: here1
show_w_opt_dbg: here2
show_w_opt_dbg: here4 pane-border-format False
cmd_dbg1: show-window-options (u'pane-border-format',) {}
cmd_dbg2: show-window-options (u'-t', u'@2', u'pane-border-format') {}
=== Setting Window Option ===
set_w_opt_dbg: " #P "
cmd_dbg1: set-window-option (u'-t$1:2', u'pane-border-format', u' #P ') {}
cmd_dbg2: set-window-option (u'-t$1:2', u'pane-border-format', u' #P ') {}
=== Showing Window Option ===
show_w_opt_dbg: here1
show_w_opt_dbg: here2
show_w_opt_dbg: here4 pane-border-format False
cmd_dbg1: show-window-options (u'pane-border-format',) {}
cmd_dbg2: show-window-options (u'-t', u'@2', u'pane-border-format') {}
======================================================== 1 failed in 0.47 seconds =========================================================

Also it probably needs to be

    assert window.show_window_options('pane-border-format') == '" #P "'

instead of

    assert window.show_window_options('pane-border-format') == [' #P ']

?


Update

Going down the rabbit hole.. kaushalmodi@6808f29

@kaushalmodi

This comment has been minimized.

Contributor

kaushalmodi commented Apr 28, 2017

I am at a deadend now.. In the failing case (' #P '), something is calling Session._info, but I cannot trace back any further.

kaushalmodi@1e10452

@tony

This comment has been minimized.

Member

tony commented Apr 28, 2017

I think it's understable that "format" type options such as:

  • automatic-rename-format
  • pane-border-format
  • window-status-current-format
  • window-status-format
  • window-status-seperator

Are wrapped in quotes.

I think that these options should be unquoted on show, and re-quoted on setting.

Part #2 of shlex.split fix
The pane-border-format option was introduced in tmux 2.3, so do not run a test
for that on older versions.

@kaushalmodi kaushalmodi force-pushed the kaushalmodi:fix-tmuxp-issue-239 branch from 6bea5d3 to 6a3a024 Apr 28, 2017

@codecov-io

This comment has been minimized.

codecov-io commented Apr 28, 2017

Codecov Report

Merging #37 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #37      +/-   ##
==========================================
+ Coverage    79.9%   79.92%   +0.02%     
==========================================
  Files           8        8              
  Lines         841      842       +1     
==========================================
+ Hits          672      673       +1     
  Misses        169      169
Impacted Files Coverage Δ
libtmux/window.py 83.01% <100%> (+0.1%) ⬆️

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 197c5f4...6a3a024. Read the comment docs.

@kaushalmodi

This comment has been minimized.

Contributor

kaushalmodi commented Apr 28, 2017

I think that these options should be unquoted on show, and re-quoted on setting.

That is not needed after part 2 of the fix; now comparison is with just ' #P '.

And all the tests are now passing too!


.. though I still get this one failure. I don't get why Travis passes but it fails when I run make test.

_____________________________________________________________ test_set_height _____________________________________________________________

session = Session($1 libtmux_dBF9RL)

    def test_set_height(session):
        window = session.new_window(window_name='test_set_height')
        window.split_window()
        pane1 = window.attached_pane
        pane1_height = pane1['pane_height']

        pane1.set_height(4)
        assert pane1['pane_height'] != pane1_height
>       assert int(pane1['pane_height']) == 4
E       AssertionError: assert 3 == 4
E        +  where 3 = int('3')

tests/test_pane.py:36: AssertionError
================================================== 1 failed, 77 passed in 14.97 seconds ===================================================
@tony

This comment has been minimized.

Member

tony commented Apr 28, 2017

> assert int(pane1['pane_height']) == 4
E AssertionError: assert 3 == 4
E + where 3 = int('3')

@kaushalmodi Can I see your .tmux.conf? How big is your terminal window? Have you tried making the font smaller/terminal window bigger so there are more cells?

window.set_window_option('main-pane-height', 40)
assert window.show_window_options('main-pane-height') == 40
assert window.show_window_options()['main-pane-height'] == 40
if has_gte_version('2.3'):

This comment has been minimized.

@tony

tony Apr 28, 2017

Member

That was just added yesterday, looks like that function is already useful. 😄

@kaushalmodi

This comment has been minimized.

Contributor

kaushalmodi commented Apr 28, 2017

Can I see your .tmux.conf?

here

I didn't realize that make test loaded the config too.

How big is your terminal window?

I think it is big enough
image

Have you tried making the font smaller/terminal window bigger so there are more cells?

No

I got that error to go away if I moved my ~.tmux.conf. I can incrementally comment my config to see what's causing that. Thanks.

@kaushalmodi

This comment has been minimized.

Contributor

kaushalmodi commented Apr 28, 2017

It's caused when the pane border is enabled!

setw -g pane-border-status "bottom"

Update: And that too only when set to "bottom".. hmm

This works fine:

setw -g pane-border-status "top"

@tony

This comment has been minimized.

Member

tony commented Apr 28, 2017

Could you move your .tmux.conf then rerun the tests? No need to kill your main tmux server, since tests run on their own server.

Just trying to isolate if its a config issue or something else.

@kaushalmodi

This comment has been minimized.

Contributor

kaushalmodi commented Apr 28, 2017

Could you move your .tmux.conf then rerun the tests?

That's what I did (see above).

No need to kill your main tmux server, since tests run on their own server.

Yup.

@tony

This comment has been minimized.

Member

tony commented Apr 28, 2017

I thought got that error to go away if I moved my ~.tmux.conf. I can incrementally comment my config to see what's causing that. Thanks.

Just read that, sorry. So did moving the tmux config make it test error go away?

@kaushalmodi

This comment has been minimized.

Contributor

kaushalmodi commented Apr 28, 2017

So did moving the tmux config make it test error go away?

Yes. That fixed the error, and also changing the pane-border-status to "top" instead of "bottom".

@tony

This comment has been minimized.

Member

tony commented Apr 28, 2017

Okay that makes sense. So yes I think we can say it's a config issue, for now, and that this issue is good to merge.

LGTM :shipit:

@tony tony merged commit 6a3a024 into tmux-python:master Apr 28, 2017

3 checks passed

codecov/patch 100% of diff hit (target 79.9%)
Details
codecov/project 79.92% (+0.02%) compared to 197c5f4
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kaushalmodi

This comment has been minimized.

Contributor

kaushalmodi commented Apr 28, 2017

So yes I think we can say it's a config issue

Seems like a tmux issue.

If I am in a tmux window with panes split into top/bottom, display "#{pane_height}" increases by 1 when I switch pane-border-status from "top" to "bottom".

Thanks for merging this PR.

@kaushalmodi kaushalmodi deleted the kaushalmodi:fix-tmuxp-issue-239 branch Apr 28, 2017

@tony

This comment has been minimized.

Member

tony commented Apr 28, 2017

If I am in a tmux window with panes split into top/bottom, display "#{pane_height}" increases by 1 when I switch pane-border-status from "top" to "bottom".

Understood.

Thanks for merging this PR.

Thanks for your time and your contribution. It is now on pypi as libtmux 0.7.1. You are credited in the CHANGES.

I thought there as an issue in tmuxp you had, but I can't find it. I still need to add a test on the tmuxp side.

@kaushalmodi

This comment has been minimized.

Contributor

kaushalmodi commented Apr 28, 2017

I thought there as an issue in tmuxp you had, but I can't find it.

It is this same issue.. tmuxp freeze failed because I had set the pane border status to " #P " (tmux-python/tmuxp#239).

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