-
Notifications
You must be signed in to change notification settings - Fork 27
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
Move overlay face to a different overlay #16
Conversation
(< (overlay-end ol) end) | ||
(overlay-get ol 'macrostep-original-text)) | ||
(macrostep-collapse-overlay ol t)))) | ||
(when (and (overlay-buffer ol) ; collapsing may delete other overlays |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain the additional (overlay-buffer ol)
condition? I guess the potential bug is that we call (overlays-in start end)
at the top of the loop and loop over the resulting list, but overlays that come later in the list may already have been deleted by collapsing overlays that come earlier. Good catch.
Is this required for the addition of the highlight-only overlays or is it more of a general cleanup/bugfix? If it's the latter, it might be good to put it in a separate commit with its own explanatory commit message.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's the bug. However, I only encountered the bug when using these extra highlight overlays. When I was testing I found that only these new overlays caused an error in that loop (since deleting their corresponding text overlay deletes them), though I can't be certain that it was never possible before.
Do you still want a separate commit for that part?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, that's fine. Sorry for letting this drop for so long, I'll merge it now.
(1+ (point)) | ||
(point)))) | ||
(back-overlay (unless macrostep-expansion-buffer | ||
(copy-overlay overlay)))) | ||
(unless macrostep-expansion-buffer |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So if I understand the patch, for each "normal" overlay we additionally create a duplicate overlay with the same extents and priority 0, which is used only for applying the highlight face. I think I'd prefer to use highlight-overlay
rather than back-overlay
for both the variable and the property, to make its purpose clearer. Also, the property symbol should be prefixed with macrostep-
to keep it scoped to this package (so macrostep-highlight-overlay
). Would you mind making those changes? Otherwise I can do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's right. I made your suggested changes as well.
Though I found a bug when giving them a nil priority (when the region surrounds the overlays completely then it didn't work), so now they're given a negative priority.
This different overlay has a negative priority, which lets the region face work with macrostep.
Applied. Thanks! |
This lets the region overlay face work properly with macrostep.
Closes #14.