I don't think 12648 is the correct issue for this PR?

There is a discussion with descriptions of this problem for trim operations , but I will move it to a new issue later.

Can you check if it's consistent now?

There is a discussion with descriptions of this problem for trim operations , but I will move it to a new issue later.

Hmm. Without an issue, I don't know what it is that I'm checking...

Sorry for the confusion. Description is here #12658.

As a user, I would not use "trim" operations on partially-selected lines -- I would select full lines with the mouse using line number margin click+drag. Or, if I did partially select some group of lines, I would expect "trim" to operate on the full lines touched by selection. I don't really want to get into the possibly endless debate about how such a thing should work (on partially selected lines), so perhaps @Yaron10 is a better and more thorough tester of this?

As a user, I would not use "trim" operations on partially-selected lines -- I would select full lines with the mouse using line number margin click+drag

So you still have a choice, and if that's your workflow, you can always select all. If we do automatic stretching on whole lines, there may be voices like this:

Why for above example line 1 and 4 are always include, I didn't mark them at all. Such automatic things done behind your back are better to do only if it's really necessary (e.g. when sorting, because taking only part of the first/last line is pointless here).

Right, I was just trying to make the point that I am not the best person to give you the testing on this.

As a user, I would not use "trim" operations on partially-selected lines -- I would select full lines with the mouse using line number margin click+drag. Or, if I did partially select some group of lines, I would expect "trim" to operate on the full lines touched by selection.

👍

so perhaps @Yaron10 is a better and more thorough tester of this?

I think that removing the selection in the following case is wrong.

Leading/Trailing blanks and non-Leading/Trailing blanks are two different "entities".
Let's imagine that Leading/Trailing tabs were represented by a different shape.
For example: ».

Selecting a non-Leading/Trailing tab

does not transform it to:

Why for above example line 1 and 4 are always include,

Line 4 should not be included.

@ArkadiuszMichalski

It's about "Trim Trailling" or "Trim Leading" operation so it's rather ones of Line Operations.
the selection on left should be considered extended as right:

Sorry, for bf34ef0 I have checked only with selection full lines.

@donho

It's about "Trim Trailling" or "Trim Leading" operation so it's rather ones of Line Operations.

Sure, but if I select a fragment (part of a line), then from my perspective this fragment can be treat as independent line (things outside the selection don't bother me). From your graphic, it's as if I just copied this selection into a new document and performed these operations without selection or select all (Ctrl+A).

If I am getting the same result after executing these commands for the left and right of the above examples, then this is correct for me. Of course, that's just my interpretation of partially selection and how commands should process it.

Back to the topic. Auto-expand selection is not that simple, because then appear other borderline cases.

1

Cursor is at the beginning on line 4, so the extension should also include line 4, like this or not?

2 What in this situation, line 2 should be selected? We start selection before CRLF on line 2.

3 We start selection before CRLF on line 2, and finish with cursor at the beginning on line 3.

4 Here I assume that the whole line 4 should be selected (because the first character A was selected)?

What should be done with the last CRLF? Should be a part of this auto-expand selection or not? From the perspective of doTrim() it doesn't matter, but this function is also used with IDM_EDIT_TRIMALL (as first part), and for this command (named Remove Unnecessary Blank and EOL), every CRLF is important. In this case the selection will be processed with doTrim() first, so it matters more what exactly are we doing with selection here.

I've already seen what problems can arise if we try implement auto-expand selection in: #12580.

That's why I came to the conclusion that it's best if the commands operate on the selection exactly made by us. Then we always have the possibility to select exactly what we need. With auto-expand selection we are forced into default behavior, which may not always meet our expectations.
Maybe a solution would be to make, for example, a separate command Expand selection to full lines available from the macro level, and if someone needed it, they could combine the appropriate commands (through macro).

@donho
But if you want auto-expand selection here then we can try, just write what should happen in the above cases.

@ArkadiuszMichalski
No auto-expand selection please. We don't need visual update.
We need only the determination of which lines should apply to trim operation, from the selection.
For the determination, you can see "Move up/down current line" commands' behaviour with the random selection.

@donho

No auto-expand selection please. We don't need visual update.
We need only the determination of which lines should apply to trim operation, from the selection.
For the determination, you can see "Move up/down current line" commands' behaviour with the random selection.

But "Move up/down current line" use auto-expand selection. So visual update occurs. If the command does any operations on characters outside of the selection, the selection should change visually (because we need to somehow inform the user that the selected data has been extended).

Without selection

After move up:

So expand to full line (with CRLF).

For partially selected line:

After move up:

So expand to both full line (with CRLF).

For selecting only CRLF

After move up:

So expand only to full line with contains this one CRLF.

We can do 3 things with partially selected lines:

• auto-expand to full lines (the only question is how to extend this selection for a few extreme cases). Sorting commands do that as well as move up/down line (but that doesn't mean they do the same).
• skip partially selected lines (consider only full selected lines). Remove Empty Lines\Remove Empty Lines (Containing Blank characters) works like that.
• treats partially selected lines as if they were independent lines.

@donho

Can you check the last commit is it better? Now it follows what commands "Move up/down current line" do.

1

Selects lines 2 and 3.

2

Selects lines 2 and 3.

3

Selects lines 2.

4

Selects lines 2 and 3 and 4.

We never expand selection to contain the last CRLF because another command may not work as expected (Remove Unnecessary Blank and EOL). If someone needs it, he has to mark the last CRLF himself. So:

would be:

as you suggested earlier #12655 (comment).

@ArkadiuszMichalski
When multi-selections are present, it takes only the last selection - but I think it's tolerable.

[xml][force compile] - it might be a good practice to not consume too much resource for the earlier stage of PR and get quickly the result, but please don't forget to remove it in the latest commit in order to validate the whole set of compiling.

@donho

When multi-selections are present, it takes only the last selection - but I think it's tolerable.

Yes, but that is due to the limitation of _findReplaceDlg.processAll. Wherever we use this function only the last selection will be processed. At the moment it may look like this, or we may not support multiselect at all (just return).
Multi-selection seems to be difficult to support because changes in one selection affect the next ones, I don't even know if Scintilla somehow properly preserves the selections we have made in such a situation.

[xml][force compile] - it might be a good practice to not consume too much resource for the earlier stage of PR and get quickly the result, but please don't forget to remove it in the latest commit in order to validate the whole set of compiling.

I know, even mention something like that here:
https://github.com/notepad-plus-plus/notepad-plus-plus/wiki/FAQ#other-available-flags
I was just waiting for you to come back to this PR and check out this new approach, because I don't know if you like it in this form.

Now after committing, I will try to adjust these other methods so that they also work on the selection.

Multi-selection seems to be difficult to support because changes in one selection affect the next ones, I don't even know if Scintilla somehow properly saves the selected data in such a situation.

What you do for this situation is, for the first change, note how the document length changes, and adjust the location of the intended second change by this amount. And so on for all the subsequent changes needed.

As I mentioned, it's tolerable for me. So let's take the simplest solution (the current one).

What you do for this situation is, for the first change, note how the document length changes, and adjust the location of the intended second change by this amount. And so on for all the subsequent changes needed.

Right, we can combine in this direction. But I would still expect that _findReplaceDlg.processAll support this firstly. It would be less work than doing everything ourselves every time we need this.