[vim/vim] [WIP] Fix split/join not updating start/end status of prop (#5839)
Axel Forsman
Some progress on making sure splitting and joining text properties updates the start/end status of the props accordingly.
What does not work is undoing the effects of a join, which add an extra textprop spanning the entire line to the unjoined lines. Would appreciate any pointers on how to fix that.
The new code calls STRLEN() a few more times than the old, but does fewer heap allocations per join. I hope this is fine, otherwise there are a few calls to get_text_props that can be done manually as we already know textlen.
Fixes #5683
You can view, comment on, or merge this pull request online at:
https://github.com/vim/vim/pull/5839
Commit Summary
- Fix splitting props modifying start/end
- Fix error with overlapping props with same id/type
- Refactor adjust_prop_columns
- Fix joining textprops (WIP)
File Changes
- M src/memline.c (21)
- M src/ops.c (78)
- M src/proto/textprop.pro (4)
- M src/testdir/test_textprop.vim (25)
- M src/textprop.c (318)
Patch Links:
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
Axel Forsman
@axelf4 pushed 2 commits.
—
You are receiving this because you are subscribed to this thread.
Axel Forsman
Turns out the problem with undo was not specific to this change: One can trivially cause an internal error by creating a three-line text property, deleting the middle line and undoing that change, then deleting the line below. This because the undo readds the deleted text property, and then add_text_props_for_append() does it again. Only one will be set to end by adjust_text_props_for_delete() when the line below is deleted.
I can think of two ways to fix this:
- Either make undo act as if the text was re-typed, and don't store props for undo,
- or, commit to "same id/type" <=> allowed to unify. That is, the undo readded some prop. This was done inside the same text prop: Extend it to cover union.
Option 2 would require much work to make intuitive for all cases, which is why I prefer 1. From what I could see undo stores the entire lines, so you'd have to compute common prefix/suffix of the first/last undo:ed lines and the old lines, copy props over and resize for the insertion?
Any thoughts on this?
Bram Moolenaar
Axel Forsman wrote:
> Turns out the problem with undo was not specific to this change: One
> can trivially cause an internal error by creating a three-line text
> property, deleting the middle line and undoing that change, then
> deleting the line below. This because the undo readds the deleted text
> property, and then `add_text_props_for_append()` does it again. Only
> one will be set to end by `adjust_text_props_for_delete()` when the
> line below is deleted.
>
> I can think of two ways to fix this:
>
> props for undo, 2. or, commit to "same id/type" <=> allowed to unify.
> text prop: Extend it to cover union.
>
> Option 2 would require much work to make intuitive for all cases,
> which is why I prefer 1. From what I could see undo stores the entire
> lines, so you'd have to compute common prefix/suffix of the first/last
> undo:ed lines and the old lines, copy props over and resize for the
> insertion?
>
> Any thoughts on this?
text properties are manually added it's the only way to have a real
undo.
But it should then also undo the changes in the line above and below, so
that text properties that were adjusted for what was deleted is also
undone. Thus the number of lines that undo saves must be adjusted also
for changes in text properties.
--
Individualists unite!
/// Bram Moolenaar -- Br...@Moolenaar.net -- http://www.Moolenaar.net \\\
/// sponsor Vim, vote for features -- http://www.Vim.org/sponsor/ \\\
\\\ an exciting new programming language -- http://www.Zimbu.org ///
\\\ help me help AIDS victims -- http://ICCF-Holland.org ///
Axel Forsman
>
> I would prefer to have undo also restore the text property. In case
> text properties are manually added it's the only way to have a real
> undo.
>
> But it should then also undo the changes in the line above and below, so
> that text properties that were adjusted for what was deleted is also
> undone. Thus the number of lines that undo saves must be adjusted also
https://github.com/vim/vim/pull/5875
Fairly messy and in need of improvement,
ml_append/delete are made to take flags, whether to touch text props.
Also I noticed I got a lot of warnings for bad format specifiers in
undo.c: What is up with that?
The other pull request
https://github.com/vim/vim/pull/5839,
am I fairly happy with now -
it should just be the screenshot test that needs updating -
but it might make sense to have it blocked on this issue.
/Axel F