Skip to content

Maintain overlays order invariant#138

Open
lua-vr wants to merge 3 commits intominad:mainfrom
lua-vr:order-invariant
Open

Maintain overlays order invariant#138
lua-vr wants to merge 3 commits intominad:mainfrom
lua-vr:order-invariant

Conversation

@lua-vr
Copy link
Copy Markdown
Contributor

@lua-vr lua-vr commented Jun 5, 2024

As discussed in #137, also related to #99 and #55. The commit messages gradually explain each change.

Also, do you have copyright assigment, see https://github.com/minad/tempel?tab=readme-ov-file#contributions?

I don't, but I sent an email to the FSF with the form you linked to in other PR (this one).

@lua-vr lua-vr force-pushed the order-invariant branch from 29440e7 to fadd5e2 Compare June 5, 2024 15:39
tempel.el Outdated
(defvar tempel--inhibit-hooks nil
"Inhibit tempel modification change hooks from running.")

(defvar tempel--pending-field-modified nil
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Can we find a better solution without this global variable?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I also find it ugly but I couldn't, because the modification hook is called twice. I tried removing one of the hooks but this would end up making the field not apply edits from one of the sides.

Copy link
Copy Markdown
Owner

@minad minad Jun 5, 2024

Choose a reason for hiding this comment

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

Yes, but maybe one could remove the hook temporarily.

EDIT: See also tempel--inhibit-hooks which we could maybe reuse? I don't think this will work, this variable is meant to be let-bound.

EDIT2: Perhaps it is also possible to use stronger conditions inside the hook to detect the double calling. The first hook invocation for empty fields could be ignored?

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.

Note that to make the examples with adjacent fields work properly the modification hook of the second field also needs to be suppressed, even if they are not empty, because otherwise the second field thinks it has been modified. But perhaps this can also be checked, I will try later.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@minad how to distinguish between the first invocation and the second invocation? if the first invocation is ignored then the overlay is still empty in the second

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.

I think it's not possible to prevent double calls to the hook without some global variable, and if every modification is not handled exactly once (across all fields) the modification range passed as argument for the subsequent calls is invalid and can potentially make a mess.

@minad
Copy link
Copy Markdown
Owner

minad commented Jun 5, 2024

Thanks. Please let me know when the copyright assignment is complete.

@lua-vr lua-vr force-pushed the order-invariant branch from fadd5e2 to 5302378 Compare June 5, 2024 19:27
lua-vr added 3 commits June 5, 2024 16:32
When tempel--synchronize-fields was called from tempel--field the range
overlay did not exist yet, which would make the first function modify
the field overlay as if it was the range overlay. This is fixed by
initializing a range overlay before pushing elements into the state.
This is done via the function tempel--ensure-ov-invariant, which
should be called after every field and form overlay motions. The
invariant is maintained by making sure the boundaries of every overlay
that was added to the state before the current overlay are clamped to
the start of the current overlay, and likewise for overlays that were
added later. Since overlays are added to the state in order, this
ensures the invariant.
When an edit spans across multiple field overlay boundaries, either
because a field has length zero, two fields are adjacent or the edit
spans across multiple fields (say, a paste operation or a mark
deletion), after the first modification hook is executed the buffer is
left in a different state than it was right after the edit. Subsequent
calls to the hook would receive the edit boundaries corresponding to
the original state, which resulted in invalid operations. This change
makes sure this doesn't happen.
@lua-vr lua-vr force-pushed the order-invariant branch from 5302378 to 552a5a7 Compare June 5, 2024 19:32
(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

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

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

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.

@lua-vr
Copy link
Copy Markdown
Contributor Author

lua-vr commented Jun 13, 2024

Hi! A small update: I've received the assignment document, but they say that my university must also sign something, so I'm not sure how long it will take (and I need to figure out how this process works). 😕

@lua-vr
Copy link
Copy Markdown
Contributor Author

lua-vr commented Jun 17, 2024

So, the university office which is supposed to take care of intellectual property did not answer my e-mails. I have no idea if they are ignoring them or just taking a long time to answer --- I can't go there physically because it's in another city. In any case, I'm not very optimist because I read from someone else that in a similar situation they didn't sign any statements. Which is a bit funny, because it's a public university and I'm pretty sure the government would never bother to claim copyright over little pieces of code.

@minad
Copy link
Copy Markdown
Owner

minad commented Jun 18, 2024

Okay, thanks for letting me know. I suggest to send them a friendly reminder in a week. I think it is best to explain the situation regarding the copyright assignment clearly, mentioning that students and the universities profit from free software for teaching and research, while at the same time there shouldn't be any overlap with the ip created during research at the university. You may also find examples of other universities which have signed the papers.

@lua-vr
Copy link
Copy Markdown
Contributor Author

lua-vr commented Jun 18, 2024

Thanks @minad. I will see what I can do, because I was also planning to improve the handling of multiple active snippets when I have some spare time.

@minad
Copy link
Copy Markdown
Owner

minad commented Jun 28, 2024

Any news?

@lua-vr
Copy link
Copy Markdown
Contributor Author

lua-vr commented Jun 30, 2024

Sorry, but no, they are really not answering the e-mails (no idea why). I don't know if there are other places where I could ask, to be honest I think people here don't bother much with unconventional things. There is a portal/system to help researchers register patents and that's it.

Edit: I will write back to the FSF and ask if they have any disclaimers from USP, and if they do, how did the signees manage to get it...

@minad
Copy link
Copy Markdown
Owner

minad commented Jun 30, 2024

Okay thanks. I close the PRs for now. Please inform me if the situation changes and then we can proceed.

@minad minad closed this Jun 30, 2024
@lua-vr
Copy link
Copy Markdown
Contributor Author

lua-vr commented Jun 30, 2024

That's unfortunate. Sorry for not waiting before submitting the PR in the first place, I was not expecting it to be so complicated.

@minad
Copy link
Copy Markdown
Owner

minad commented Jul 18, 2024

I guess there still hasn't been any progress?

@lua-vr
Copy link
Copy Markdown
Contributor Author

lua-vr commented Jul 18, 2024

No, the FSF has not replied my last email either...

@lua-vr
Copy link
Copy Markdown
Contributor Author

lua-vr commented Aug 22, 2024

If anyone seeing this has any legal suggestions, I'm all ears, I really don't want to be unable to contribute to any FSF-affiliated project. The context is that nobody is replying me, neither the FSF nor my university!

@minad minad reopened this Jan 21, 2026
@minad
Copy link
Copy Markdown
Owner

minad commented Jan 21, 2026

Iirc you got copyright assignment now. I added your first commit in the 3c69363, since this is a clear bug fix. Regarding the other commits I am not yet entirely sure.

  1. I'd like to avoid the global variable for the pending edits, but I have to think more about this.
  2. I wonder why we have to enforce the order invariant explicitly. I naively expected that the orderlays should stay in order, but this seems to only hold if overlays are not in direct contact?

@lua-vr
Copy link
Copy Markdown
Contributor Author

lua-vr commented Jan 21, 2026

It has been a long time, but reading my last commit message I also have the impression that using a global variable is wrong. Perhaps it should make sure the beg and end are kept correct for subsequent calls by using two markers? In any case, I'm moving to another continent soon and I make no promises I will be able to resurrect this PR 🙂

@lua-vr
Copy link
Copy Markdown
Contributor Author

lua-vr commented Jan 21, 2026

I wonder why we have to enforce the order invariant explicitly. I naively expected that the orderlays should stay in order, but this seems to only hold if overlays are not in direct contact?

IIRC, the problem was that they could overlap. (but not completely move past each other). So it was enforcing the order of the markers themselves (overlays are only partially ordered).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants