Comment by ~hi_angel on ~tsdh/highlight-parentheses.el
I see, thank you very much!
on ~tsdh/highlight-parentheses.el
"~hi_angel" outgoing@sr.ht writes:
Cool, thanks! Just tested, works for me!
Great.
I'm not seeing how to leave a code commentary on this site, so some comments here:
@@ 170,27 176,66 @@ If the optional argument OVERLAYS (a list) is non-nil, delete all overlays in it instead." (mapc #'delete-overlay overlays)) +(defun highlight-parentheses--highlight-needed-p () + "Return non-nil when if re-highlighting is needed.""when if" in the description, probably meant to be just
when
or justif
.Fixed.
+ (let ((start (if (< point last-point) point last-point)) + (end (if (< point last-point) last-point point)))This part can be simplified (and optimized for that matter) by using min and max like:
(let ((start (min point last-point)) (end (max point last-point)))
Right, fixed.
+ ;; Lets assume we keep moving in the same direction, so update + ;; last-point to the current value of point. + (setq highlight-parentheses--last-point point) + nil))))))You execute
(setq highlight-parentheses--last-point point)
both here and in(highlight-parentheses--highlight)
right afterwards. I think one of them is extraneous.No, that's intended.
highlight-parentheses--highlight-needed-p
moves point when no re-highlighting is needed and therefore thesetq
inhighlight-parentheses--highlight
won't be executed.I don't understand the algorithm too well to say which one, but offhand I would remove the one you just added to
highlight-parentheses--highlight-needed-p
because the function seems to scan range for parentheses, and then changing "last point" is sudden for me as a side-reader. Especially so since it's not even changing the "last pair" and postfixed with-p
.Yes, that's surely a sneaky side-effect. But it's an optimization. It assumes that you probably keep moving in the same direction so it sets the last point value in that direction if no paren crosses our way so that the 5000-char window where we scan each character moves with point.
+ (setq highlight-parentheses--last-pair + (cons (overlay-start (car highlight-parentheses--overlays)) + (overlay-start (cadr highlight-parentheses--overlays)))))))I presume that part should be at the end of the
(catch 'no-change …)
instead of being outside it? I.e. because you only want to update the last pair when it needs to be updated, and it only needs to be updated when'no-change
wasn't thrown. Unless I'm misunderstanding something of course.I update it always with the values of the surrounding pair which are the first 2 overlays in
highlight-parentheses--overlays
. That has the nice effect that we change the value to(nil)
also when we aren't surrounded by parens anymore. In your version, we would update the value only if there is a surrounding pair. And in your version, we also wouldn't update if the closing paren was gone (we killed it with some editing command).I have a question. I'm probably asking something stupid, because I don't understand this algorithm too well, but why in the code below you execute
move-overlay
always, disregarding if last pair changed or not? It would seem to me overlay only needs to be moved when pair cache is invalidated…Yes, true. But I benchmarked it. Moving an overlay to its current location one million times takes 0.2 seconds so no need to worry.
Bye, Tassilo
Comment by ~hi_angel on ~tsdh/highlight-parentheses.el
Hmmm, weird. Part of the text got lost. It is good that I edited it via Qutebrowser + Emacs and I still have a buffer with the text.
Actually, nvm, it may have been on my side but it's weird. I just noticed that the leftover buffer is still marked as unsaved (and if I undo I see that the last time it was saved matches with the "truncated text"). But it's weird, because I edit texts in browser via a separate
emacsclient
which I always exit with:x
(using Evil). I mean, I just don't exit this window in a way that may not get saved. No idea what happened there.
Comment by ~hi_angel on ~sircmpwn/todo.sr.ht
Hmmmm… Actually, now I am not sure the issue exists, maybe it should be closed… (which I can't seem to be able to do)
The reason I'm saying this is because I just saw in the Emacs buffer a mark mentioning that buffer was modified but not saved. This is weird because I edit with separate
emacsclient
window which I always exit with:x
so I can't imagine why buffer would've not been written. But the fact is it looks like it haven't been so the issue may be on my side…
Comment by ~hi_angel on ~sircmpwn/todo.sr.ht
Sorry, can't change the title. The part
and lost
probably needs to be removed: "getting truncated" is descriptive enough and the "lost" sounds like a comment gets lost which is not what I meant 😅
Ticket created by ~hi_angel on ~sircmpwn/todo.sr.ht
I was writing a comment on an issue and apparently it got too long so
todo
interface without any warning just truncated the text after submitting.It is fortunate that I had been writing via Emacs + Qutebrowser, so I still had full version of my text in the editor. Otherwise that would've been pretty bad as I spent quite some time there reviewing the code.
#Steps to reproduce
Open an issue
Post a comment with the following text
Details
> I've implemented both your and my optimizations. I've tested both individually but it seems the effect is best when used together Cool, thanks! Just tested, works for me! I'm not seeing how to leave a code commentary on this site, so some comments here: ```diff @@ 170,27 176,66 @@ If the optional argument OVERLAYS (a list) is non-nil, delete all overlays in it instead." (mapc #'delete-overlay overlays)) +(defun highlight-parentheses--highlight-needed-p () + "Return non-nil when if re-highlighting is needed." ``` "when if" in the description, probably meant to be just `when` or just `if`. ```diff + (let ((start (if (< point last-point) point last-point)) + (end (if (< point last-point) last-point point))) ``` This part can be simplified (and optimized for that matter) by using min and max like: (let ((start (min point last-point)) (end (max point last-point))) ----------------------- ```diff + ;; Lets assume we keep moving in the same direction, so update + ;; last-point to the current value of point. + (setq highlight-parentheses--last-point point) + nil)))))) ``` You execute `(setq highlight-parentheses--last-point point)` both here and in `(highlight-parentheses--highlight)` right afterwards. I think one of them is extraneous. I don't understand the algorithm too well to say which one, but offhand I would remove the one you just added to `highlight-parentheses--highlight-needed-p` because the function seems to scan range for parentheses, and then changing "last point" is sudden for me as a side-reader. Especially so since it's not even changing the "last pair" and postfixed with `-p`. ```diff + (setq highlight-parentheses--last-pair + (cons (overlay-start (car highlight-parentheses--overlays)) + (overlay-start (cadr highlight-parentheses--overlays))))))) ``` I presume that part should be at the end of the `(catch 'no-change …)` instead of being outside it? I.e. because you only want to update the last pair when it needs to be updated, and it only needs to be updated when `'no-change` wasn't thrown. Unless I'm misunderstanding something of course. ----------------------- I have a question. I'm probably asking something stupid, because I don't understand this algorithm too well, but why in the code below you execute `move-overlay` always, disregarding if last pair changed or not? It would seem to me overlay only needs to be moved when pair cache is invalidated… ```diff + (move-overlay (pop overlays) pos1 (1+ pos1)) + (when (setq pos2 (scan-sexps pos1 1)) + (move-overlay (pop overlays) (1- pos2) pos2) + ;; Check if the immediately surrounding pair of parens is at the + ;; same location as before. If so, we can skip moving the other + ;; overlays since they haven't changed, too. + (when (and first-iteration + (equal highlight-parentheses--last-pair + (cons pos1 (1- pos2)))) + (throw 'no-change t)) + (setq first-iteration nil))))) ```
#Expected
The comment is posted in full
#Actual
The comment gets truncated
#Additional information
An issue where I've seen this originally https://todo.sr.ht/~tsdh/highlight-parentheses.el/5
Comment by ~hi_angel on ~tsdh/highlight-parentheses.el
Hmmm, weird. Part of the text got lost. It is good that I edited it via Qutebrowser + Emacs and I still have a buffer with the text.
The lost part:
+ (setq highlight-parentheses--last-pair + (cons (overlay-start (car highlight-parentheses--overlays)) + (overlay-start (cadr highlight-parentheses--overlays)))))))I presume that part should be at the end of the
(catch 'no-change …)
instead of being outside it? I.e. because you only want to update the last pair when it needs to be updated, and it only needs to be updated when'no-change
wasn't thrown. Unless I'm misunderstanding something of course.
I have a question. I'm probably asking something stupid, because I don't understand this algorithm too well, but why in the code below you execute
move-overlay
always, disregarding if last pair changed or not? It would seem to me overlay only needs to be moved when pair cache is invalidated…+ (move-overlay (pop overlays) pos1 (1+ pos1)) + (when (setq pos2 (scan-sexps pos1 1)) + (move-overlay (pop overlays) (1- pos2) pos2) + ;; Check if the immediately surrounding pair of parens is at the + ;; same location as before. If so, we can skip moving the other + ;; overlays since they haven't changed, too. + (when (and first-iteration + (equal highlight-parentheses--last-pair + (cons pos1 (1- pos2)))) + (throw 'no-change t)) + (setq first-iteration nil)))))
Comment by ~hi_angel on ~tsdh/highlight-parentheses.el
I've implemented both your and my optimizations. I've tested both individually but it seems the effect is best when used together
Cool, thanks! Just tested, works for me!
I'm not seeing how to leave a code commentary on this site, so some comments here:
@@ 170,27 176,66 @@ If the optional argument OVERLAYS (a list) is non-nil, delete all overlays in it instead." (mapc #'delete-overlay overlays)) +(defun highlight-parentheses--highlight-needed-p () + "Return non-nil when if re-highlighting is needed.""when if" in the description, probably meant to be just
when
or justif
.+ (let ((start (if (< point last-point) point last-point)) + (end (if (< point last-point) last-point point)))This part can be simplified (and optimized for that matter) by using min and max like:
(let ((start (min point last-point)) (end (max point last-point)))
+ ;; Lets assume we keep moving in the same direction, so update + ;; last-point to the current value of point. + (setq highlight-parentheses--last-point point) + nil))))))You execute
(setq highlight-parentheses--last-point point)
both here and in(highlight-parentheses--highlight)
right afterwards. I think one of them is extraneous. I don't understand the algorithm too well to say which one, but offhand I would remove the one you just added tohighlight-parentheses--highlight-needed-p
because the function seems to scan range for parentheses, and then changing "last point" is sudden for me as a side-reader. Especially so since it's not even changing the "last pair" and postfixed with-p
.
Comment by ~hi_angel on ~tsdh/highlight-parentheses.el
Oh, I'm sorry, I didn't see that. Okay, well, we can modify the original idea a bit.
So, your problem basically comes down to the fact that movement may change what pair is the closest even if we're still in the range of the one last cached. Which makes tracking the range useless… or does it? 😊
Well, note that you often have to call
(scan-sexps)
more than once, 4 times by default since you want 4 pairs to be highlighted (IIUC). So what you can do here is you call(scan-sexps)
as usual but only once. And then you check if the boundaries you get are matching the closest pair (one you cached). If it does, that means you're all good and no need to call(scan-sexps)
again.So the idea becomes:
- Have the closest pair location cached.
- If
(point)
is moved, call(scan-sexps)
just once to check if cache is still valid, otherwise invalidate it.- If buffer is modified, unconditionally invalidate the cache (buffer may get modified in multiple locations at once, so cache can no longer be trusted).
That's why my original idea was to check every character between old and new point location if there is some paren in between. Of course, that is more costly than your approach.
Well, I think my suggestion now becomes similar to yours, except buffer scanning is still done by Emacs'
(scan-sexps)
and that I imagine it's still beneficial to the common usecase of just navigating the code.
on ~tsdh/highlight-parentheses.el
"~hi_angel" outgoing@sr.ht writes:
Thank you very much for looking into it! 🎉
Too bad, we didn't see the following problem:
(((point is here: | (more (parens))))) ^ ^
So when point is at the
|
above, the cache contains the paren pair marked with ^. Now when we move point to the the "more", we are still in the boundaries, so no re-highlighting is done although it should be.That's why my original idea was to check every character between old and new point location if there is some paren in between. Of course, that is more costly than your approach.
If you have some better idea, I'm all ears. Otherwise, I'll test my original approach.