Closed savetheclocktower closed 1 month ago
Despite my long-term goal to make all of you realize that you need snippets in your life, I understand that some people may not feel they have enough expertise to review this one or understand what's happening. This isn't the first time this has happened.
Since this change has a spec to support it and does not regress any existing tests, I view this as exceedingly safe to merge, so I'll set a reminder on my calendar to merge this PR on Thursday. Feel free to take a look before then, but feel no obligation.
Identify the Bug
This has been biting me for a while. Given the following snippet:
This is what I expect:
=
This produces a lovely banner comment… until I try it again with leading whitespace.
https://github.com/pulsar-edit/pulsar/assets/3450/426ddddb-0caa-4f22-b2b7-26cc224ce524
When we expand a snippet, we know where the variables are within the snippet; they're described using
Point
s, but the origin of the coordinate system is the beginning of the snippet. To translate them into actual bufferPoint
s during snippet expansion, we “add” each point to thePoint
that marks the insertion point of the snippet: e.g., “start at (1, 2), then go right two columns and down one row.“When doing so, we need to remember that, when a snippet contains a newline and content after that newline, that second line of content will be indented by the same amount as the initial line. We know how much leading whitespace (if any) there was before the snippet trigger text. Folks tend to want a snippet to be indented an equal amount on each line unless they explicitly add/subtract space on subsequent lines; so if a snippet creates a new line of text, we make sure it starts with an equal amount of whitespace.
Description of the Change
For this reason, we were adding the two points incorrectly. Since the column position doesn't reset to
0
each time a row is advanced, we should've been using Point#translate, not Point#traverse.I'd have caught this earlier if I had thought to test the combination of variable expansion and leading whitespace.
Does it work?
https://github.com/pulsar-edit/pulsar/assets/3450/24242d91-d7ed-4df2-a722-1a3374191637
(I see other usages of
Point#traverse
in the same file, but those contexts are different, so I'll leave them be until they can be proven to be the source of a bug.)Alternate Designs
Since this was an incredibly simple change, I considered no alternate designs.
Possible Drawbacks
None.
Verification Process
Added a new spec for this behavior; all the existing specs pass. If the button's green, click it!
Release Notes