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

Inconsistent toggle when no text selected #1

Closed
pdonias opened this issue May 10, 2017 · 4 comments
Closed

Inconsistent toggle when no text selected #1

pdonias opened this issue May 10, 2017 · 4 comments

Comments

@pdonias
Copy link

pdonias commented May 10, 2017

There is a weird behaviour regarding which part of the text is "toggled" when no text is selected:
toggler

  • Line 1: first, the single quote after the cursor is toggled to double quote. Then the double quote before the cursor is toggled
  • Line 2: the single quote before the cursor is toggled back and forth
  • Line 3: the 0/1 after the cursor is toggled

This is probably related to the order in which each string is written in the config file :)

@HiDeoo
Copy link
Owner

HiDeoo commented May 10, 2017

Thanks for your feedback.

When no text is selected, we're parsing the whole text around the cursor (exclusive) to find the first possible match possible.

Line 1: The configuration order is the following ["'", "\"", "`"] so the order is single quote => double quote => backtick.

  • " ' " will match the single quote first (as it's declared first in the configuration) and toggle it to a double quote.
  • " " " will match the first double quote and toggle it to a backtick.

Line 2: The configuration order is the same.

  • " " will match the first double quote and change it to a simple quote.
  • ' " will match the first single quote first (as it's declared first in the configuration) and toggle it to a double quote.

When finding a match to toggle without a text selection, if the cursor is in or around the match, we're not making any assumption or looking if the cursor is before or after the match for example to force toggling a match after the cursor.

Without being opinionated on what should be toggled first (what's before or after the cursor), I don't really know how this behavior could be avoided.

What would you expect to happen regarding line 1 & 2? Always toggle first what's after the cursor if possible? Always toggle first what's before the cursor if possible?

Line 3: This is only a matter of configuration order as ["0", "1"] is defined way before ["'", "\"", "`"] so we'll always stop on the number and toggle it.

Regarding the part only specific to configuration order, the main reason I wrote this plugin is to being able to modify the configuration quickly without requiring an update of the plugin and let people have their own custom configuration. So if some people prefer to have '/"/` matched & toggled before 0/1, they can modify the configuration order.

@pdonias
Copy link
Author

pdonias commented May 10, 2017

First of all, I forgot to mention I removed the backtick in my configuration, so the " switching back to ' is the normal behaviour for me, sorry about that :)

Then, regarding the behaviour I would expect, I'm not really sure about it. One thing I'm pretty sure about though, is that if I spam the toggle key, it should cycle and come back to the original state eventually (which is not the behaviour I had in line 1).

Sure, we can choose between "before the cursor" or "after the cursor" but that wouldn't even be enough.
Let's say we choose "before", if I have such a configuration:

["abc", "def"],
["a", "z"]

and such a text: zbc, with the cursor between z and b, then it will switch like this:
zbcabcdefabc→ ...
But this is not a cycle :(

And it should also prevent this kind of behaviour:
installuninstallununinstall→ ...

So this probably needs more thinking. I'll try to use it a bit more to think of what could be a good and consistent behaviour.

@HiDeoo
Copy link
Owner

HiDeoo commented May 10, 2017

The idea of an 'original state' is something definitely interesting.

Regarding the installuninstallununinstall issue, this is something I addressed in v0.3.0 but forgot to modify some configuration orders required by this change.

I just pushed some modification to defaults.json to avoid this issue.

The new behavior with these changes is closer to what would be expected:

toggle

Before pushing a release with just this change, I'll try some experiments with your idea of 'original state', maybe based on if the cursor position changed between 2 invocations of the plugin.

But like you said this indeed needs more thinking and I'll be happy to add any modification if we can come up with something.

@HiDeoo
Copy link
Owner

HiDeoo commented Jun 16, 2017

I'm gonna close this issue at the moment, I haven't come up with something really relevant regarding this 'original state' idea that would provide a way better behaviour over the current one.

I would be happy to reopen / review any PR if someone can comes up with something new in that regard.

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

No branches or pull requests

2 participants