sync terminal buffer's cwd with shell's pwd#6290
Conversation
|
This pull request introduces 1 alert when merging f012d10 into 3d9207a - view on LGTM.com new alerts:
|
d1e00da to
3e387f4
Compare
Codecov Report
@@ Coverage Diff @@
## master #6290 +/- ##
==========================================
- Coverage 89.23% 89.12% -0.12%
==========================================
Files 148 148
Lines 165558 163179 -2379
==========================================
- Hits 147735 145431 -2304
+ Misses 17823 17748 -75
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
991d775 to
65d91d7
Compare
|
I do not see a change to the help files - what will the user experience? Changing directories can have side effects. Perhaps it should be possible to disable this feature? |
Thank you for the suggestion! Change applied. |
|
@brammool glib provides g_filename_from_uri to convert a file URI to a path which is exactly what I need. Since vim (vim-tiny) isn't supposed to depend on glib, now I have two choices,
Which one do you prefer? |
|
@brammool glib provides [g_filename_from_uri](https://github.com/GNOME/glib/blob/ce005e83c64d3d4fbf482f4bfd27a8aefc50b719/glib/gconvert.c#L1621) to convert a file URI to a path which is exactly what I need. Since vim (vim-tiny) isn't supposed to depend on glib, now I have two choices,
- borrow the code of g_filename_from_uri (~100 lines) from glib
- only enable this feature for glib
Which one do you prefer?
In the patch I see syn_cwd() already doing this. Is something missing
here and you want to replace it?
I believe glib is GPL licensed, thus we can't just copy that code. You
can look at the code and rewrite it (leave out parts we don't need,
etc.).
There are many places where glib won't be available, disabling this
feature there would be disappointing.
The uri_decode() I see in the patch mentions it's coming from github.
Did you check the license on that code? It's also possible to rewrite
that code. You need to use "char_u" instead of "unsigned char", for
example. And we don't use "const" much in Vim code.
…--
Eight Megabytes And Continually Swapping.
/// Bram Moolenaar -- Bram@Moolenaar.net -- http://www.Moolenaar.net \\\
/// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
\\\ an exciting new programming language -- http://www.Zimbu.org ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///
|
|
On Wed, Feb 03, 2021 at 07:29:45AM -0800, Bram Moolenaar wrote:
> @brammool glib provides [g_filename_from_uri](https://github.com/GNOME/glib/blob/ce005e83c64d3d4fbf482f4bfd27a8aefc50b719/glib/gconvert.c#L1621) to convert a file URI to a path which is exactly what I need. Since vim (vim-tiny) isn't supposed to depend on glib, now I have two choices,
> - borrow the code of g_filename_from_uri (~100 lines) from glib
> - only enable this feature for glib
>
> Which one do you prefer?
In the patch I see syn_cwd() already doing this. Is something missing
here and you want to replace it?
It now can only deal with one case, i.e., file://path. It can't deal
with g_filename_from_uri file://host/path or other corner cases as shown
in the test suite for g_filename_from_uri [1].
[1] http://web.mit.edu/barnowl/src/glib/glib-2.16.3/tests/uri-test.c
I believe glib is GPL licensed, thus we can't just copy that code. You
can look at the code and rewrite it (leave out parts we don't need,
etc.).
Thanks for reminding me about the license issue.
There are many places where glib won't be available, disabling this
feature there would be disappointing.
I'll re-write uri_decode based on glib's uri_decode then:)
The uri_decode() I see in the patch mentions it's coming from github.
Did you check the license on that code? It's also possible to rewrite
that code. You need to use "char_u" instead of "unsigned char", for
example. And we don't use "const" much in Vim code.
Thanks again for reminding me about the license issue and also the
programming tips for Vim.
…--
Eight Megabytes And Continually Swapping.
/// Bram Moolenaar -- ***@***.*** -- http://www.Moolenaar.net \\\
/// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
\\\ an exciting new programming language -- http://www.Zimbu.org ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///
--
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub:
#6290 (comment)
--
Best regards,
Coiby
|
0d5668e to
8f17e2e
Compare
|
@brammool It turns out we needn't to deal with all the corner cases listed in http://web.mit.edu/barnowl/src/glib/glib-2.16.3/tests/uri-test.c since we can assume the emitted OSC 7 escape sequence is correctly uri-encoded. If there's something wrong, whoever emits the OSC 7 escape sequence should take the responsibility. So the license issues have been resolved. I've also fixed the test failure for FreeBSD. But there is one failed test on macos (huge, clang). Unfortunately, I don't have a Mac system to test on. |
|
The name 'autochdirsh' is hard to read. How about making it 'autoshelldir' ? "char buffer[3] = {src[i+1], src[i+2], 0};" I'm not sure all C90 compilers accept this. syn_cwd() should be renamed to sync_shell_dir(). The test should probably CheckNotBSD now. |
|
On Tue, Mar 02, 2021 at 11:25:58AM -0800, Bram Moolenaar wrote:
The name 'autochdirsh' is hard to read. How about making it 'autoshelldir' ?
Please use the Vim usual C code formatting. E.g. "} else {" is written in three lines.
"char buffer[3] = {src[i+1], src[i+2], 0};" I'm not sure all C90 compilers accept this.
You can use the hexhex2nr() function.
syn_cwd() should be renamed to sync_shell_dir().
Thanks for the suggestions all of which have been applied.
hexhex2nr also reminds me previously the test case didn't cover
part of code in url_decode. So I've added some percent-code
characters (Chinese "中文") to the path in the test.
The test should probably CheckNotBSD now.
Yes, the reason is FreeBSD's sh doesn't play well with OSC 7 escape
sequence. If we use bash for the test in FreeBSD, the test could
pass. Since vim use sh for testing, the test has to be disabled for
FreeBSD.
…
--
You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub:
#6290 (comment)
--
Best regards,
Coiby
|
|
For the change in parse_osc(), we should perhaps always recognize the code, but ignore it when p_asd is not set. |
Thank you for spotting this issue. Yes, to be consistent with OSC 52, it seems we should return 1 as long as osc 7 is recognized regardless of p_asd being set or not. But at least right now, what's returned doesn't matter because string_fragment doesn't check what's returned from on_osc according to Line 76 in fa79be6 static void string_fragment(VTerm *vt, const char *str, size_t len, int final)
{
...
switch(vt->parser.state) {
case OSC:
if(vt->parser.callbacks && vt->parser.callbacks->osc)
(*vt->parser.callbacks->osc)(vt->parser.v.osc.command, frag, vt->parser.cbdata);
break; |
|
You need to adjust the #ifdef for hexhex2nr(). See test failures. |
7f3f12a to
da751fc
Compare
When there is pwd change, the terminal could be notified of the change via `OSC 7;` command. Use this feature to chdir buffer's cwd to the shell's pwd.
pwd The user can choose whether to set cwd to the shell's pwd automatically when FEAT_AUTOSHELLDIR is set. FEAT_AUTOSHELLDIR will be set when FEAT_TERMINAL is set. Don't set cwd to the shell's pwd automatically by default.
8730853 to
71ee4cc
Compare
Thanks. I have used FEAT_AUTOSHELLDIR to enable hexhex2nr to fix this kind of test failures. commit 4355894 ("patch 8.2.2627: no need to check for BSD after checking for not root") removed CheckNotBSD. What can I do to skip the tests for BSD (CheckNotRoot doesn't work)? |
|
> You need to adjust the #ifdef for hexhex2nr(). See test failures.
Thanks. I have used FEAT_AUTOSHELLDIR to enable hexhex2nr to fix this kind of test failures.
commit 4355894 ("patch 8.2.2627: no
need to check for BSD after checking for not root") removed
CheckNotBSD. What can I do to skip the tests for BSD (CheckNotRoot
doesn't work)?
You can restore that check if you need to use it.
I see a test failure on Linux, looks like there is still something to
fix.
…--
"Space is big. Really big. You just won't believe how vastly hugely mind-
bogglingly big it is. I mean, you may think it's a long way down the
road to the chemist, but that's just peanuts to space."
-- Douglas Adams, "The Hitchhiker's Guide to the Galaxy"
/// Bram Moolenaar -- Bram@Moolenaar.net -- http://www.Moolenaar.net \\\
/// \\\
\\\ sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///
|
e1d9089 to
548d970
Compare
548d970 to
d8b24f3
Compare
|
Looks OK now. We don't need a feature for this, just check that the 'autoshelldir' option works. |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
Do you mean there should be an "set noacd" after finishing the test? |
Thanks! It seems I should have mentioned this issue in the doc which according to https://gitlab.gnome.org/GNOME/vte/-/issues/37 is still not resolved. |
Thanks! I'm not sure I understand you correctly. Do you mean FEAT_AUTOSHELLDIR is not needed? |
…t followed Problem: Directory change in a terminal window shell is not followed. Solution: Add the 'autoshelldir' option. (closes vim/vim#6290) vim/vim@8b9abfd
…t followed Problem: Directory change in a terminal window shell is not followed. Solution: Add the 'autoshelldir' option. (closes vim/vim#6290) vim/vim@8b9abfd
…t followed Problem: Directory change in a terminal window shell is not followed. Solution: Add the 'autoshelldir' option. (closes vim/vim#6290) vim/vim@8b9abfd
…t followed Problem: Directory change in a terminal window shell is not followed. Solution: Add the 'autoshelldir' option. (closes vim/vim#6290) vim/vim@8b9abfd
…t followed Problem: Directory change in a terminal window shell is not followed. Solution: Add the 'autoshelldir' option. (closes vim/vim#6290) vim/vim@8b9abfd
…t followed Problem: Directory change in a terminal window shell is not followed. Solution: Add the 'autoshelldir' option. (closes vim/vim#6290) vim/vim@8b9abfd Note that Vim's libvterm has a slightly different API - in particular, the vterm fallbacks take a VTermStringFragment instead of a char*+len, which means the parsing of OSC sequences had to be tweaked a little bit.
…t followed Problem: Directory change in a terminal window shell is not followed. Solution: Add the 'autoshelldir' option. (closes vim/vim#6290) vim/vim@8b9abfd Note that Vim's libvterm has a slightly different API - in particular, the vterm fallbacks take a VTermStringFragment instead of a char*+len, which means the parsing of OSC sequences had to be tweaked a little bit.
In gnome-terminal/xfce4-terminal, it's handy to create a new terminal tab with pwd set to the current terminal's pwd. To let vim's terminal enumerator have this feature, we should set a buffer's cwd to the shell's pwd.
In current version of vim, when a new terminal instance is started, the cwd will be set to
~. On Linux, when there is the change of current working directory, the shell will notify the terminal by theOSC 7;command. Use this feature to chdir buffer's cwd to the shell's working directory.