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 cosmetic/usability bug with marking paths. #123

Closed
wants to merge 4 commits into from

Conversation

lifepillar
Copy link
Contributor

This commit addresses #76
and more specifically, this comment: #76 (comment).
It likely needs some cleanup, but it basically works.

A few notes:

  • Syntax rules for DirvishFullPath and DirvishArg must follow the other
    rules, otherwise they may be overriden (see :h syn-priority).
  • I am not sure whether \v has any effect in syntax rule, as the manual
    says that syntax patterns are always magic (:h syn-pattern).
  • map() has been replaced by a for loop: this way the code is cleaner
    (IMO) and the resulting syntax rules shorter.
  • I am not sure what the place for the condition testing for
    b:current_syntax is. That might need to be changed.

This commit addresses justinmk#76
and more specifically, this comment: justinmk#76 (comment).
It likely needs some cleanup, but it basically works.

A few notes:

- Syntax rules for DirvishFullPath and DirvishArg must follow the other
  rules, otherwise they may be overriden (see :h syn-priority).
- I am not sure whether \v has any effect in syntax rule, as the manual
  says that syntax patterns are always magic (:h syn-pattern).
- map() has been replaced by a for loop: this way the code is cleaner
  (IMO) and the resulting syntax rules shorter.
- I am not sure what the place for the condition testing for
  b:current_syntax is. That might need to be changed.
@lifepillar
Copy link
Contributor Author

There likely should be a syntax clear DirvishFullPath DirvishArg before defining those highlight groups, to avoid duplication of rules.

exe 'syntax match DirvishPathTail =\v[^\'.s:sep.']+\'.s:sep.'$='
exe 'syntax match DirvishSuffix =[^\'.s:sep.']*\%('.join(map(split(&suffixes, ','), s:escape), '\|') . '\)$='

for p in argv()
Copy link
Owner

Choose a reason for hiding this comment

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

I think this will set g:p, better use s:p.

@@ -5,16 +5,18 @@ endif
let s:sep = exists('+shellslash') && !&shellslash ? '\' : '/'
let s:escape = 'substitute(escape(v:val, ".$~"), "*", ".*", "g")'

" Define (again). Other windows may need the old definitions ...
Copy link
Owner

Choose a reason for hiding this comment

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

comment is still relevant, so that I don't repeat the mistake :)

@justinmk
Copy link
Owner

@lifepillar Thank you! This was very bothersome. I think I can finally tag 1.0 after this.

It seems to work. LMK if you're at a stopping point.

@justinmk
Copy link
Owner

justinmk commented Jun 12, 2018

I am not sure whether \v has any effect in syntax rule, as the manual says that syntax patterns are always magic (:h syn-pattern).

Yeah that's left over from before I knew better. I think \v should not be used.

Syntax rules for DirvishFullPath and DirvishArg must follow the other rules, otherwise they may be overriden (see :h syn-priority).

Good call, should put that comment in the code.

I am not sure what the place for the condition testing for b:current_syntax is. That might need to be changed.

Yes that was to avoid duplicate rules.

@lifepillar
Copy link
Contributor Author

I've left \v and the current syntax check untouched, 'cause they are not relevant to this PR.

@justinmk
Copy link
Owner

There likely should be a syntax clear DirvishFullPath DirvishArg before defining those highlight groups, to avoid duplication of rules.

Can't do that (AFAIK) because other windows (showing the same Dirvish buffer) may have their own local arglists which used the existing buffer-local :syntax match rules. That's what the "Define (again) ..." comment refers to.

However, it's not a big problem, a Refresh clears the redundant syntax definitions for example.

I am not sure what the place for the condition testing for b:current_syntax is.

It's to avoid redundant definitions for DirvishPathHead etc. I changed it as follows:

" Define once (per buffer).
if !exists('b:current_syntax')
  exe 'syntax match DirvishPathHead =\v.*\'.s:sep.'\ze[^\'.s:sep.']+\'.s:sep.'?$= conceal'
  exe 'syntax match DirvishPathTail =\v[^\'.s:sep.']+\'.s:sep.'$='
  exe 'syntax match DirvishSuffix   =[^\'.s:sep.']*\%('.join(map(split(&suffixes, ','), s:escape), '\|') . '\)$='
endif

@kristijanhusak
Copy link
Contributor

@justinmk i have one question kinda related to this. Would you agree on support for custom columns/flags/whatever you want to call it, per single path, something like this https://github.com/Xuyuanp/nerdtree-git-plugin? as @lifepillar noted, it's really hard to work with these paths and concealing. In my git plugin for divish i somehow managed to conceal these paths, but it's really messy. It would simplify managing these things, but i assume it would complicate handling paths for other things.

@justinmk
Copy link
Owner

justinmk commented Nov 14, 2018

@kristijanhusak Left-side column would be possible after #70 is addressed, because that will entail adding some whitespace for indentation. Though I don't plan to work on it anytime soon.

Right-side "column" can be achieved with "virtual text" which is supported by Nvim 0.3.2 (prerelease), and I assume Vim will eventually have a similar feature. Anything else will be a mess.

If there's some way to get left-side column support before #70 , without a ton of code, I'd be in favor.

@justinmk justinmk closed this in 5c98d10 Nov 15, 2018
justinmk added a commit that referenced this pull request Nov 15, 2018
For voodoo reasons it may not be best practice.

ref #123
justinmk added a commit that referenced this pull request Nov 15, 2018
Match DirvishArg to full path and mark it with
"contains=DirvishPathHead".

ref #123
@justinmk
Copy link
Owner

Merged, thanks @lifepillar . I was also able to simplify it in 9c67fa7.

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.

3 participants