bugGNU nano - Bugs: bug #49812, the softwrap routines include much...

 
 

bug #49812: the softwrap routines include much duplicate/unclear code

Submitted by:  David Lawrence Ramsey <dolorous>
Submitted on:  Fri 09 Dec 2016 02:55:47 PM UTC  
 
Severity: 3 - NormalStatus: Wont Fix
Assigned to: NoneOpen/Closed: Closed

Add a New Comment(Rich Markup)
   

You are not logged in

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

 

Thu 16 Mar 2017 07:38:34 PM UTC, comment #5:

This has been obsoleted by the softwrap overhaul work.

Benno Schulenberg <bens>
Project Administrator
Tue 13 Dec 2016 04:05:36 PM UTC, comment #4:

I've pushed the eighth patch (the eliding of two variables), git c9f743f6. I'm not sure I want to make the rest of the changes yet.

Benno Schulenberg <bens>
Project Administrator
Tue 13 Dec 2016 01:15:03 AM UTC, comment #3:

Okay, and thank you. The changes needed for patches two through four are clear enough, and easily done. Attached as softwrap cleanup V2, patches 1 (containing the changes for two and three) and 2 (containing the changes for four).

Patch five obviously needs some explanation. The reason I did it that way is because, just as get_page_start() gets the location of the first visible column of the current line, get_softwrap_page_start() should get the first visible column of the current softwrapped portion of the current line (column / editwincols is the integer number of wraps at column, multiplied by editwincols again to get the value of the first column of the wrapped line containing column).

For example, say that the terminal is 80 columns wide, the current line is 229 columns wide (starting from zero), and softwrap is on. The three softwrapped lines go from column 0 to column 79, column 80 to column 159, and column 160 to column 229. If the cursor is on the first softwrapped line, the function will return 0; if the cursor is on the second softwrapped line, the function will return 80, and if the cursor is on the third softwrapped line, the function will return 160.

How do I make this function clearer by your standards? Should I change the name to something more descriptive, like a shorter version of first_visible_column_of_the_softwrapped_line_at_this_column()?

(Whether the function stays or not, if the behavior in bug 49100 is implemented, which I will have a proposal for momentarily, nano will have to use similar logic to adapt to the corner case where (a) softwrapping makes the line at edittop take up more than one virtual line, and (b) scrolling through virtual lines moves the edit window down just enough to where the top line of edittop is no longer visible, but other lines of it are. At this point, edittop->data[0] will be offscreen, so the assumption throughout the code that edittop->data[0] points to the first visible character in the edit window will be broken. To work around this, there will have to be a size_t edittop_begin_x or something similar, set to the first visible column of edittop->data.)

(file #39210, file #39211)

David Lawrence Ramsey <dolorous>
Project Administrator
Sat 10 Dec 2016 04:43:12 PM UTC, comment #2:

Hi David,

Thanks for all that. However... the first patch is not cleanup, it is a bugfix. I've reported bug #49821 for that, and will apply your patch for it soon.

I don't see what the second patch brings us -- except lines that stay within the maximum width. :) The patch also changes the meaning of the orig_lenpt variable, so I think the second and third patch should be a single one: change name and content at the same time.

The fourth patch I don't like: the name of the function is wrong (y is a position, not a number of lines), and almost all the calls do use function within function: actual_softwrap_y(strlenpt(...)). I would change that to: number_of_wrappings(...). The few calls that don't include strlenpt() can continue to use "/ editwincols".

The fifth patch... only makes things /less/ clear.

The sixth... I don't want to consider yet, because I'm going to trim down get_totsize() soon.

The seventh... will be fine when the preceding ones have made it.

The eighth patch looks good, but I will have to test and verify.

Benno Schulenberg <bens>
Project Administrator
Fri 09 Dec 2016 06:11:26 PM UTC, comment #1:

Attached an eighth patch in the sequence (dependent on the patch in bug 49816's being applied first), to simplify a check for softwrapped lines in the line numbering routine.

(file #39188)

David Lawrence Ramsey <dolorous>
Project Administrator
Fri 09 Dec 2016 02:55:47 PM UTC, original submission:

I've been looking into the softwrap code lately (thanks for applying my previous patches, by the way), and I think I've figured out how to make some parts of it better, before even considering making any adjustments to how it works, as suggested in bug 49100.

The attached patch sequence against current git adjusts the code so that, hopefully, none of the functionality changes, but it's easier to understand what the code is actually doing. (It's currently in a zip file to avoid any problems with having seven attachments, although the first three are really cleanups to prepare for the other four.)

For example, the "strlenpt() / editwincols" formula repeated all over the place is a mess; such logic is now moved into its own function. Also, the "strlenpt % editwincols" formula in update_line() is handled in its own function (used in only one place for now, but should probably be used elsewhere eventually).

Note that not all of the softwrap code is cleaned up by these, as I don't quite understand all of it, and I'm leaving the parts I don't understand alone for now (notably, the computed number of lines to scroll in do_right()). So it's a work in progress.

David Lawrence Ramsey <dolorous>
Project Administrator

 

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

Attach File(s):
   
   
Comment:
   

Attached Files
file #39190:  0008-Softwrap-code-cleanup-part-8.patch added by dolorous (2KiB - text/x-patch - fixed to compile with NANO_TINY and check for softwrap mode)
file #39184:  softwrap-patches.zip added by dolorous (9KiB - binary/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -unavailable- added by bens (Posted a comment)
  • -unavailable- added by dolorous (Submitted the item)
  •  

    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 11 latest changes.

    Date Changed By Updated Field Previous Value => Replaced By
    Thu 16 Mar 2017 07:38:34 PM UTCbensStatusNone=>Wont Fix
      Open/ClosedOpen=>Closed
      SummaryThe softwrap routines include much duplicate/unclear code=>the softwrap routines include much duplicate/unclear code
    Tue 13 Dec 2016 01:15:03 AM UTCdolorousAttached File-=>Added 0001-Softwrap-code-cleanup-V2-part-1.patch, #39210
      Attached File-=>Added 0002-Softwrap-code-cleanup-V2-part-2.patch, #39211
    Fri 09 Dec 2016 06:23:49 PM UTCdolorousAttached File-=>Added 0008-Softwrap-code-cleanup-part-8.patch, #39190
    Fri 09 Dec 2016 06:23:28 PM UTCdolorousAttached File#39189=>Removed
    Fri 09 Dec 2016 06:19:50 PM UTCdolorousAttached File-=>Added 0008-Softwrap-code-cleanup-part-8.patch, #39189
    Fri 09 Dec 2016 06:19:29 PM UTCdolorousAttached File#39188=>Removed
    Fri 09 Dec 2016 06:11:26 PM UTCdolorousAttached File-=>Added 0008-Softwrap-code-cleanup-part-8.patch, #39188
    Fri 09 Dec 2016 02:55:47 PM UTCdolorousAttached File-=>Added softwrap-patches.zip, #39184

    Back to the top


    Powered by Savane 3.1-cleanup1