drym-org / symex.el

An intuitive way to edit Lisp symbolic expressions ("symexes") structurally in Emacs
Other
271 stars 22 forks source link

Generalize overlay for use in Lisp too #52

Closed countvajhula closed 2 years ago

countvajhula commented 2 years ago

Summary of Changes

The Symex master branch contains an optional "highlight" feature that uses Emacs regions to highlight the current expression. This is not a great way to do it since regions have existing connotations that interfere with their ability to play the pure UI role of highlighting. For instance, Evil considers an active region to mean "Visual mode". Thus, using highlighting causes existing features like evaluate to work in unexpected ways.

The tree-sitter branch introduces an overlay for highlighting the currently selected expression. This is the right way to do it and it doesn't suffer from the problems associated with using regions. The present PR promotes the overlay used for tree-sitter code to the UI level (instead of the primitive tree-sitter level) so that it works for both tree-sitter as well as Lisp, with appropriate primitives fulfilling basic utilities in Lisp and tree-sitter.

(Moved from polaris64/symex.el#1)

Public Domain Dedication

countvajhula commented 2 years ago

If I understand correctly, I believe it does undo the excursion since, after selecting the topmost node, it then goes down by the originally calculated "point-height offset" (i.e. the height difference in steps from the current node to the highest node that has the same starting point in the buffer), so that should return it to the original node.

I agree it is a little awkward, but I don't know if there is a better way to get the save excursion behavior. It would be ideal if the underlying tree-sitter library offered a way to select a node by id -- then, we could save the id before doing things and afterwards select the node by id when we're done. But I didn't see such a utility in the tsc module after a brief survey, or any way to get a unique identifier for a node.

polaris64 commented 2 years ago

If I understand correctly, I believe it does undo the excursion since, after selecting the topmost node, it then goes down by the originally calculated "point-height offset" (i.e. the height difference in steps from the current node to the highest node that has the same starting point in the buffer), so that should return it to the original node.

OK that makes sense.

I agree it is a little awkward, but I don't know if there is a better way to get the save excursion behavior. It would be ideal if the underlying tree-sitter library offered a way to select a node by id -- then, we could save the id before doing things and afterwards select the node by id when we're done. But I didn't see such a utility in the tsc module after a brief survey, or any way to get a unique identifier for a node.

I had a look and I came across tree-sitter-save-excursion. Perhaps that handles what we need directly?

In any case, as I say it seemed to work fine when I tested it so I'm happy to approve if the above isn't of any use :)

countvajhula commented 2 years ago

Good find @polaris64 . Unfortunately I don't think we can use it at the moment, since it doesn't just "save excursion" but also includes some UI-related functionality such as recentering the window and smooth scrolling, which could interfere with higher-level functionality (e.g. I ran into errors while using it with Rigpa Buffer mode due to the save excursion macro attempting to recenter a window that was no longer active after navigating to a different buffer).

Comparing the docstrings for save-excursion vs tree-sitter-save-excursion, it's apparent that they aren't quite equivalent for our purposes:

save-excursion:

Save point, and current buffer; execute BODY; restore those things.
Executes BODY just like ‘progn’.
The values of point and the current buffer are restored
even in case of abnormal exit (throw or error).

tree-sitter-save-excursion:

After the location is restored, the buffer text is scrolled so that point stays
at roughly the same vertical screen position. If `pixel-scroll' is available and
`tree-sitter-save-excursion-pixelwise' is non-nil, pixelwise scrolling is used
instead, to make this restoration exact.

So I'd prefer to merge the current implementation as is for now, since we don't have any actual failing cases. We can always draw inspiration from this other implementation (i.e. the non-UI parts) down the line if we discover an edge case where the current implementation fails.

polaris64 commented 2 years ago

I completely agree; it seems to work fine during all of my tests so I'm happy to approve it. As you say we can always revisit it later.

I'll approve it now!

polaris64 commented 2 years ago

Ah, I already reviewed it with my comment above by the looks of it, so please take this as approval and please merge.