Skip to content

Maintain overlays order invariant #138

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.

Sign up for GitHub

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

Open
lua-vr wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Maintain overlays order invariant #138

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6fac8bd
Initialize range overlay before pushing elements
lua-vr Jun 5, 2024
1259519
Ensure the order of overlays is maintained as an invariant.
lua-vr Jun 5, 2024
552a5a7
Call field modification hook only once per edit
lua-vr Jun 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 34 additions & 13 deletions tempel.el
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,11 @@ If a file is modified, added or removed, reload the templates."
(defvar tempel--inhibit-hooks nil
"Inhibit tempel modification change hooks from running.")

(defvar tempel--pending-field-edit nil
"Internal variable to supress repeated calls of
`tempel--field-modified' when an edit overlaps multiple field
boundaries.")

(defvar-local tempel--active nil
"List of active templates.
Each template state is a pair, where the car is a list of overlays and
Expand Down Expand Up @@ -220,11 +225,25 @@ REGION are the current region bounds."
(tempel--inhibit-hooks t))
(tempel--disable (overlay-get ov 'tempel--range)))))

(defun tempel--ensure-ov-invariant (st ov)
"Ensure the overlay order is maintained, by clamping the boundaries
of every overlay which was added to the state ST before OV to the
start of OV, and likewise for overlays added to ST after OV."
(let ((start (overlay-start ov))
(end (overlay-end ov))
before-current)
(dolist (ov_ (cdar st))
(if (eq ov ov_) (setq before-current t)
(if before-current
(move-overlay ov_ (min start (overlay-start ov_)) (min start (overlay-end ov_)))
(move-overlay ov_ (max end (overlay-start ov_)) (max end (overlay-end ov_))))))))
Copy link
Copy Markdown
Contributor Author

@lua-vr lua-vr Jun 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@minad I'm pretty sure it should be safe to just enforce this on the overlays after the current overlay (so we could stop the loop when (eq ov ov_)). But I'm not completely sure, do you have opinions?

Copy link
Copy Markdown
Contributor Author

@lua-vr lua-vr Jun 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Proof: we start in a state where the order of overlay markers is right. Markers with different positions or with the same insertion type never change their relative order after a buffer edit. If they have the same position and different insertion type, the one with insertion type t can move forward and the other stay fixed.

The only field or form overlay marker with insertion type t is the marker at the start of a form overlay. If it moves forward relative to a marker of other overlay, this marker should be the end marker or be at the same position of the end marker since the state began ordered. But then the state is still ordered.

The other way for markers to change position is if we set the position in code. But in the code we are only moving the end of form/field overlays to the right (forward direction), so it suffices to enforce the invariant to the right.

Copy link
Copy Markdown
Owner

@minad minad Jun 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, my opinion was that there should be a proof. ;)

Please continue improving the code in your PRs - maybe you find better/more robust solutions. I will definitely merge these fixes. I just want to let you know that I won't look often at your PR due to time constraints - so please don't worry when I am not responding soon. Right now we still have to wait for the FSF assignment anyway.


(defun tempel--field-modified (ov after beg end &optional _len)
"Update field overlay OV.
AFTER is non-nil after the modification.
BEG and END are the boundaries of the modification."
(unless tempel--inhibit-hooks
(unless (or tempel--inhibit-hooks (xor after tempel--pending-field-edit))
(setq tempel--pending-field-edit (not after))
(let ((inhibit-modification-hooks nil)
(tempel--inhibit-hooks t))
(cond
Expand All @@ -237,7 +256,8 @@ BEG and END are the boundaries of the modification."
(after
(let ((st (overlay-get ov 'tempel--field)))
(unless undo-in-progress
(move-overlay ov (overlay-start ov) (max end (overlay-end ov))))
(move-overlay ov (overlay-start ov) (max end (overlay-end ov)))
(tempel--ensure-ov-invariant st ov))
(when-let ((name (overlay-get ov 'tempel--name)))
(setf (alist-get name (cdr st))
(buffer-substring-no-properties
Expand All @@ -256,20 +276,20 @@ BEG and END are the boundaries of the modification."
(let (x)
(setq x (or (and (setq x (overlay-get ov 'tempel--form)) (eval x (cdr st)))
(and (setq x (overlay-get ov 'tempel--name)) (alist-get x (cdr st)))))
(when x (tempel--synchronize-replace (overlay-start ov) (overlay-end ov) ov x)))))
(when x (tempel--synchronize-replace (overlay-start ov) (overlay-end ov) ov x st)))))
;; Move range overlay
(move-overlay range (overlay-start range)
(move-overlay range (min (overlay-start range) (overlay-start ov))
(max (overlay-end range) (overlay-end ov))))))

(defun tempel--synchronize-replace (beg end ov str)
(defun tempel--synchronize-replace (beg end ov str st)
"Replace region between BEG and END with STR.
If OV is alive, move it."
(let ((old (buffer-substring-no-properties beg end)))
(setq ov (and ov (overlay-buffer ov) ov))
(unless (equal str old)
(unless (eq buffer-undo-list t)
(push (list 'apply #'tempel--synchronize-replace
beg (+ beg (length str)) ov old)
beg (+ beg (length str)) ov old st)
buffer-undo-list))
(let ((buffer-undo-list t))
(save-excursion
Expand All @@ -278,6 +298,7 @@ If OV is alive, move it."
(insert str)
(when ov
(move-overlay ov beg (point))
(tempel--ensure-ov-invariant st ov)
(tempel--update-mark ov)))))))

(defun tempel--update-mark (ov)
Expand All @@ -297,7 +318,7 @@ INIT is the optional initial input.
Return the added field."
(let ((ov (make-overlay (point) (point)))
(hooks (list #'tempel--field-modified)))
(push ov (car st))
(push ov (cdar st))
(when name
(overlay-put ov 'tempel--name name)
(setq init (or init (alist-get name (cdr st))))
Expand All @@ -316,6 +337,7 @@ Return the added field."
(overlay-put ov 'tempel--default
(if (string-suffix-p ": " init) 'end 'start)))
(tempel--synchronize-fields st ov)
(goto-char (overlay-end ov))
ov))

(defun tempel--form (st form)
Expand All @@ -329,7 +351,7 @@ Return the added field."
(let ((ov (make-overlay beg (point) nil t)))
(overlay-put ov 'face 'tempel-form)
(overlay-put ov 'tempel--form form)
(push ov (car st))
(push ov (cdar st))
ov)))

(defmacro tempel--protect (&rest body)
Expand Down Expand Up @@ -403,13 +425,12 @@ If a field was added, return it."
(when (and (<= (overlay-start ov) (point)) (>= (overlay-end ov) (point)))
(setf (overlay-end ov) (point)))))
;; Activate template
(let ((st (cons nil nil))
(ov (point))
(tempel--inhibit-hooks t))
(let* ((ov (make-overlay (point) (point) nil t))
(st (cons (list ov) nil))
(tempel--inhibit-hooks t))
(while (and template (not (keywordp (car template))))
(tempel--element st region (pop template)))
(setq ov (make-overlay ov (point) nil t))
(push ov (car st))
(move-overlay ov (overlay-start ov) (point))
(overlay-put ov 'modification-hooks (list #'tempel--range-modified))
(overlay-put ov 'tempel--range st)
(overlay-put ov 'tempel--post (plist-get plist :post))
Expand Down