when executing the command "Select Line" (e.g. assigning it a keyboard shortcut), Geany still remembers the last X incorrectly, so Shift-Down and other keys don't work well. this PR sets the last X correctly for this case and many other cases.
the caret/anchor is also swapped for select all. previously the selection was not in the natural forward direction and therefore very strange. almost every other editor including in this textarea in most browsers has it the correct way. You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/4093
-- Commit Summary --
* send SCI_SWAPMAINANCHORCARET after selecting all for a more natural selection order * send SCI_CHOOSECARETX after setting selection
-- File Changes --
M src/sciwrappers.c (5)
-- Patch Links --
https://github.com/geany/geany/pull/4093.patch https://github.com/geany/geany/pull/4093.diff
Hi, just following up with this, any feedback on this? Thanks! @techee
@limwz01 Thanks for the patches.
If I'm not mistaken, you are making this change only for the select line case, am I right? I think for line selection, updating "last X" makes sense; however, I think it shouldn't be done universally in the `sci_` wrappers. They are accessible to plugins and plugins can use them for various purposes, possibly combining several `sci_` functions, and we cannot be sure if they really want to update "last X".
So I'd suggest moving the "last X" update to the actual line selection implementation, i.e. `editor_select_lines()`.
Regarding `SCI_SWAPMAINANCHORCARET`, I'm not sure - other editors may do it this way, on the other hand it has been like this in Geany since the beginning and I can imagine some people got used to this behavior so maybe better not to break it for them.
So I'd suggest moving the "last X" update to the actual line selection implementation, i.e. editor_select_lines().
Now by looking at the code (briefly), I'm not even sure if `editor_select_lines()` is the right function - this function is used for various features and possibly not all of them should update "last X" (like e.g. deleting current line).
@techee you can try deleting the current line yourself in the current Geany since it is already assigned a keyboard shortcut by default. note that the cursor already moves to the start anyway, so updating the last X is a natural thing to do
about the updating of last X for every active command-based selection change, I think it's acceptable because it is natural. the only reason to have a last X in the first place is when you are temporarily extending the selection by cursoring up and down using the arrow keys and you don't want your caret point location to keep changing randomly.
@techee you can try deleting the current line yourself in the current Geany since it is already assigned a keyboard shortcut by default. note that the cursor already moves to the start anyway, so updating the last X is a natural thing to do. and actually there is one command that it eventually runs which happens to update the last X.
I'm not questioning the "select line" case - as I said, for that feature it makes sense. Notice, however, that your implementation sets "last X" to 0 for all other uses of the functions you modified and this is not OK. Your patch should only affect the "select line" case and not other cases. IMO, to achieve this,
https://github.com/geany/geany/blob/3eeb361b6a2e29cdbc473cf4dc277cbe5b23283b...
should be replaced with ```C if (doc != NULL) { editor_select_lines(doc->editor, FALSE); SSM(doc->editor->sci, SCI_CHOOSECARETX, 0, 0); }
``` (completely untested). Then it's guaranteed that the change won't have some negative impact on other Geany features.
about the updating of last X for every active command-based selection change, I think it's acceptable because it is natural. the only reason to have a last X in the first place is when you are cursoring up and down using the arrow keys (or extending the selection) and you don't want your caret point location to keep changing randomly. the last X is not even something the user is actively aware of and keeps track of, so I don't consider it a major change. there is also nothing inefficient about updating the last X as it is just copying an integer so there is no reason not to.
The functions from `sciwrappers.c/h` aren't the right place to do that. As the name of the file suggests, these are just thin wrappers around Scintilla which are meant to offer a nicer alternative to the Scintilla API but nothing more. These functions are exposed to plugins and modifying them means breaking Geany API.
No, it's not just about line selection, it's also word selection, which is <shift><alt>w by default. you can try it out and see how unnatural it feels right now.
If there are more use cases for that, they should be addressed on a case by case basis but not by modifying scintilla wrappers universally.
You mentioned "delete line" as one of the other uses of `editor_select_lines` as your justification for what is being affected. Hence I'm just pointing out that even in your example, updating the last X doesn't make a difference.
@techee I just reread you objection again and I think you misunderstood what `SCI_CHOOSECARETX` does.
From https://scintilla.org/ScintillaDoc.html#SCI_CHOOSECARETX (emphasis mine):
SCI_CHOOSECARETX Scintilla remembers the x value of the last position horizontally moved to explicitly by the user and this value is then used when moving vertically such as by using the up and down keys. This message sets the **current x position of the caret** as the remembered value.
@techee I just reread your objection again and I think you misunderstood what SCI_CHOOSECARETX does.
I think I understand very well what it does.
But you don't seem to understand that if you want to modify the behavior of certain actions where updating SCI_CHOOSECARETX makes sense (e.g. selecting the current line), you should modify those actions _only_ and not to do it universally in scintilla wrappers for SCI_SETSELECTIONSTART, SCI_SETSELECTIONEND, etc. - these functions are exposed to plugins and plugins expect these functions to do exactly the same thing as what Scintilla does - nothing extra.
Consider for instance a plugin wants to implement a "copy current line to clipboard" feature and decides to do it this way: 1. Remembers current caret position 2. Uses `sci_set_selection_start()` and `sci_set_selection_end()` to make a selection of the current line 3. Copies the selection to clipboard 4. Clears the selection 5. Restores the caret position from (1)
To the user, after performing such an operation, it appears as if no change in the editor was made - the caret is at the same position, just the line is copied to the clipboard. Yet, if what you propose with your patch is implemented, SCI_CHOOSECARETX would be set to 0 which is something the user doesn't expect. And the plugin author doesn't expect this neither.
Even in your example, in step 5, restoring the caret position with sci_set_selection_start/sci_set_selection_end/sci_set_selection and will automatically restore the last X again, if you use my modifications. Typically this should actually be achieved by sci_set_target_start and sci_set_target_end. A lot of selection functions already alter the last X, for example `sci_replace_sel`.
Closed #4093.
I think it's about time to end the discussion.
As Neil tried to explain to you in https://sourceforge.net/p/scintilla/feature-requests/1536/, your patch isn't a good idea. I tried to do the same here without much success. We are not going to break our API because of that and we are not going to reset "last X" to 0 when making an arbitrary selection using the API.
I even doubt that SCI_CHOOSECARETX should get set to 0 after selecting a line which is the only use case for this patch you mentioned so far. I imagine that users use this to delete the current line (i.e. select line, delete it). With your patch, this will throw you to the beginning of the line for subsequent navigation using arrows. I think it's much more useful to be able to return to "last X" afterwards.
I'm closing this PR.
Oh well, I don't really care now because I have my own fork of Geany. But I find it quite unreasonable that so many of the scintilla wrappers already update the last X and for this case where it actually matters, you don't want to. It's not like it's even an important thing to keep last X being different from the caret position.
we are not going to reset "last X" to 0 when making an arbitrary selection using the API
again, you say "reset.. to 0". let me say once again that my modifications do NOT reset it to a fixed value of 0. it resets it to the current x position of the **caret**.
But I find it quite unreasonable that so many of the scintilla wrappers already update the last X and for this case where it actually matters, you don't want to.
Only `sci_set_current_position()` does.
again, you say "reset.. to 0". let me say once again that my modifications do NOT reset it to a fixed value of 0. it resets it to the current x position of the caret.
Correct, 0 is not universal.
I wrote this with the "select current line" scenario in mind where it resets the value to 0 which, as I wrote, might not be what users expect. You prefer that after it the value is reset to 0; others may prefer being able to return where they were.
`sci_replace_sel` also does, not in Geany code but in Scintilla's own code. and hence so many other functions also set last X.
I imagine that users use this to delete the current line (i.e. select line, delete it). With your patch, this will throw you to the beginning of the line for subsequent navigation using arrows.
At first what you said makes a bit of sense but not after thinking about it more carefully. The fact is that if you deleted the line already why would you want to go back to the last position you were at on that line?
@limwz01 Please find someone else to continue with this discussion. I just don't have time for this. I believe both I and Neil gave enough arguments why we think this is not a good idea.
I'm unsubscribing from this issue.
github-comments@lists.geany.org