check for EOF before unsetting row, col & line cache in view_coord_get

[jeremybobbin]

fixes #1074 & #1084 & #1086 https://github.com/martanne/vis/issues/1074 https://github.com/martanne/vis/issues/1084 https://github.com/martanne/vis/issues/1086

[mcepl]

After the disaster of https://github.com/martanne/vis/issues/1074, it would be probably prudent to explain slightly more why do you think that this PR works better than the other one.

[erf]

@mcepl "disaster" ..? Please try to keep a positive language when commenting . There really isn't much development going on here lately, and we don't want to discourage people who actually wants to contribute !

EDIT: i thought you referred to his contribution , but if you rather referred to the buggy code, then my mistake !

[jeremybobbin]

Personally I do not mind the language. :) I believe "disaster" is a fair characterization, but then again, the fact that #1074 had gone unnoticed for as long as it did, is also disastrous - especially given vis is "Combining Modal Editing with Structural Regular Expressions".

If I didn't use structural regular expressions, I'd be using vim or neovim.

The change from pos > view->end to pos >= view->end made it such that if a selection was beyond the view, view_coord_get would return false. I believe view_coord_get is supposed to return whether or not a position in the buffer is visible.

There is a case, however, where the position can be at the view's end & still be visible, and that's when EOF is in the view. In this patch, we check if the view's end is EOF.

[mcepl]

Personally I do not mind the language. :) I believe "disaster" is a fair characterization, but then again, the fact that #1074 had gone unnoticed for as long as it did, is also disastrous - especially given vis is "Combining Modal Editing with Structural Regular Expressions".

True, and I am sorry if I am too direct, but how else should I call a commit which had to be withdrawn? A shining success? And of course it doesn’t mean anything about yourself personally, I do such disasters a dozen weekly. :(

And, of course, I appreciate a lot that somebody finally started to look at the C part of vis.

The change from pos > view->end to pos >= view->end made it such that if a selection was beyond the view, view_coord_get would return false. I believe view_coord_get is supposed to return whether or not a position in the buffer is visible.

There is a case, however, where the position can be at the view's end & still be visible, and that's when EOF is in the view. In this patch, we check if the view's end is EOF.

Again, I am not the greatest C programmer myself, but doesn’t view->end != text_size(view->text)) look like a way too complicated way how to say !EOF()? Shouldn’t we have some at least macro for it?

[erf]

True, and I am sorry if I am too direct, but how else should I call a commit which had to be withdrawn? A shining success? And of course it doesn’t mean anything about yourself personally, I do such disasters a dozen weekly. :(

It doesn't have to be either a "disaster" or a "shining success", just try to be somewhat professional and ask for more information, in a polite way. No reason to address a failed attempt at fixing an issue like a "disaster", it doesn't encourage more attempts at fixing stuff, if we don't get it right the first time.

Also please read the Github Community Guidelines if you have not. We want to have a positive community around this project.

[jeremybobbin]

but doesn’t view->end != text_size(view->text)) look like a way too complicated way how to say !EOF()? Shouldn’t we have some at least macro for it?

Makes sense - from another function in view.c:406:

        bool eof = view->end == text_size(view->text);
        if (view->line->len == 0 && eof && view->line->prev)

I've changed the patch to match the above.

No reason to address a failed attempt at fixing an issue like a "disaster"

In mcepl's defense, #1074 introduced a bug which would be easily noticed by users who are opening a vis buffer for the first time, possibly leaving a bad impression. On a separate note, If I were driven away by the "disaster" characterization, what would that say about the value I see in Vis?

That aside, these sorts of issues should be covered in the test suite. Writing a test for that now.

[mcepl]

      bool eof = view->end == text_size(view->text);

Isn’t this line a perfect candidate for refactoring (that’s why I was talking about macro, or perhaps function)?

That aside, these sorts of issues should be covered in the test suite. Writing a test for that now.

Yes, a low coverage of our test suite is much larger problem. I think we should include https://github.com/martanne/vis-test to this repo and start to working more intensly on increasing the coverage, but that is aside of this particular issue.

[rnpnr]

I've been using this for a day and it seems correct.

It also fixes another unreported issue with the previous commit where starting a visual line selection and moving the cursor to the end of window would make it look like the selection was lost.

I'm in favor of merging this rather than reverting the previous commit.

[mcepl]

It also fixes another unreported issue with the previous commit where starting a visual line selection and moving the cursor to the end of window would make it look like the selection was lost.

I'm in favor of merging this rather than reverting the previous commit.

Oh, I had https://github.com/martanne/vis/issues/1074 already reverted, you mean, it could be kept around with this commit on the top?

[rnpnr]

Oh, I had #1074 already reverted, you mean, it could be kept around with this commit on the top?

This commit applies on top of c22b2c2937d9dd65d0609e57f948f08da7f5c5ea yes.

[ninewise]

Applied here as well, thanks!