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

Workaround for issue 1329 #1457

Closed
wants to merge 1 commit into from

Conversation

paschalis-mpeis
Copy link

This is for issue #1329 . Previous workaround (#1355 ) to this wasn't accepted.

It is just @dhelonious solution based on @SolidTux comments on the latest commit (45376f1).
I've tested it a few days and this seems to be solving all the issues I've had.
The only thing I am not sure, is that it has the cursor, after completion, stays at the end of the last character inside the brackets. If this is the expected behaviour it can be merged.

@r-stein
Copy link
Member

r-stein commented Jan 23, 2020

Have you also tried this with the normal sublime built-in command mode?

@dhelonious
Copy link

dhelonious commented Jan 24, 2020

EDIT: Simplified expression with tuple as suggested by @gerardroche below.

@Paschalis Thank you for implementing my workaround and for this PR.

However, there remain some issues with this solution that I wanted to look into for some time. For example, if you escape the quick panel, I would want an empty set of brackets to be inserted. However, in my original workaround the order of brackets is messed up.

These can be solved by a minor change: Remove the opening brackets from the string in line 501, i. e.

if(cmd_mode and view.substr(sel.end()+1).strip() in ('', '\x00')
   and not view.substr(sel.end()) in tuple('.,;?!\'")]}')):

Then the edge case I mentioned above is resolved correctly. The inclusion of the closing MATCH_CHARS only seems to be the better choice.

There also some spacing issues with your code, though only because of consistency. I would suggest to add a space in front of the lines 499 and 504, respectively.

@Paschalis I think that the positioning of the cursor after the insert is somewhat correct, as it resembles the behaviour of NeoVintagous and LaTeXTools on inserting ordinary brackets (the cursor remains on the opening brace, the character before the closing brace). Though, this could be resolved by adding something like

if view.substr(new_end) in self.MATCH_CHARS.values():
    new_end += 1

before line 511, this would break consistency in my opinion.

@r-stein I think you mean the command mode of the Vintage package? I briefly tested this workaround with the Vintage command mode and found no issues. As both of these packages try to emulate Vim behaviour, I would not expect any differences here.

view.insert(edit, sel.end(), value)
# Workaround for Issue 1329 (based on @dhelonious)
if(cmd_mode and view.substr(sel.end()+1).strip() in ('', '\x00')
and not view.substr(sel.end()) in '.|,|;|?|!|\'|"|(|)|[|]|{|}'.split('|')):

Choose a reason for hiding this comment

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

Rather than

and not view.substr(sel.end()) in '.|,|;|?|!|\'|"|(|)|[|]|{|}'.split('|')):

You can do

and not view.substr(sel.end()) in tuple('.,;?!\'"()[]{}'):

@johnlb
Copy link

johnlb commented Mar 10, 2020

FYI, I loaded up this branch and can confirm the reverse-bracket problem, but also noticed a new one: when the ref is followed by a colon (as in \ref{}:), the brackets (and whatever fills them) appears on the other side of the colon (\ref:{example}).

@dnlbauer
Copy link

Hi.
This issue is driving me nuts :)

Is this PR still worked on? It hasn't seen a lot of activity in the last few month. If everyone is fine with that, I will pick it up and see if I can fix the remaining concerns raised here to make it mergeable.

Am I correct that there are only these points that need to be addressed before a merge:

  1. Replace the str.split implementation with a tuple
  2. Make the cursor end inside the brackets if the dialogue is dismissed following @dhelonious solution
  3. Keep consistency with ordinary brackets and vim mode
  4. consistent whitespacing
  5. (Hopefully finding a workaround for @johnlb problem if I can reproduce it)

@dhelonious
Copy link

At least for me, @gerardroche provided some suitable solutions for this issue.

@dnlbauer
Copy link

you are right. thanks for pointing me to the workaround!

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

6 participants