bugGNU nano - Bugs: bug #55679, replacing a double-width character...

 
 

You are not allowed to post comments on this tracker with your current authentication level.

bug #55679: replacing a double-width character at window's edge shows an extra continuation character

Submitter:  Benno Schulenberg <bens>
Submitted:  Fri 08 Feb 2019 04:51:10 PM UTC
   
 
Severity:  2 - Minor Status:  Fixed
Assigned to:  dolorous Open/Closed:  Closed

Jump to the original submission

Fri 08 Mar 2019 07:05:02 PM UTC, comment #30: 

I thought that maybe the suggested change would affect the softwrap case, but it seems to work fine too.  So...

Pushed to master, commit 43b42246.

Benno Schulenberg <bens>
Group administrator
Thu 07 Mar 2019 08:52:02 PM UTC, comment #29: 

It seems so.  Looking at display_string() again, isdata assumes the string is being displayed as a full line in the edit window, and reserves space both at the beginning and end of the line.  isprompt, on the other hand, reserves space only at the end of the line, which is the behavior we actually need.

Attached a new version of the patch with that change, plus an updated commit message.

(Also, I'll be generous enough to forget that you told me to fuck off, this one time.  Do NOT make me forget again.)

(file #46455)

David Lawrence Ramsey <dolorous>
Group Member
Thu 07 Mar 2019 06:58:44 PM UTC, comment #28: 

The patch causes a regression.  To reproduce, paste "ㄓㄓ123" (without the quotes) into an empty buffer of the patched nano.  Then press <Home> and hold down x until the 3 has gone offscreen (and thus the 2 is overwitten by the > character).  Then type:

M-R  M-R  M-V 003113.* <Enter> aaa <Enter>

And see how " ]ㄓ1>" is highlighted instead of "ㄓㄓ1>".

I suggest to replace "overshoots, FALSE" with "FALSE, overshoots" in the relevant call of display_string().  Is that okay?

Benno Schulenberg <bens>
Group administrator
Wed 06 Mar 2019 07:40:25 PM UTC, comment #27: 

Oh please, stop your vehemence.  Sure, it's good to correct the logic.  But no matter of testing would have found any observable wrong behavior, because this one buglet would have hidden it.

"Trying to argue..."  Oh, fuck off already!

Benno Schulenberg <bens>
Group administrator
Wed 06 Mar 2019 06:50:23 PM UTC, comment #26: 


> The logic may be wrong, but if there is no new observable wrong
> behavior, then commit edc0d628 did not break anything.


If the logic is wrong, then there is no observable wrong behavior yet.  Isn't it better to fix potential problems before they show up, instead of effectively waiting until the last minute?  Taken too far, that approach leads to who knows how many problems just waiting to happen.

> If however that fix does cause new observable wrong behavior (in
> other words: a regression), then the proper course of action is
> to post a new bug with a reproduction recipe.


If the original fix for bug #55638 had been correct, there would be no problem with the patch here applied.  Since that fix was not correct, that led to the problem here.  That is a regression; it just wasn't exposed until now.

If the original fix had gotten a bit more testing before being applied, the problem would have been caught beforehand and some of this mess could have been avoided.  Trying to argue that it isn't really a regression because neither of us saw the problem with it until after you committed it is not helpful.

David Lawrence Ramsey <dolorous>
Group Member
Wed 06 Mar 2019 06:27:30 PM UTC, comment #25: 

A comment that I wrote before comment #23 but that somehow didn't make it through.

The logic may be wrong, but if there is no new observable wrong behavior, then commit edc0d628 did not break anything.  If however that fix does cause new observable wrong behavior (in other words: a regression), then the proper course of action is to post a new bug with a reproduction recipe.

(After the 4.0 release, I will set as_an_at to TRUE at the start of update_line(), which will allow removing the parameter from control_mbrep(), and then change the call of display_string() in update_softwrapped_line() to set isdata to FALSE, so the "&& !ISSET(SOFTWRAP)" can be removed from display_string().  And maybe after that isprompt and isdata can be merged into a single parameter.)

Benno Schulenberg <bens>
Group administrator
Tue 05 Mar 2019 10:37:17 PM UTC, comment #24: 

A combination of paranoia and ease of readability.  It was easier to trace what the code was doing if it left to_col alone and modified a copy of to_col in a separate variable instead.  But using to_col directly does seem to work.  Attached a patch with that simplification added.

(file #46439)

David Lawrence Ramsey <dolorous>
Group Member
Tue 05 Mar 2019 07:58:20 PM UTC, comment #23: 

Why do you need break_col?  Doing:

  break_col = from_col + (to_col - from_col);

is the same as doing:

  break_col = from_col + to_col - from_col;

is the same as:

 break_col = to_col;

Right?

Benno Schulenberg <bens>
Group administrator
Mon 04 Mar 2019 10:07:50 PM UTC, comment #22: 

Attached an updated version of my last patch, resynced against git 37c8232, that sets isdata properly.  Combined with the last patch in comment #9 of bug #55638, everything should work.

(file #46421)

David Lawrence Ramsey <dolorous>
Group Member
Mon 04 Mar 2019 09:47:41 PM UTC, comment #21: 


> The fix for bug #55638 did not break anything in master, as far
> as I can see, it just broke the patch for this bug.


The fix for bug #55638 did break master because its logic is incorrect.  It's not that "it breaks this patch"; it's that "its incorrect logic is exposed by this patch".

> But this is a very minor bug, it doesn't push any characters to
> the wrong row, it's just a minor representation bug. So I'm
> going to let this be for now, I don't want any more changes to
> a central routine like display_string() before the 4.0 release.


If you don't want any more changes to display_string(), it's better to revert the erroneous fix to #55638, since having a half-fix will just lead to other problems.

Alternatively, I do have a simple patch that should fix it properly, which I'll be posting on that bug.  But then I'll need to update this patch to set isdata properly.

David Lawrence Ramsey <dolorous>
Group Member
Sun 03 Mar 2019 09:10:00 AM UTC, comment #20: 

Dang...  I had tried a recipe similar to comment #11 and it had worked okay.  I must have forgotten to type 'make' or something.  :|

The fix for bug #55638 did not break anything in master, as far as I can see, it just broke the patch for this bug.  But this is a very minor bug, it doesn't push any characters to the wrong row, it's just a minor representation bug.  So I'm going to let this be for now, I don't want any more changes to a central routine like display_string() before the 4.0 release.

Benno Schulenberg <bens>
Group administrator
Sat 02 Mar 2019 08:01:01 PM UTC, comment #19: 

The off-by-one error you mentioned in comment #11, which I was still seeing in the version in comment #14, and which I'm not seeing in the new version I posted in comment #15.  Besides, you only mentioned syncing it up with git in comment #14, not any bug fixes in it.

As for bug #55638, the fix for it broke something else, as described in comment #11, but I need to fix the real problem in display_string() and then update my patch to set isdata properly.    (Unless you want to look into fixing the display_string().)  I suppose that discussion does belong in bug #55638, though.

David Lawrence Ramsey <dolorous>
Group Member
Fri 01 Mar 2019 06:53:43 PM UTC, comment #18: 

If bug #55638 is not fully fixed (for the edit window), then please post on that bug.  If however the fix (commit edc0d628) for that bug causes a new issue, then please post a new bug.

Benno Schulenberg <bens>
Group administrator
Fri 01 Mar 2019 06:40:12 PM UTC, comment #17: 

Sorry, I don't follow.  Where is there an off-by-one error in the patch that I posted in comment #14?  With that patch applied to current git, I can't find any mishighlighting of the string to be replaced.

Benno Schulenberg <bens>
Group administrator
Fri 01 Mar 2019 03:11:55 PM UTC, comment #16: 

Addendum:

> As for what spotlight() should do with isdata, it should set it
> to TRUE only when the match extends past the last column of the
> screen, since the match only needs to be trimmed in that case.


Specifically, in spotlight(), the patch should call display_string() with isdata set to whatever overshoots is.  I haven't done this yet because the aforementioned issues with bug #55638 need to be fixed first for it to really work properly.

David Lawrence Ramsey <dolorous>
Group Member
Fri 01 Mar 2019 07:59:06 AM UTC, comment #15: 

Attached an updated version of your patch that should fix the off-by-one in highlighting.  Both our versions were a bit off in that regard.  I had to put break_col back in this version, but it should work.  Also, I changed isdata back to FALSE instead of TRUE in the display_string() call for now, since we're not trimming the string under all circumstances.

How this interacts with bug #55638 is important.  The reason for that bug, and how it interacts with this one, is a deeper problem in display_string():

1. When isprompt is FALSE, isdata is FALSE, and/or softwrap mode is on, display_string() needs to trim a given string to the column at the edge of the screen, since placeholder characters will not be displayed.

2. When isprompt is TRUE, isdata is TRUE, and/or softwrap mode is off, display_string() needs to trim a given string to one column before the edge of the screen, since placeholder characters will be displayed.

The current code in display_string(), in comment #12, is a twisted mess that tries to do both, and so doesn't do either quite correctly.  As for what spotlight() should do with isdata, it should set it to TRUE only when the match extends past the last column of the screen, since the match only needs to be trimmed in that case.

So... some refactoring needs to be done to fix this, in both functions, and I don't have any more time to code until at least the beginning of next week.  Or maybe you have some ideas as to how to do this?

(file #46381)

David Lawrence Ramsey <dolorous>
Group Member
Thu 28 Feb 2019 06:59:20 PM UTC, comment #14: 

Updated patch for current git is attached.

(file #46374)

Benno Schulenberg <bens>
Group administrator
Wed 27 Feb 2019 09:01:52 PM UTC, comment #13: 

I'm back.  I still haven't figured out how to fix this (also because the fix for bug #55773 overlaps with it), but since your patch does seem to fix this problem, and the apparent problem with it is actually caused by a botched fix for bug #55638, that bug should probably be reopened.

(Although, making spotlight_softwrapped() call display_string() with isdata set to TRUE really does need to be part of your fix, since spotlight_softwrapped() is supposed to match spotlight() entirely except for the softwrap handling.)

David Lawrence Ramsey <dolorous>
Group Member
Fri 22 Feb 2019 01:28:52 AM UTC, comment #12: 

The same problem occurs in my version of it.

I know what the problem is, however.  It's related to the change made by commit edc0d62, which fixed the problem it meant to fix, but in the process introduced this one.  See winio.c, lines 1990-1992:


    /* If there is more text than can be shown, make room for the ">". */
    if ((*buf != '\0' || column > beyond) &&
                    (currmenu != MMAIN || (isdata && !ISSET(SOFTWRAP)))) {


What we're running into is the first part of the if clause.  At this point in the code, buf doesn't point to '\0' because we're displaying only part of the line, and there's text after the part we display.  column is exactly equal to beyond because we've displayed everything we need to.  The first part of the condition is met (incorrectly) while the second is not.

I'm not sure what the proper logic is at the moment.  I'm going to be away from the computer for the next few days, and busy upgrading the system for a few days after that, so I won't be able to do much coding until it's all over.

David Lawrence Ramsey <dolorous>
Group Member
Wed 20 Feb 2019 06:56:15 PM UTC, comment #11: 

Oww...  My version of the patch does not work right.  To reproduce, run 'src/nano --ignore README' and type: M-R edit <Enter> xx <Enter>.  See that actually just "edi" gets highlighted.  :|  I don't get it.

Benno Schulenberg <bens>
Group administrator
Tue 19 Feb 2019 04:23:24 PM UTC, comment #10: 

Thanks for testing.

A rationale for the way a match is highlighted that ends at the screen's edge.  It could be done in one of three ways.  Say we are replacing the word "nano".  If the "o" of nano would be in the last column of the screen and there is more text after the word "nano", then these are the three ways:

1) Highlight just "nan" (for me that would be in bright white on magenta) followed by the ">" in reverse video.  But this gives the impression that nano is doing something wrong, that it is matching "nan" instead of "nano" -- or the user might think they made a typo.

2) Highlight "nan>", that is : highlight "nan" plus the continuation character.  But this gives the impression that there is more of the match beyond the screen's edge, out of view.  This is misleading.

3) Highlight the full "nano" and sacrifice the continuation character.  This will give the impression that the line ends there, but when doing replacements I don't think it is often relevant whether there is more text after the match or not.

Of these three imperfect choices, I have opted for the latter one, as I prefer to see the full match, which is especially important when doing regex replacements.

(I'll push the patch when I'm done with the new options stuff.)

Benno Schulenberg <bens>
Group administrator
Mon 18 Feb 2019 10:41:10 PM UTC, comment #9: 

After some testing, your version does seem to work.

David Lawrence Ramsey <dolorous>
Group Member
Fri 15 Feb 2019 06:01:26 PM UTC, comment #8: 

(Updated the patch for the changes in master.)

(file #46275)

Benno Schulenberg <bens>
Group administrator
Thu 14 Feb 2019 08:20:36 PM UTC, comment #7: 

Updated patch that properly covers the case where the match ends in the screen's last column: then we skip showing the continuation character as it would only get in the way.

(file #46267)

Benno Schulenberg <bens>
Group administrator
Thu 14 Feb 2019 08:09:22 PM UTC, comment #6: 

Here's a version that smooths things over a bit.  The extra break_col variable is not needed, I think.

(file #46266)

Benno Schulenberg <bens>
Group administrator
Thu 14 Feb 2019 02:22:49 AM UTC, comment #5: 

Argh.  Forget that version; it's still buggy.

Try this new version.  The only way I could think of to fix the problem was to revamp the logic to be more like spotlight_softwrapped().

As for why, the real reason for the problem in spotlight() is that we're displaying the entire string of highlighted text and then (incorrectly) trying to trim it after the fact.  On the other hand, spotlight_softwrapped() displays only enough of the string of highlighted text to fit on the screen, which is what we actually want to do, barring the final '$'.

Part of the patch also makes the calls to display_string() in spotlight() and spotlight_softwrapped() set isdata to TRUE.  This is correct behavior for text that will be displayed in the edit window.  In spotlight(), it's also needed to make display_string() handle halves of two-column characters.  In spotlight_softwrapped(), it's also needed to properly guarantee that nulls are displayed as ^@.  (This seems to be the case anyway, but it's better to make it explicit.)

Note that the patch is still rough, and mixes a few changes that you'd probably want split up.  However, I'm putting it out as is because I don't have any more time to work on it for the next few days, and because it needs testing, given how tricky this problem is.

(file #46250)

David Lawrence Ramsey <dolorous>
Group Member
Wed 13 Feb 2019 09:29:14 PM UTC, comment #4: 

How about the attached new version?

(I'm aware that having two #ifdefs is ugly, but I can't think of a way to simplify it right now, and it does seem to work, accounting for the cases you brought up.)

(file #46249)

David Lawrence Ramsey <dolorous>
Group Member
Wed 13 Feb 2019 06:34:05 PM UTC, comment #3: 

Thanks.  The fix works when the double-width character is just before the screen's edge, but not when it straddles the screen's edge.  That is: when you type one more space before the fi so that the shown text looks like "fi$", and then do the replacement, it highlights "fi>", and there is no continuation character.  However, if you had typed an <Enter> after the "xxx", then the highlighted continuation character "$" is wrongly shown on the next row.  :|

Benno Schulenberg <bens>
Group administrator
Tue 12 Feb 2019 07:54:11 PM UTC, comment #2: 

Attached an updated version of the patch, which is resynced against git e7557a9 and fixes an inaccurate  comment.

(file #46244)

David Lawrence Ramsey <dolorous>
Group Member
Mon 11 Feb 2019 11:23:44 PM UTC, comment #1: 

This occurs because spotlight() contains no logic to handle double-width characters.  All it does is move back one character and display a '$', not accounting for a potential '>' beforehand.

The attached experimental patch against 5618189 seems to fix this, by swiping a bit of logic from display_string() to handle double-width characters.

(Ideally, it would be using display_string() to accomplish this, but I'm not sure how to modify display_string() for this case without breaking something else.)

(file #46240)

David Lawrence Ramsey <dolorous>
Group Member
Fri 08 Feb 2019 04:51:10 PM UTC, original submission:  

To reproduce, run 'nano --ignore --nowrap' and paste "fiㄓxxx" into the empty buffer.  Then press <Home> and hold <Space> until the cactus has just reached the window's edge and its left half has been replaced with ">".  Then type: M-R M-R fi.* <Enter> foo <Enter>.  See how "fi$" is highlighted before the final "$", instead of "fi>$".

The buglet is old, since at least 2.2.6.

Benno Schulenberg <bens>
Group administrator

 

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

 

Depends on the following items: None found

Items that depend on this one: None found

 

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

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

     

    Follow 16 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2019-03-25 bens Open/ClosedOpen Closed
    2019-03-08 bens StatusIn Progress Fixed
    2019-03-07 dolorous Attached File- Added 0001-display-properly-handle-double-width-characters-when.patch, #46455
    2019-03-05 dolorous Attached File- Added 0001-display-properly-handle-double-width-characters-when.patch, #46439
    2019-03-04 dolorous Attached File- Added 0001-display-properly-handle-double-width-characters-when.patch, #46421
    2019-03-01 dolorous Attached File- Added 0001-display-properly-handle-double-width-characters-when.patch, #46381
    2019-02-28 bens Attached File- Added 0001-display-properly-handle-double-width-characters-when.patch, #46374
    2019-02-15 bens Attached File- Added 0001-display-properly-handle-double-width-characters-when.patch, #46275
    2019-02-14 bens Attached File- Added 0001-display-properly-handle-double-width-characters-when.patch, #46267
    2019-02-14 bens Attached File- Added 0001-display-properly-handle-double-width-characters-when.patch, #46266
        StatusNone In Progress
        Assigned toNone dolorous
    2019-02-14 dolorous Attached File- Added 0001-display-fix-spotlighting-of-halves-of-double-width-c.patch, #46250
    2019-02-13 dolorous Attached File- Added 0001-display-fix-spotlighting-of-halves-of-double-width-c.patch, #46249
    2019-02-12 dolorous Attached File- Added 0001-display-fix-spotlighting-of-halves-of-double-width-c.patch, #46244
    2019-02-11 dolorous Attached File- Added 0001-display-fix-spotlighting-of-halves-of-double-width-c.patch, #46240

    Back to the top

    Powered by Savane 3.13-4448.
    Corresponding source code