added screen segments #24
Conversation
|
Nice, it seems to work fine, just two quick question:
|
I'd be fine with that. I often use PIDs as mnemonics for short-term multitasking, but pty/pts would work just as well. |
|
Both of your points have been addressed, so I think it's good to go. |
tests/fltest_screen.fish
Outdated
| set -gx STY 76230.test123 | ||
| __fishline_test STY | ||
|
|
||
| set -gx 69359.ttys001.Providence |
There was a problem hiding this comment.
This result will not work (requires a variable name)
tests/fltest_screen.fish
Outdated
|
|
||
| echo "Context: STY var is set to 'fishline_test'" | ||
| set -gx STY 76230.test123 | ||
| __fishline_test STY |
There was a problem hiding this comment.
You are calling the STY segment when your segment is called SCREEN
segments/__flseg_screen.fish
Outdated
|
|
||
| if set -q STY | ||
| __fishline_segment $FLCLR_SCREEN_BG $FLCLR_SCREEN_FG | ||
| printf $FLSYM_SCREEN" "(echo $STY |cut -d'.' -f2-) |
There was a problem hiding this comment.
Missing a space after the | (just for clarity)
themes/default_symbols.fish
Outdated
| @@ -36,6 +36,9 @@ set FLSYM_VFISH "\u2635" | |||
| # Symbol for CONDA segment | |||
| set FLSYM_CONDA "\U223F" | |||
|
|
|||
| # Symbol for SCREEN segment | |||
| set FLSYM_SCREEN "\U239A" | |||
|
|
|||
There was a problem hiding this comment.
Can we use the lowercase \u as a prefix to stay consistent with the other symbols as they are 4 digit UTF-8 escape sentence (\uXXX vs \UXXXXXXXX which is 8 digit escape).
PS: did not notice for the FLSYM_CONDA that it was also in the \U form.
|
|
||
| function fltest_screen | ||
|
|
||
| echo "Context: STY var is set to 'fishline_test'" |
There was a problem hiding this comment.
A context line should appear before each call to __fishline_test, here:
- STY var is set to '76230.test123' (custom session name)
- STY var is set to '69359.ttys001.Providence' (automated OSX style name)
- STY var is set to '20424.pts-0.userctl' (automated Linux style name)
There was a problem hiding this comment.
hah, thanks! I actually have no idea what i'm doing when it comes to writing tests for this kind of thing, but that makes sense. I'll correct.
There was a problem hiding this comment.
That's a part of fishline that is currently broken / hard to explain. Its just a custom script that shows you all segment in their context. It broke in fish 2.6 due to a bug (which is fixed on master) (see fish-shell/fish-shell#4206). The thing is you should be able to test a segment by running test/run.fish screen and it will run this script in an enclosed fish session with fishline sourced from the git repository you are running test/run.fish from.
There was a problem hiding this comment.
Makes sense. Probably worth documenting that somewhere (CONTRIBUTING.md?) but that's a separate issue.
There was a problem hiding this comment.
Exactly, I need to update the documentation badly about all the internals and way to you can create and extend fishline.
A lot of the work done for Fishline 3.0 missed documentation and I wanted to wait before releasing it, but in the end I preferred to have it with out-of-date documentation as it was my vision of what fishline should have been from the get go and a lot of the people that are using fishline needed some of the change of v3.0.
I am taking note about that though, to add some note about the test runner.
There was a problem hiding this comment.
Sounds good, and all too familiar of a maintainer woe! FWIW, I ran the test locally (ubuntu fish2.6 build) and it crashed out with a segfault, but I'm pretty sure that's on fish itself and not these tests.
There was a problem hiding this comment.
Yes thats the issue I linked in my other comment. Read broke in 2.6 and is no fix on master (but there is still another issue with printf on the latest master that wasnt there previously [starting a format string with "--" cause printf to try to use the full string as an argument now]).
This scripts tends to show me quite well if something "breaks" with the read & printf builtin.
|
@bmcfee, I regularly use named screen thats why I am not really sure of the utility of the PID. And I do find that the automated name when I am not using a named screen are already quite useful. If you'd like to keep them why not, but I might just add a lot of bulk to the segment itself. |
|
Will be merging as is and updates the test file myself before releasing v3.1.0. This way I can release this new version tonight with the little fixes I've added to the tests. Thanks a lot for this PR though, really enjoy seeing fishline be updated by other devs 👍 |
|
Thanks! |
This PR adds a segment for screen sessions, as accessed via the
$STYvariable.