Prevented fails of fontification of annotated regions

[cage2]

Hello Bastian!

As we walk across the overlays we can get past the limit, the best i figured out to fix that was to check point even before regexp matching.

Bye! C.

[bastibe]

Hi Cage!

I admit that I don't fully understand this code any longer, so take my comments with a grain of salt...

If I understand the purpose of this function correctly, it jumps to the end of the longest annotation under point, and then captures the rest of the line, and the line break. This fails if the longest annotation extends beyond the line break, right?

However, what if point is on a short annotation and a long, multiline annotation. In that case, this PR would skip displaying both annotations. But a correct solution would display the short one, but ignore the long one.

So if that reasoning is correct, we should cl-remove-if-not any annotation that ends beyond limit a few lines before the current PR. That would filter out the multiline annotation, but leave the short annotation. And then we wouldn't have to (if (> (point) limit) ...) any more, because we are guaranteed to be within limit.

Is that reasoning correct? It's been a long time since I touched this code...

[cage2]

On Thu, Jan 30, 2020 at 01:11:27AM -0800, Bastian Bechtold wrote:

Hi Cage!

Hi! :)

I admit that I don't fully understand this code any longer, so take my comments with a grain of salt...

That's OK, i think constructive discussions takes a big part in code developing and, i dare to say, in any human activity :)

If I understand the purpose of this function correctly, it jumps to the end of the longest annotation under point, and then captures the rest of the line, and the line break. This fails if the longest annotation extends beyond the line break, right?

I admit i have some problem to wrap my head around 'font-lock' machinery but, in my opinion this is correct :)

However, what if point is on a short annotation and a long, multiline annotation. In that case, this PR would skip displaying both annotations. But a correct solution would display the short one, but ignore the long one.

I have a strong feeling this is the solution.

So if that reasoning is correct, we should cl-remove-if-not any annotation that ends beyond limit a few lines before the current PR. That would filter out the multiline annotation, but leave the short annotation. And then we wouldn't have to (if (> (point) limit) ...) any more, because we are guaranteed to be within limit.

Is that reasoning correct? It's been a long time since I touched this code...

I can not honestly say if it is correct or not (given my problem with fontification process) but, if i was able to translate to code what you just wrote, i would say this works and is much more elegant that the code in the first commit! :)

I just left the first limit checks (if (> (point) limit) ...) because i think this is harmless and instead a nice optimization (not profiled the code but i am confident that this gives an improvement in speed with no efforts (given the checks was there from long time :-D).

Bye! C.

[bastibe]

Cool, this looks great! Did you check, by any chance, whether the removal should occur at (> (overlay-end a) limit))) or (>= (overlay-end a) limit)))?

Otherwise, great work, as always!

[cage2]

On Sun, Feb 02, 2020 at 08:44:43AM -0800, Bastian Bechtold wrote:

Hi Bastian!

Cool, this looks great! Did you check, by any chance, whether the removal should occur at (> (overlay-end a) limit))) or (>= (overlay-end a) limit)))?

This is, indeed, a good observation!

An i think i need a little help :)

The manual says (i hope this is the right section):

https://www.gnu.org/software/emacs/manual/html_node/elisp/Search_002dbased-Fontification.html#Search_002dbased-Fontification

"[...]When function is called, it receives one argument, the limit of the search; it should begin searching at point, and not search beyond[1] the limit.[...]"

So my best understanding is that the correct test was:

(> (overlay-end a) limit)))

and, furthermore, we should change this test as well:

(>= (point) limit)

with

(> (point) limit)

but i am not sure the whole reasoning is correct, what do you think?

Moreover, this is unrelated to the problem above, i think the test:

(when (< (point) limit)

was useless and can be removed.

Otherwise, great work, as always!

Thank you! Is a pleasure to collaborate with you as your comments are always precise and useful!

Bye! C.

[1] The emphasis is mine

[bastibe]

I've had to think about this a bit.

"Not search beyond limit" sounds like <= limit is acceptable. However, that's talking about the result of re-search-forward, while we are talking about the overlay exclusion (aka. the start of the search aka overlay-end). If we allow overlay-end == limit, that just guarantees that re-search-forward will return nil. So in order for re-search-forward to possibly return a usable result, I think overlay-end < limit is actually better.

What a mind-bender. I'm not at all sure if that reasoning is correct, either.

You might be able to create a test case by putting the end of an annotation just before a newline, on a newline, and just after a newline. I'd be very interested in the behavior in these cases, as I remember them as beeing difficult.

[cage2]

Hi Bastian!

I've had to think about this a bit.

That's OK, take your time! :)

"Not search beyond limit" sounds like <= limit is acceptable. However, that's talking about the result of re-search-forward, while we are talking about the overlay exclusion (aka. the start of the search aka overlay-end). If we allow overlay-end == limit, that just guarantees that re-search-forward will return nil. So in order for re-search-forward to possibly return a usable result, I think overlay-end < limit is actually better.

This appears entirely reasonable to me! :)

What a mind-bender. I'm not at all sure if that reasoning is correct, either. You might be able to create a test case by putting the end of an annotation just before a newline, on a newline, and just after a newline. I'd be very interested in the behavior in these cases, as I remember them as beeing difficult.

I changed the test to reflect your considerations (i tried, actually :-)) and i performed the actions you suggested, you can find the file here (sorry i was not able to upload the file on github :/).

https://www.autistici.org/interzona/img/misc/annot.png

Seems to me that if you annotate the newline the annotation extends, and ~overwrite the~ stacks on top of the symbol before, this in not unexpected in my opinion.

I have no answers to provide unfortunately only many doubts. :D

Bye! C.

addendum:

This is what happens when there is no symbol before the newline character i try to annotate:

https://www.autistici.org/interzona/img/misc/annot2.png

I think this is not good as the third annotation (the one on the newline does not appears). Definetely i need some ~think~ time to think about that :)

Anyway the original bug this PR was intended for has disappeared, so we can be halfway happy :)

[bastibe]

Oh man, what a strange bit of code that is. Still, I would think we've already made great progress here.

[cage2]

Hi Bastian!

I was investigating the issue we got annotating the newline and i have met some bugs in the process! :)

Unfortunately i come to the conclusion that is not possible to annotate the newline itself because of the role if has in the annotation process, in fact, when annotated, it propagate the annotation overlay (the one with the underline property) to the prefix text we use to pad the annotation (the actual text of annotation, i mean). This is a problem not only when try to annotate the newline but, also, when the annotated text is a multi line one (using a region) and -of course- it include a newline char.

A picture, probably is going to explain better:

https://www.autistici.org/interzona/img/misc/annot-long-overlay-newline.png

I scratched my head about this problem, i think i could split the multi line annotation in multiple overlay that skips the newline character: there will be a lot of works to refactor all the functionality but i hope this is doable, at least; i can not see solution when the only character annotated is the newline.

I hope there is a proper solution, though. Like if was possible to delete the overlay on part of the text or just remove the underline property.

Bye! C.

PS: i think we could clean and merge this PR and move the problem about the newline to the issue tracker so can we can discuss it.

[bastibe]

Oh wow, this gets trickier and trickier.

I agree that it is probably prudent to not allow annotations ending or starting on a newline. This will probably have to be fixed at creation time.

And one more comment on your latest commits: It seems to me that you need to sort the results from overlays-at before applying cl-first, right? Specifically, sort them by start or end, depending on which you are searching for.

PS: i think we could clean and merge this PR and move the problem about the newline to the issue tracker so can we can discuss it.

I agree.

[cage2]

On Mon, Feb 10, 2020 at 02:13:38AM -0800, Bastian Bechtold wrote:

Hi Bastian!

Oh wow, this gets trickier and trickier.

Yes but eventually we humans will won over the machine! ;-D

I agree that it is probably prudent to not allow annotations ending or starting on a newline.

OK, i think this is an assumption that makes things easy without sacrificing fundamental features.

This will probably have to be fixed at creation time.

Yes, and probably when saving database too.

And one more comment on your latest commits: It seems to me that you need to sort the results from overlays-at before applying cl-first, right? Specifically, sort them by start or end, depending on which you are searching for.

Please Bastian, can you point the line where the problem is? I am a bit confused and i can not find it. :(

PS: i think we could clean and merge this PR and move the problem about the newline to the issue tracker so can we can discuss it.

I agree.

Very well, so we have a starting point! :)

My proposal road-map is:

• clean this PR (probably there is something left to do and i need to update NEWS and changelog);

• add an entry on the issue tracker referencing the problem with newline and a new PR with at least a draft of what i think could be a patch that mitigate the problem, so we can start discussing with some actual code (this usually helps, in my experience).

Do you think this is a good idea?

Bye! C.

[bastibe]

Please Bastian, can you point the line where the problem is?

Sorry about that, I added two comments.

• clean this PR (probably there is something left to do and i need to update NEWS and changelog);
• add an entry on the issue tracker referencing the problem with newline and a new PR with at least a draft of what i think could be a patch that mitigate the problem, so we can start discussing with some actual code (this usually helps, in my experience).

Great! Sounds like a plan!

[cage2]

On Tue, Feb 11, 2020 at 12:38:04AM -0800, Bastian Bechtold wrote:

Hi Bastian!

Please Bastian, can you point the line where the problem is?

Sorry about that,

No problem, no problem at all! :)

I added two comments.

Thank you, i can understand the suggestion now

I think that after this commit:

https://github.com/bastibe/annotate.el/pull/58/commits/ed0c9bc64be9feb4e6eb4f18a3655682ebcef49f#diff-b69fd45142ab15cfc8044b84df3a30ceR361

was safe to assume that a single point in a buffer can hold 0 or 1 annotation as maximum, so - if this is correct - there is non need to sort the annotations. Of course i could be wrong here (as usual!) but this is what i think.

• clean this PR (probably there is something left to do and i need to update NEWS and changelog);
• add an entry on the issue tracker referencing the problem with newline and a new PR with at least a draft of what i think could be a patch that mitigate the problem, so we can start discussing with some actual code (this usually helps, in my experience).

Great! Sounds like a plan!

Very well! Honestly i was very sad about all this problems some days ago. I mean, i was not able to figure out an acceptable solution or even a way to find one, now i am more optimistic at least! :)

Bye! C.

[bastibe]

I think that after this commit: ed0c9bc#diff-b69fd45142ab15cfc8044b84df3a30ceR361 was safe to assume that a single point in a buffer can hold 0 or 1 annotation as maximum, so - if this is correct - there is non need to sort the annotations. Of course i could be wrong here (as usual!) but this is what i think.

I see! I had missed that, sorry about that.

I am a bit torn about that restriction, to be honest. What was the reasoning for limiting each spot to one annotation?

Honestly i was very sad about all this problems some days ago. I mean, i was not able to figure out an acceptable solution or even a way to find one, now i am more optimistic at least! :)

If at any point you want to move on and be done with a feature, or abandon an issue entirely, please just say so. I know this work can be frustrating, especially with a nagging second person (me) to criticize your every step.

There's no rush, there's no pressure, there's no commitment. Only work on stuff if it's fun. The alternative is burnout, and your mental health is much, much, much more important (to me) than an issue in some bugtracker.

[cage2]

On Wed, Feb 12, 2020 at 01:53:25AM -0800, Bastian Bechtold wrote:

Hi Bastian!

I think that after this commit: ed0c9bc#diff-b69fd45142ab15cfc8044b84df3a30ceR361 was safe to assume that a single point in a buffer can hold 0 or 1 annotation as maximum, so - if this is correct - there is non need to sort the annotations. Of course i could be wrong here (as usual!) but this is what i think.

I see! I had missed that, sorry about that.

That's ok, no need to be sorry! :)

I am a bit torn about that restriction, to be honest.

that's funny because i was thinking exacting the opposite :D

What was the reasoning for limiting each spot to one annotation?

well i am not contrary in principle to that, just i do not think is useful; but this is just matter of personal taste, of course.

But seems to me i can see problems with this approach in the existing code, for example take a look here, the code to modify an already existing annotation:

https://github.com/bastibe/annotate.el/blob/master/annotate.el#L1373

if i understand correctly this function just "pick" the one of the annotations if they are overlapped making the others unreachable.

Moreover when two annotation overlaps there is no visual hints that this appended, i mean the user can see just one of the color of the underline property, is not this confusing?

Of course all the above could be fixed but my question is now: is the results worth the efforts?

These is actually honest questions, i am not saying at all that my point of view is "correct" (does corrects point of view does exists, anyway? :-)) or that i am sure that multiple annotations on the same text is useless; i am looking forward to read your opinions instead! :)

Honestly i was very sad about all this problems some days ago. I mean, i was not able to figure out an acceptable solution or even a way to find one, now i am more optimistic at least! :)

If at any point you want to move on and be done with a feature, or abandon an issue entirely, please just say so. I know this work can be frustrating, especially with a nagging second person (me) to criticize your every step.

Bastian, we are collaborating on this code for more than six month now and i can assure you i was never felt annoyed from your suggestions, instead i think you contributions was essential to improve the software; i was not able to spot so many errors, or to made code clearer and effective without your collaborations. So, in short: everything is fine! :)

I would say that even my English is improving! Well maybe this is exaggerate! :D

There's no rush, there's no pressure, there's no commitment. Only work on stuff if it's fun. The alternative is burnout, and your mental health is much, much, much more important (to me) than an issue in some bugtracker.

I can not hide that my feeling was a bit depressive in the last days but at least i do not think this comes form some kind of "social pressure" so to say. I agree that of course our mental health is much more important! :)

Thank you for caring! I accept the suggestion: let's keeps the things fun! :)

Bye! C.

[bastibe]

Of course all the above could be fixed but my question is now: is the results worth the efforts?

I hadn't thought of these issues. But now that you mention them, I agree with your assessment, that it's not worth the effort. Overlapping annotations impart more ambiguous behavior and complexity than they solve problems. Thank you for clearing that up!

I can not hide that my feeling was a bit depressive in the last days but at least i do not think this comes form some kind of "social pressure" so to say.

I am honestly glad to hear that. I've been on both sides of such a situation and it is not fun either way.

Thank you for caring! I accept the suggestion: let's keeps the things fun! :) Bye! C.

Absolutely!

[cage2]

On Thu, Feb 13, 2020 at 01:00:16AM -0800, Bastian Bechtold wrote:

Hello Bastian!

Of course all the above could be fixed but my question is now: is the results worth the efforts?

I hadn't thought of these issues. But now that you mention them, I agree with your assessment, that it's not worth the effort. Overlapping annotations impart more ambiguous behavior and complexity than they solve problems. Thank you for clearing that up!

Thank you Bastian, i thought about that and i think having overlapping annotations is not a bad idea per se and an interesting problem to solve too! I think it's Just a too complicated to reach at this moment.

I can not hide that my feeling was a bit depressive in the last days but at least i do not think this comes form some kind of "social pressure" so to say.

I am honestly glad to hear that. I've been on both sides of such a situation and it is not fun either way.

I can not say that i am immune to what other people thinks about me or my work but these feeling just started to be more weak than some times before, well i think this is a good parts of getting old! :D

Thank you for caring! I accept the suggestion: let's keeps the things fun! :) Bye! C.

Absolutely!

Very well, so i think there is some more cleaning, i have to check if there are missing docstrings and then probably we can considering merging!

Anyway, good week end! :D

C,

[cage2]

Hello Bastian!

I hope you are OK!

I made a quick check and, in my opinion, this PR could be merged.

Bye! C.

[cage2]

On Wed, Feb 19, 2020 at 12:01:14AM -0800, Bastian Bechtold wrote:

bastibe approved this pull request.

Hi Cage,

Hi Bastian!

Sorry for not answering, I must have missed your last comment.

Oh, no need to be sorry, you missed nothing as i was really busy in the last days and did not checked the code until yesterday! :D

Great work on the PR! Easily the most involved change and discussion, yet!

Nice to hear! :)

Feel free to merge.

Very well!

Bye! C.