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