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

WIP. Empty window titles cause errors. #183 #189

Closed
wants to merge 4 commits into from

Conversation

BlitzKraft
Copy link

Added functions for testing empty titles and a config
window_title_empty.yaml.

Added functions for testing empty titles and a config
window_title_empty.yaml.
@BlitzKraft
Copy link
Author

I need some pointers on getting the window titles right. When the title is not set or empty, the title gets set to the shell (bash, in my case).

@tony
Copy link
Member

tony commented Dec 22, 2016

can you rebase this against the latest codebase?

@BlitzKraft
Copy link
Author

Will do.

@tony tony mentioned this pull request Oct 7, 2017
@tony
Copy link
Member

tony commented Mar 4, 2018

Sorry for the late response, I am having issues keeping up with the project. I'm seeking maintainers @ #290. If you (or someone you know) may be interested in helping maintain libtmux or tmuxp, feel free to contact me by email or in the #290 issue.

In the mean time, if you're still interested in this PR, can you rebase it via git pull --rebase origin master and git push --force?

@BlitzKraft
Copy link
Author

I'll rebase and see how it goes. I am interested in maintaining this project.

@tony
Copy link
Member

tony commented Mar 4, 2018

If you're not able to rebase immediately, that's also fine, but if you do, ping me here. GitHub has an issue notifying me when force pushes happen (even when I'm watching).

If you can pitch in time to help maintain, can you drop a message in #290? In addition to registering your interest and how much time you can commit, it would be encouraging for more people to come forward, too. My hope is to always have at least one person on duty and a quick turnaround time on issues/PRs. I'm wondering if we need a couple people? Other than that, ATM I'm working on some issues to streamline contributions/issues/debugging.

I'm also weighing creating an organization for tmuxp/libtmux.

@tony
Copy link
Member

tony commented Mar 4, 2018

Also, I didn't mention this, but after a git pull --rebase origin master, you will have to git push --force the branch. That's OK in this case, since it's your branch on your fork, and a pull request.

@tony
Copy link
Member

tony commented Jul 9, 2020

Can you give this a rebase? @BlitzKraft

@tony
Copy link
Member

tony commented Aug 16, 2020

@BlitzKraft Can you rebase? (giving you a ping)

If I don't get a response, I may close the PR (I think?) - but you can still create a new PR if you / someone else regains interest in it

@BlitzKraft
Copy link
Author

Got it, I will rebase.

@tony
Copy link
Member

tony commented Aug 16, 2020

Nice! That was quick. Welcome back @BlitzKraft

BK Bolisetty and others added 2 commits August 16, 2020 16:29
@BlitzKraft
Copy link
Author

Seems I might have messed up. Not sure how to fix. What do you suggest?

@tony
Copy link
Member

tony commented Aug 16, 2020

git reflog can help you

Since you probably have the old ref : https://stackoverflow.com/a/10099285/1396928

You'd git reset --hard <ref>

@BlitzKraft Would that help?

@codecov
Copy link

codecov bot commented Aug 16, 2020

Codecov Report

Merging #189 into master will decrease coverage by 3.18%.
The diff coverage is 42.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #189      +/-   ##
==========================================
- Coverage   77.08%   73.89%   -3.19%     
==========================================
  Files           6        6              
  Lines         829      835       +6     
  Branches      238      241       +3     
==========================================
- Hits          639      617      -22     
- Misses        131      160      +29     
+ Partials       59       58       -1     
Impacted Files Coverage Δ
tmuxp/workspacebuilder.py 83.43% <0.00%> (-5.06%) ⬇️
tmuxp/config.py 87.09% <100.00%> (+0.91%) ⬆️
tmuxp/cli.py 60.00% <0.00%> (-6.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 bdb2154...7316fdd. Read the comment docs.

@BlitzKraft
Copy link
Author

I think I got it. Did not reflog. I think this commit should be squashed. And I need to run the tests locally first. I keep forgetting to do that.

@tony
Copy link
Member

tony commented Aug 16, 2020

@BlitzKraft Nice

Are the new gh action tests helpful? They should detect if other functionality was affected / any tests added

@BlitzKraft
Copy link
Author

I think they are helpful. I haven't used them very much. I need to get a hang of that.

@BlitzKraft
Copy link
Author

make test encounters this: https://dpaste.org/zGVC

Should I just ignore that?

@tony
Copy link
Member

tony commented Aug 17, 2020

@BlitzKraft poetry install -E "docs test coverage lint format"

Does doing this and rerunning change anything?

pip show pytest

Are you using poetry for this? e.g. poetry shell, poetry run py.test ?

@tony
Copy link
Member

tony commented Aug 17, 2020

(it's not a requirement but gives more piece of mind you have only the locked packages installed)

@BlitzKraft
Copy link
Author

I was missing some dependencies. Installing them using poetry, like you mentioned, I was able to run all the tests.

There is still one failing test. test_window_title - is there a separate test for empty titles?

@tony tony self-requested a review August 17, 2020 13:37
Copy link
Member

@tony tony left a comment

Choose a reason for hiding this comment

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

@BlitzKraft see if this helps with the test

Comment on lines +387 to +388
if w.name == '':
empty_window_title = True
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if w.name == '':
empty_window_title = True
empty_window_title = w.name == ''

Assures the variable is always declared

Should fix https://github.com/tmux-python/tmuxp/pull/189/checks?check_run_id=991200510#step:15:372:

                wconf['panes'].append(pconf)
>               if empty_window_title:
E               UnboundLocalError: local variable 'empty_window_title' referenced before assignment

tmuxp/workspacebuilder.py:416: UnboundLocalError

Copy link
Author

Choose a reason for hiding this comment

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

That resolves the unbound variable. I am adding that change.

@@ -209,7 +209,10 @@ def expand(sconf, cwd=None, parent=None):
if 'session_name' in sconf:
sconf['session_name'] = expandshell(sconf['session_name'])
if 'window_name' in sconf:
sconf['window_name'] = expandshell(sconf['window_name'])
if not (sconf['window_name'] is None):
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if not (sconf['window_name'] is None):
if sconf['window_name'] is not None:

Copy link
Author

Choose a reason for hiding this comment

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

This does not change the outcome of the test_window_title. As expected. The if statements - before and after the suggested changes - are equivalent. Were you expecting something different?

Copy link
Author

Choose a reason for hiding this comment

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

Found something. window_name is being set to tmux. Hence the failing assertion. Now, need to dig where this is happening.

@tony
Copy link
Member

tony commented Jan 9, 2021

@BlitzKraft Have you looked at the tests yet? (It says the tests failed)

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
0 out of 2 committers have signed the CLA.

❌ BK Bolisetty
❌ BlitzKraft


BK Bolisetty seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@tony
Copy link
Member

tony commented Jan 29, 2022

@BlitzKraft

If you want to continue with this contribution, please make a new pull request based off the newest master.

I am going to close this pull request for now as a significant amount of time has passed.

Also I am at fault for letting the wait go too long in between. Sorry about that.

Window titles

This may behave differently depending on tmux's config itself, via automatic-rename (e.g. setw -g automatic-rename)

A future PR would need to account for that

@tony tony closed this Jan 29, 2022
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.

None yet

3 participants