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

A fix with revealRange. And allow conditional selection's jump-to-next or extend-to-next for the "forwardNext" pattern. #10

Closed
wants to merge 2 commits into from

Conversation

johnnytemp
Copy link

Summary

A fix: new text selection is not revealed after command selectby.regex
And: Allow conditional selection's jump-to-next or extend-to-next for the "forwardNext" pattern.

For example, support this flow: extends last selection only if last selection is non-empty, and also "forwardNext" give an anchored match at the first search position after "forward" pattern (e.g. "forward" may be a zero-or-more-whitespaces pattern for skipping). Otherwise, make new selection with the "forwardNext" match's range (i.e. a jumped selection).

Two options added for this feature: "forwardNextIncludeExclusivelyWhen" and "forwardNextInsteadExtendsSelectionIfAny".

Example rule:

"selectby.regexes": {
  "Select/extend-to next tuple item": {
    "forward": "\\s*",
    "forwardInclude": false,
    "forwardNext": "\\w+\\s?(,\\s*)?",
    "forwardNextInsteadExtendsSelectionIfAny": true,
    "forwardNextIncludeExclusivelyWhen": "jumped"
  }
}

"Select/extend-to next tuple item" may be used to select single/multiple tuple items for swapping orders, removing or copying.

Run rule result:

(  v original cursor after "1," )
(1,`' 2, 3)  -->  (1, `2, '3)  -->  (1, `2, 3')  -->  (1, 2, 3)    -->  (1, 2, 3)    -->  (1, 2, 3)    -->  (1, 2, 3)
(4, 5, 6)         (4, 5, 6)         (4, 5, 6)         (`4, '5, 6)       (`4, 5, '6)       (`4, 5, 6')       (4, 5, 6)`'

Terminology:

  • `..' : indicate a selection
  • --> : show the `..' selection after a "Select/extend-to next tuple item" command

Options meaning from README.md:

  • forwardNextIncludeExclusivelyWhen: should the matched forwardNext search text be the only selection at the given condition: "always", "jumped" or "never" (default: "never")
  • forwardNextInsteadExtendsSelectionIfAny: should the selection made by a matched forwardNext search text, instead, extends from the selection start when the original+backward+forward include-range is non-empty. The default behavior is to start a new (jumped) selection from the start of the forward match (or the end of current-selection if no match). Note that forwardNextIncludeExclusivelyWhen overrides this behavior (default: false)

… for the "forwardNext" pattern.

For example, support this flow: extends last selection only if last selection is non-empty, and also "forwardNext" give an anchored match at the first search position after "forward" pattern (e.g. "forward" may be a zero-or-more-whitespaces pattern for skipping). Otherwise, make new selection with the "forwardNext" match's range (i.e. a jumped selection).

Two options added for this feature: "forwardNextIncludeExclusivelyWhen" (highest priority, but "forwardNextInclude" must be true for not conflicting); and "forwardNextInsteadExtendsSelectionIfAny" (2nd highest priority). See the code comment, package.json & README.md.
@rioj7
Copy link
Owner

rioj7 commented Aug 18, 2020

Putting 2 different fixes (commits) in 1 pull request is a bad practice. It one of the commits needs to be rejected the other is also rejected.

You don't update the version in package,json so I have to add a commit after the merge and pull before I can publish the extension. And this commit has no relation to the actual changes.

Don't use the construct:

a = b = expression;

but use (better readability and intent)

a = expression;
b = a;

The large comment in the code should be put in the README.

I have read the text and code multiple times.

The main reason to close this pull request is that I have no clue what to fill in the new properties to get what I want. The names are very confusing and the description is unclear to me. I have to maintain the code and doc and have not a clear understanding if it does what the doc says because the doc is confusing. And if all possible use cases are covered.

I have written a version of your feature and published it in v0.8.0.

It is better to propose a feature request and discuss the API before doing the coding/testing work.

@rioj7 rioj7 closed this in 644dc3b Aug 18, 2020
@johnnytemp
Copy link
Author

johnnytemp commented Aug 18, 2020

Thanks for your response. I'm thinking about a feature in the following demo, which is not achievable in your plugin.

demo

The example is for select a tuple item, and extends it one by one if adjacent. Otherwise, find and jump to next such similar simple expression. This example rule aids e.g. function parameter(s) selection.

Note the first selection of "2, " doesn't contain the whitespaces of the "forward" pattern". i.e. don't want to select " 2, " which has leading whitespace(s).

I wrote the implementation so as to make the feature I want and share the idea & feature. It just a way to support it but could further discuss and fine tune.

The rule used is the one mentioned above, which is included here:

"selectby.regexes": {
  "regex1": {  // Select/extend-to next tuple item
    "forward": "\\s*",
    "forwardInclude": false,
    "forwardNext": "\\w+\\s?(,\\s*)?",
    "forwardNextInsteadExtendsSelectionIfAny": true,
    "forwardNextIncludeExclusivelyWhen": "jumped"
  }
}

It is of course not compulsory to accept the feature. But the idea sharing for to what extent the plugin could do may be useful.

Thank you. Happy coding.

@rioj7
Copy link
Owner

rioj7 commented Aug 18, 2020

Maybe you have missed the comment line that I have implemented your proposal in v0.8.0. For the commit see the close comment.

From your example ascii drawing and part of the text I extracted the workings of the feature request.

If you update the extension to v0.8.* and use the following setting

"selectby.regexes": {
  "regex1": {  // Select/extend-to next tuple item
    "forward": "(?=\\w+)",
    "forwardNext": "\\w+(\\s*,\\s*)?",
    "forwardNextExtendSelection" : true
  }
}

You have the exact same behavior.

Next thing to do is the QuickPick list if you run the selectby.regex command.

@johnnytemp
Copy link
Author

Please also have a check to commit 56ef834

Without such fix, a rule with only "forwardNext" pattern, e.g. "regex1": { "forwardNext": "[^{}]+" }, won't scroll the new selection into visible range if the new selection gets out-of-screen.

Thank you.

@johnnytemp
Copy link
Author

Also, thank you for supporting my use case. :-)

rioj7 added a commit that referenced this pull request Oct 8, 2020
@rioj7
Copy link
Owner

rioj7 commented Oct 8, 2020

@johnnytemp I have added 56ef834 in v0.12.1.

That is the reason of my remark to not include 2 different things in 1 pull request.

1 commit is a "bug fix" and 1 commit is a feature request.

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.

2 participants