Sat 26 Aug 2017 09:24:09 AM UTC, comment #15:
Fixed in git, 59451da3. Thanks.
(Without your adjustment, though. A patch should change the minimum amount of code in order to fix the bug. Normally I wouldn't have changed the comment either, but I have let that pass. Whitespace and comment changes and other tweaks are for follow-up patches, but I won't accept any in this case: I don't want the check for SOFTWRAP to move to get_chunk_and_edge() because I hate this mechanism of "if pointer not is NULL, then set the pointed-to". And in number of lines it makes no difference.)
|
Thu 24 Aug 2017 08:15:30 PM UTC, comment #14:
No problem, and looks good to me.
I did make one small adjustment relative to your patch: the two workarounds for calling leftedge_for() and chunk_for() in non-softwrap mode are replaced with one workaround in get_chunk_and_edge(). This is because leftedge_for() and chunk_for() are wrappers for it, it's not covered in non-softwrap mode otherwise, and doing things that way requires less code than doing it individually in multiple functions.
The git patch (with that adjustment) is attached, against git a850bb2.
(file #41645)
|
Thu 24 Aug 2017 07:38:25 PM UTC, comment #13:
Thanks for the patches. But... it should be obvious that it cannot be right to push x forward twice. What is needed is to use actual_x(), but to prevent it from making x recede again.
Attached version of your patch fixes both the sticking problem and the overstepping problem. If you are okay with it, please provide a normal git patch.
(file #41644)
|
Thu 24 Aug 2017 01:48:28 AM UTC, comment #12:
For the record, the potential problem even occurs with the original version of the melded patch.
And sorry for any confusion; I apparently missed that your patch in comment #7 used actual_x() instead of proper_x() before my version did (although that still turned out to be problematic either way).
|
Thu 24 Aug 2017 01:42:28 AM UTC, comment #11:
Forgot that proper_x() still needs to adjust leftedge as well as index. The attached new patch fixes that. The potential problem still occurs, though.
(file #41638)
|
Thu 24 Aug 2017 01:26:25 AM UTC, comment #10:
Attached another version of the patch that should be closer to what you want in general, accounting for the issues found in the previous patches.
Specifically, proper_x() now pushes the index forward if current_x is in the middle of a tab, and still sets shifted if it does so. It does no more and no less. As for the rest of the positioning variables, there's a new function called get_proper_index_and_column() which sets those, calling proper_x() and calling it again if an index shift requires it.
do_up(), do_down(), do_page_up(), and do_page_down() call get_proper_index_and_column(), so they do all they should, and do_home() calls proper_x() directly, since that's all it needs.
Note that there's still one potential problem. With the patch applied, in an 80x24 terminal, run:
./nano --ignore --soft --nowrap -T164 +7,-1 ../NEWS
Press Up twice, and then Down twice. The cursor won't be quite where it was before, even if your added warnings say the column values are the same.
(file #41637)
|
Thu 24 Aug 2017 12:31:11 AM UTC, comment #9:
Attached a newer version of the patch that, after it recalculates leftedge, recalculates current_x using proper_x() again instead of actual_x(). This is actually required in some cases. Without it, in an 80x24 terminal, try running
./nano --ignore --soft --nowrap -T181 ../NEWS
and pressing Down several times. After the first time, the cursor will no longer move down. With it, Down works again as expected.
It also updates comments about proper_x() to properly mention leftedge shifting. (So, relative to the first melded patch, the only code change is having proper_x() recalculate leftedge.)
(file #41636)
|
Wed 23 Aug 2017 11:50:43 PM UTC, comment #8:
Attached a new version of the patch that does parts of what you want. proper_x() now adjusts leftedge, so it takes a filestruct instead of a char* because leftedge_for() needs a filestruct. Also, the adjustment of current_x after leftedge is recalculated is done with actual_x(), as you mentioned. And the comment for proper_x() is expanded to mention what the forward parameter does.
However, I haven't moved the other parts into proper_x() yet. First, since proper_x() doesn't directly use globals such as current_x elsewhere, having it adjust placewewant seems out of place. Since placewewant is just leftedge plus target_column, it could be easily set afterwards, since target_column is relative to the current row anyway, barring adjustments via actual_last_column().
Second, adjusting placewewant and recalculating index will produce the right behavior in do_up(), do_down(), do_page_up(), and do_page_down(), but doing so will cause problems in do_home(), which also uses proper_x().
Since it's not a vertical movement function, do_home() does its own placewewant handling, so any setting of placewewant inside proper_x() will be immediately overridden. But the worse problem is this: in do_home(), adjusting leftedge is correct (and it should have been done in the earlier patch as well), but adjusting the index afterwards is not. The index is already where it needs to be after being pushed forward the first time; recalculating it via actual_x() later will shift it backwards again and put it at the wrong place.
(file #41635)
|
Wed 23 Aug 2017 07:47:53 PM UTC, comment #7:
Ah, I see now: when x get pushed forward, it sits right after the tab, on the first character of a row of text. It is only leftedge and placewewant that have been set correctly, but not x. So x needs to be recalulated from the now correct placewewant. But... the second call in order to adjust x should be to actual_x(), not proper_x(). It makes no difference in the end effect, but it is shorter.
So, your melded patch now works. But... all this recalculation needs to be moved into proper_x() instead of being repeared four times. When x is pushed forward, proper_x must adjust leftedge and placewewant and recalculate index. So leftedge must be passed as an address, placewewant is "global", shifted is not needed, and index is what is returned any way. Something like that.
(file #41632)
|
Wed 23 Aug 2017 06:53:25 PM UTC, comment #6:
Attached the reconciled version that seems to solve both, although more testing is probably needed. (The changes to leftedge_for() are needed so that we don't try to calculate a leftedge outside of softwrap mode, just as in the older patch.)
(file #41631)
|
Wed 23 Aug 2017 06:28:39 PM UTC, comment #5:
And after looking at that patch again, and comparing it with mine, I know why Down (and probably PageDown?) isn't working with that one.
If proper_x() shifts current_x forward, it's not enough to have leftedge and placewewant calculated based on current_x; current_x has to be calculated based on leftedge and placewewant, so if those two change, current_x has to be updated after them. This is why my patch has proper_x() set a flag if it changes current_x, so that current_x is only recalculated under those circumstances.
If we can reconcile my patch here with yours in bug #51827, I think both bugs may be solved.
|
Wed 23 Aug 2017 06:14:25 PM UTC, comment #4:
And I know the patch can probably be simplified (after looking at the patch in bug #51827), but first I'm trying to ensure that it actually works.
|
Wed 23 Aug 2017 06:09:34 PM UTC, comment #3:
Never mind; the do_down() issue was caused by not restoring placewewant properly. It's fixed in the attached new version.
(Although it would still be good to have an explanation on how the shim code works.)
(file #41630)
|
Wed 23 Aug 2017 05:57:01 PM UTC, comment #2:
The patch attached to bug #51827 improves the issue for <Up> near the start of a row. But not for the case of <Down>.
Maddeningly, when pressing <Down> several times (with the reproduction recipe) and then pressing <Up> several times, the status bar will show that Edge and Target are exactly the same when the cursor lands on a row with text, but the cursor gets placed differently. Absurd.
|
Wed 23 Aug 2017 05:55:16 PM UTC, comment #1:
Attached a patch that seems to mostly fix it (based on the last one in bug #51787).
With the patch applied, nano works as you expect with the example you give, but if I run your example with the nano binary instead of README (with its extremely long lines), holding Down makes the cursor move forward more than necessary, and go further and further off course. Holding PageDown doesn't, even when I run your example with both the nano binary and --smooth.
I'm not sure why this happens. I can't understand how the formula in get_edge_and_target() involving the shim variable works in all cases; would you mind providing more of an explanation? If it's miscalculating something with long lines, maybe that's causing this problem, although I'm just guessing at the moment.
(file #41628)
|
Wed 23 Aug 2017 04:15:47 PM UTC, original submission:
To reproduce, run:
stty cols 80 && src/nano --soft -T164 +7,-1 README
Then press <Up> seven times. Notice that whenever the cursor lands in a row with text, the cursor will be in the "desired" column -- in this case column 59 of the screen.
Now press <Down> twenty times. See how the cursor is always at the left edge of the screen or at the start of a row of text, never in the desired column. Now press <Up> twenty times. The cursor is again in the desired column whenever it is in a row of text. So nano hasn't forgotten the target column, it just uses the wrong leftedge when adding the target column.
|