Bug: keymap "context" keys `preceding_text` and `following_text` include selection

[faelin]

Description

When configuring a regex_match or not_regex_match operator for a key binding "context", the preceding_text and following_text keys both include the entire selected text (i.e. the value of the text key). This means that RegEx matches that look for the beginning or the end of the selection via <pattern>$ (in _precedingtext) or ^<pattern> (in _followingtext) fail if there is a selection.

Steps to reproduce

1. Create a key binding with preceding_text and/or following_text context keys:

   {
   "keys": ["shift+super+up"],
   "command": "insert_snippet",
   "args": {"contents": "***${0:$SELECTION}***"},
   "context":
   [
   { "key": "selection_empty", "operator": "not_equal", "operand": true },
   { "key": "preceding_text", "operator": "regex_match", "operand": ".*=>$", "match_all": true },
   { "key": "following_text", "operator": "regex_match", "operand": "^<=.*", "match_all": true },
   ]
   }

2. Type the following into an editor: =>example<=
3. Select the word "example" from the above text and enter the shortcut: ⇧⌘↑

Expected behavior

The text in the editor should read: =>***example***<=.

Actual behavior

The text has note changed, and still reads: =>example<=.

Proof of Bug

1. Update the previous keybinding to read as follows

   {
   "keys": ["shift+super+up"],
   "command": "insert_snippet",
   "args": {"contents": "***${0:$SELECTION}***"},
   "context":
   [
   { "key": "selection_empty", "operator": "not_equal", "operand": true },
   { "key": "preceding_text", "operator": "regex_match", "operand": ".*=>example$", "match_all": true },
   { "key": "following_text", "operator": "regex_match", "operand": "^example<=.*", "match_all": true },
   ]
   }

2. Select the word "example" from the above text and enter the shortcut: ⇧⌘↑
3. The text now reads: =>***example***<=

Environment

• Build: 3211
• Operating system and version: macOS 10.15.7

[pepkin88]

I will add that it was known to some people for quite some time, and developers could write their plugins relying on this behavior. Although in the official docs it is stated, that preceding_text is the text before the selection, there is an unofficial documentation, where that behavior is correctly described: “preceding_text - Test against the text on the line up to and including the selection”. The oldest commit on that docs informing about that behavior is from May of 2017: https://github.com/guillermooo/sublime-undocs/commit/69c1a05e4b6367b6b4a1c7e2703fec709cdbf39b.

I'd opt for not changing this behavior, as it would mean a breaking change. Instead, I think it'd better to add contexts without including the selected text. Name candidates for the fixed preceding_text context:

• strict_preceding_text
• preceding_text_strict
• real_preceding_text
• preceding_text_no_selection
• preceding_text_without_selection
• preceding_text_excluding_selection
• something else

Analogously for following_text.

[pepkin88]

As for the use case, yesterday I tried to add a key binding activating only when there were selected whole lines, and only then. Like the state after hitting primary+l (that is command+l on Mac, ctrl+l on Windows).

At first I was confused how those context worked, something seemed wrong. Later I discovered, that those contexts include the selected text, which I find impractical. After all, we have a context named text for matching just the selected text.

In the end I couldn't find any way to make my key binding work with such behavior. But I suspect it is possible to to this with the fixed one.

[faelin]

@pepkin88 good point about the backwards compatibility! Also, the context for my use was removing surrounding characters for custom "surround selection with symbols" commands, such as adding and removing *bold* tags or _italic_ tags in Markdown.

I think you're onto something with just creating new contexts. Additional candidates for naming the new scopes:

• look_behind / look_ahead (matching the Regex standards)
• text_before_cursor / text_after_cursor

However, I think your suggestion of preceding_text_strict / following_text_strict is ultimately the best fit.

[eugenesvk]

I'd opt for not changing this behavior, as it would mean a breaking change. Instead, I think it'd better to add contexts without including the selected text.

Counterpoint: without a breaking change users have to waste a lot of time to learn that the obvious (from the name) and documented to match the obvious, behavior is actually incorrect; and given enough time the amount of future users' pain will dwarf any amount of transition pain of the current users that depend on this bug (for compatibility it's better to add an appropriately named preceding_text_and_selection and selection_and_following_text so you can do a simple search&replace) Such ugly mistakes should be fixed, not kept around forever

[pepkin88]

If it was just for the end users, sure, put it in the changelog message and that's it. But I'm afraid that it will break some packages. Packages can include their own key bindings and can use those context directives. And it is harder to fix existing packages, some of them are not actively maintained

[eugenesvk]

But you're also making it harder to fix existing packages by keeping the bug. Case in point: I found out about this bug after wasting a lot of time trying to update an existing package (and also realized just now why some of the other packages' keybinds weren't working properly, as you mentioned yourself, some of the conditions are impossible to write if you include the selected text)! And it's a challenge to discover solutions since the name is obvious, so you keep wasting time trying to fix your non-existing regex mistakes before looking for another way out So even if the docs are fixed, it'd still be a waste, and a permanent one at that! That's the key factor factor

If it was just for the end users, sure, put it in the changelog message and that's it.

Well, but this is the type of bug that an end user can fix by copying&renaming the keybinds, and in some dream world you could even have a Sublime message when a package with an update date before the bug fix is using these APIs, warning that the API name might have changed, so if something is not working, there is a way to fix it

[pepkin88]

You bring a fair point, that some packages that are using those context directives might have assumed different behavior, than the actual, buggy one. In that case indeed, maybe it is better to fix it in place, keeping the name.

I don't know, how many of such packages exist. I also don't know how many packages rely on the current behavior.

And it's a challenge to discover solutions since the name is obvious, so you keep wasting time trying to fix your non-existing regex mistakes before looking for another way out

Yes, that is a challenge. In my case, I started from copying some rule from a different source, then realized, it does not work as I presumed, then looked up in the docs, but the docs were saying, that everything is fine. Only then I found an alternative source saying, that the implementation is different. And then I was looking for an existing issue on the Sublime forum and here. If the complete information was in the docs, including the info about a fixed directive, I'd have stopped there. But with your solution implemented, I'd have stopped even sooner. So personally I like your proposal.

I just wanted to remind about potential consequences, because there might be some packages relying on the current behavior, and users relying on those packages. However, I don't know the scale of such consequences. Maybe it's negligible, maybe it's worth fixing the behavior directly. I don't know.

[faelin]

Such ugly mistakes should be fixed, not kept around forever

Generally this is true, however the definition of Semantic Versioning would require that breaking changes be relegated to MAJOR versions, i.e. ST 5.x.x

The definition of semantic versioning probably shouldn't be enough of a reason to limit such a change, but maybe the best solution is to add additional scopes to accommodate the desired functionality now (i.e. the changes described here: strict_preceding_text and strict_following_text), and then upgrade the API to have expected functionality (the described changes) with a new context-key for the currently defined scope (e.g. selection_with_preceding_text, selection_with_following_text) in version 5.x.x

[eugenesvk]

Generally this is true, however the definition of Semantic Versioning would require that breaking changes be relegated to MAJOR versions, i.e. ST 5.x.x

But ST doesn't follow SemVer, 4 is a marketing version, no? Also, the "non-changes" are also "breaking", a plugin that added this API isn't working

Your solution requires double the maintenance work — add the new weirdly-named APIs and then remove them later when the proper names are available — so it's hardly the best. I think a better way to maintain compatibility would be a solution similar to the Syntax Definitions version header, where you can opt into version 2 of a syntax definition (which has some incompatible bug fixes and extra functionality like inheritance) So you could add version 2 to a keymap file and have preceding_text behave properly. Though I'd prefer that you should add version 1 to keep the old behavior as otherwise you still have the issue with time wasted before you discover the issue and realize that you need to opt into a versioned solution

[faelin]

Ah, yes, API versioning! Agreed, that's a better solution!