bugGNU nano - Bugs: bug #51828, pressing <Down> does not...

 
 

bug #51828: pressing <Down> does not retain the desired column with overwide tabs

Submitted by:  Benno Schulenberg <bens>
Submitted on:  Wed 23 Aug 2017 04:15:47 PM UTC  
 
Severity: 3 - NormalStatus: Fixed
Assigned to: David Lawrence Ramsey <dolorous>Open/Closed: Closed

Add a New Comment(Rich Markup)
   

You are not logged in

Please log in, so followups can be emailed to you.

 

(Jump to the original submission Jump to the original submission)

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.)

Benno Schulenberg <bens>
Project Administrator
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)

David Lawrence Ramsey <dolorous>
Project AdministratorIn charge of this item.
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)

Benno Schulenberg <bens>
Project Administrator
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).

David Lawrence Ramsey <dolorous>
Project AdministratorIn charge of this item.
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)

David Lawrence Ramsey <dolorous>
Project AdministratorIn charge of this item.
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)

David Lawrence Ramsey <dolorous>
Project AdministratorIn charge of this item.
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)

David Lawrence Ramsey <dolorous>
Project AdministratorIn charge of this item.
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)

David Lawrence Ramsey <dolorous>
Project AdministratorIn charge of this item.
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)

Benno Schulenberg <bens>
Project Administrator
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)

David Lawrence Ramsey <dolorous>
Project AdministratorIn charge of this item.
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.

David Lawrence Ramsey <dolorous>
Project AdministratorIn charge of this item.
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.

David Lawrence Ramsey <dolorous>
Project AdministratorIn charge of this item.
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)

David Lawrence Ramsey <dolorous>
Project AdministratorIn charge of this item.
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.

Benno Schulenberg <bens>
Project Administrator
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)

David Lawrence Ramsey <dolorous>
Project AdministratorIn charge of this item.
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.

Benno Schulenberg <bens>
Project Administrator

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach File(s):
   
   
Comment:
   

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by dolorous (Updated the item)
  • -unavailable- added by bens (Submitted the item)
  • -unavailable- added by bens
  •  

    Do you think this task is very important?
    If so, you can click here to add your encouragement to it.
    This task has 0 encouragements so far.

    Only logged-in users can vote.

     

    Please enter the title of George Orwell's famous dystopian book (it's a date):

     

     

    Follow 15 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Sat 26 Aug 2017 09:24:09 AM UTCbensStatusIn Progress=>Fixed
      Open/ClosedOpen=>Closed
    Thu 24 Aug 2017 08:15:30 PM UTCdolorousAttached File-=>Added 0001-softwrap-fix-vertical-navigation-through-overwide-ta.patch, #41645
    Thu 24 Aug 2017 07:38:25 PM UTCbensAttached File-=>Added do-use-actual-x--but-with-judgment.patch, #41644
    Thu 24 Aug 2017 01:42:28 AM UTCdolorousAttached File-=>Added melded-push-forward-on-tabs-cleaned-up.patch, #41638
    Thu 24 Aug 2017 01:26:25 AM UTCdolorousAttached File-=>Added melded-push-forward-on-tabs-newest.patch, #41637
    Thu 24 Aug 2017 12:31:11 AM UTCdolorousAttached File-=>Added melded-push-forward-on-tabs-newer.patch, #41636
    Wed 23 Aug 2017 11:50:43 PM UTCdolorousAttached File-=>Added melded-push-forward-on-tabs-new.patch, #41635
    Wed 23 Aug 2017 07:47:53 PM UTCbensAttached File-=>Added push-forward-and-recalculate.patch, #41632
      StatusNone=>In Progress
      Assigned toNone=>dolorous
    Wed 23 Aug 2017 06:53:25 PM UTCdolorousAttached File-=>Added melded-push-forward-on-tabs.patch, #41631
    Wed 23 Aug 2017 06:09:34 PM UTCdolorousAttached File-=>Added 0001-moving-recalculate-the-cursor-position-if-current_x-.patch, #41630
    Wed 23 Aug 2017 05:55:16 PM UTCdolorousAttached File-=>Added 0001-moving-recalculate-the-cursor-position-if-current_x-.patch, #41628
    Wed 23 Aug 2017 04:15:47 PM UTCbensCarbon-Copy-=>Added dolorous

    Back to the top


    Powered by Savane 3.1-cleanup1