GNOME Bugzilla – Bug 341117
sort plugin should not take the last empty line into account
Last modified: 2019-03-23 20:32:59 UTC
The sort plugin should not take into account the last line of the selection if it's empty. When you select part of the file, you usually start at the begin of a line and go bottom until you've selected the part you're interested in. This causes the last \n to be selected, and then the sort plugin thinks there is an empty line at the end. This line usually goes to the top of the sorted file, and the line following the selection is on the same line as the last sorted entry.
Yep, this is a bug we need to fix. Patches are welcome :)
Created attachment 65073 [details] [review] patch
Created attachment 65075 [details] [review] updated patch Removed the selection stuff and simplified the last empty line handling. I didn't use the same way than bug 132824 comment 10 because it seems more complicated wrt what the plugin actually does.
Created attachment 65093 [details] [review] patch What about something along these lines? Instead of fetching the whole slice of text and then splitting lines out of it, it requests a line at a time to the buffer. I don't think[1] this is slower (at least noticeably) given that GtkTextBuffer stores text in a Btree, so fetching a whole slice does more or less the same thing: it fetches every segment and concatenates them... The patch also avoids allocating lines[N_LINES_IN_THE_BUFFER] instead of lines[SELECTED_LINES] and makes code a bit cleaner. I tested it, but more complete testing is a good idea. [1] more accurate data is welcome, I tried ordering a long buffer and both methods are "equally" slow
It works well... Any reason not to commit right now ?
nud: pbor: if you both have tested the patch and feel confident with the code changes, please commit it to CVS HEAD. Does it handle empty lines in the right way? + if (gtk_text_iter_get_visible_line_offset (&end) == 0) + end_line = MAX (start_line, end_line - 1); + else + gtk_text_iter_forward_line (&end); Are you sure about get_visible_line_offset? + gtk_text_buffer_insert (GTK_TEXT_BUFFER (doc), + &start, + "\n", + -1); What about preserving the right line terminator while getting the line from the buffer so there is not need to add "\n"?
> Does it handle empty lines in the right way? Yes, as far as I can see. > Are you sure about get_visible_line_offset? Well, ordering lines with word wrapping turned on is not very well defined... But you are right, given that the current algo orders real paragraphs and not visible lines, we should use get_line_offset() instead. > What about preserving the right line terminator while getting the line from the > buffer so there is not need to add "\n"? Well, preserving the line terminator is a bit tricky... beside inserting \n is what we do already even without the patch, so IMHO this could be done at a later time.
Then please use get_line_offset and commit. Why do you think preserving line terminator is tricky? We only need to modify the new get_line_slice function so that "end" is the beginning of the next line. Only the last line of the buffer must be managed in a special case by adding a "line terminator" depending on the OS (or if we don't care about it we can add "\n" for the moment).
Ok, I committed my patch after having changed get_visible_line_offset to get_line_offset. The problem with including line terminators is that a trailing line terminator would be added when the selection includes the end iter. Apart from that, while testing I found another pretty major bug in the plugin (which was there already before the patch): when you change the start from column spinbox, the spin box entry takes focus and the selection in the text is lost, so it's not possible to order just a selection.
see bug 342795 for that particular bug closing as fixed