Hello all.
I have several suggestions and questions about certain line operations implemented in Geany.
1) Recently I found that "move lines up/down" command does not work properly for the last line not ending with a newline. You can easily check it yourself. Couple of minutes ago I made a pull request [1] with an implementation of `editor.c:move_lines()` which handles the problem. Dear unknown someone, please review and pull if it's okay.
[1]: https://github.com/geany/geany/pull/21
2) I want to raise a question, do we need a "join lines" command? This command would be a companion of the existing "reflow paragraph" command, but in contrast with the latter, it would only join lines, but not split them.
This command may be useful when, let's say, you have a document created by someone else who sticks with line breaking and inserts \n at column 80. Suppose that you prefer using line wrapping instead and want to remove those \n in a peace of a document which you're editing. The new command would help you a bit.
Implementation of the new "join lines" command could use the bits of code already written for "reflow paragraph" (though, those bits need to be extracted/refactored first). Moreover, I already implemented it and, if the new command seems useful to anyone, I can put my implementation in a pull request.
3) Here there should have been a point about some bug reports we have in Geany bug tracker, referring to "reflow paragraph" glitches. As this message is already too long, I decided to designate a separate message for that (later).
Thank you for reading :) Please write your thoughts about 1) and 2)
-- Best regards, Eugene.
Le 05/02/2012 13:51, Eugene Arshinov a écrit :
Hello all.
Hey Eugene,
I have several suggestions and questions about certain line operations implemented in Geany.
- Recently I found that "move lines up/down" command does not work
properly for the last line not ending with a newline. You can easily check it yourself. Couple of minutes ago I made a pull request [1] with an implementation of `editor.c:move_lines()` which handles the problem. Dear unknown someone, please review and pull if it's okay.
I know this problem, but it needed a so big code refactoring it hurts (I want as a proof the fact your rewrite is... huge), so... I just postponed it. Ok, shame on me.
I haven't reviewed the new code yet, but I'll do. Just as a note, Scintilla has a command to do that that suffers (suffered?) of the exact same bug we had. Maybe it'd be good to fix their copy and use the SCI message in place of our code.
- I want to raise a question, do we need a "join lines" command? This
command would be a companion of the existing "reflow paragraph" command, but in contrast with the latter, it would only join lines, but not split them.
This command may be useful when, let's say, you have a document created by someone else who sticks with line breaking and inserts \n at column 80. Suppose that you prefer using line wrapping instead and want to remove those \n in a peace of a document which you're editing. The new command would help you a bit.
Implementation of the new "join lines" command could use the bits of code already written for "reflow paragraph" (though, those bits need to be extracted/refactored first). Moreover, I already implemented it and, if the new command seems useful to anyone, I can put my implementation in a pull request.
Not sure I see a use case for this (read: I probably wouldn't use it if it simply removes the EOLs), but why not if some wants it. But maybe Scintilla already have a message for it and thus we'd only need to use it?
Regards, Colomban
- Here there should have been a point about some bug reports we have
in Geany bug tracker, referring to "reflow paragraph" glitches. As this message is already too long, I decided to designate a separate message for that (later).
Thank you for reading :) Please write your thoughts about 1) and 2)
-- Best regards, Eugene. _______________________________________________ Geany-devel mailing list Geany-devel@uvena.de https://lists.uvena.de/cgi-bin/mailman/listinfo/geany-devel
[...]
Not sure I see a use case for this (read: I probably wouldn't use it if it simply removes the EOLs), but why not if some wants it. But maybe Scintilla already have a message for it and thus we'd only need to use it?
That would be SCI_LINESJOIN ??
Cheers Lex
On Sun, 05 Feb 2012 20:50:38 +0100 Colomban Wendling lists.ban@herbesfolles.org wrote:
Le 05/02/2012 13:51, Eugene Arshinov a écrit :
Hello all.
Hey Eugene,
I have several suggestions and questions about certain line operations implemented in Geany.
- Recently I found that "move lines up/down" command does not work
properly for the last line not ending with a newline. You can easily check it yourself. Couple of minutes ago I made a pull request [1] with an implementation of `editor.c:move_lines()` which handles the problem. Dear unknown someone, please review and pull if it's okay.
I know this problem, but it needed a so big code refactoring it hurts (I want as a proof the fact your rewrite is... huge), so... I just postponed it. Ok, shame on me.
No problem. As the changes I made are (almost?) a complete rewrite, I would suggest viewing the differences side by side instead of `diff` format. That will be easier. Though, the resulting function indeed became longer than the previous version...
I haven't reviewed the new code yet, but I'll do. Just as a note, Scintilla has a command to do that that suffers (suffered?) of the exact same bug we had. Maybe it'd be good to fix their copy and use the SCI message in place of our code.
Uhh, I didn't know that… I'll take a look later.
- I want to raise a question, do we need a "join lines" command?
This command would be a companion of the existing "reflow paragraph" command, but in contrast with the latter, it would only join lines, but not split them.
This command may be useful when, let's say, you have a document created by someone else who sticks with line breaking and inserts \n at column 80. Suppose that you prefer using line wrapping instead and want to remove those \n in a peace of a document which you're editing. The new command would help you a bit.
Implementation of the new "join lines" command could use the bits of code already written for "reflow paragraph" (though, those bits need to be extracted/refactored first). Moreover, I already implemented it and, if the new command seems useful to anyone, I can put my implementation in a pull request.
Not sure I see a use case for this (read: I probably wouldn't use it if it simply removes the EOLs), but why not if some wants it. But maybe Scintilla already have a message for it and thus we'd only need to use it?
As Lex already wrote, there is SCI_LINESJOIN. And that's the command which is already used in Geany to implement "reflow paragraph" (to be more precise, two commands are used: LINESJOIN and LINESSPLIT).
By the way, here is a related discussion: "[Geany-devel] Support SCI_LINESJOIN and SCI_LINESSPLIT (patch included)" [1]. The first message is authored by me :)
[1]: http://lists.uvena.de/geany-devel/2009-July/000834.html
-- Best regards, Eugene.
On Mon, 6 Feb 2012 11:55:25 +0400 Eugene Arshinov earshinov@gmail.com wrote:
On Sun, 05 Feb 2012 20:50:38 +0100 Colomban Wendling lists.ban@herbesfolles.org wrote:
Le 05/02/2012 13:51, Eugene Arshinov a écrit :
Hello all.
[snip]
- I want to raise a question, do we need a "join lines" command?
This command would be a companion of the existing "reflow paragraph" command, but in contrast with the latter, it would only join lines, but not split them.
This command may be useful when, let's say, you have a document created by someone else who sticks with line breaking and inserts \n at column 80. Suppose that you prefer using line wrapping instead and want to remove those \n in a peace of a document which you're editing. The new command would help you a bit.
Implementation of the new "join lines" command could use the bits of code already written for "reflow paragraph" (though, those bits need to be extracted/refactored first). Moreover, I already implemented it and, if the new command seems useful to anyone, I can put my implementation in a pull request.
Not sure I see a use case for this (read: I probably wouldn't use it if it simply removes the EOLs), but why not if some wants it. But maybe Scintilla already have a message for it and thus we'd only need to use it?
As Lex already wrote, there is SCI_LINESJOIN. And that's the command which is already used in Geany to implement "reflow paragraph" (to be more precise, two commands are used: LINESJOIN and LINESSPLIT).
By the way, here is a related discussion: "[Geany-devel] Support SCI_LINESJOIN and SCI_LINESSPLIT (patch included)" [1]. The first message is authored by me :)
I created pull request #26 [2] containing the implementation of the new "Join lines" command so that it won't get lost.
[2]: https://github.com/geany/geany/pull/26
-- Best regards, Eugene.
Le 23/02/2012 11:08, Eugene Arshinov a écrit :
On Mon, 6 Feb 2012 11:55:25 +0400 Eugene Arshinov earshinov@gmail.com wrote:
On Sun, 05 Feb 2012 20:50:38 +0100 Colomban Wendling lists.ban@herbesfolles.org wrote:
Le 05/02/2012 13:51, Eugene Arshinov a écrit :
Hello all.
[snip]
- I want to raise a question, do we need a "join lines" command?
This command would be a companion of the existing "reflow paragraph" command, but in contrast with the latter, it would only join lines, but not split them.
This command may be useful when, let's say, you have a document created by someone else who sticks with line breaking and inserts \n at column 80. Suppose that you prefer using line wrapping instead and want to remove those \n in a peace of a document which you're editing. The new command would help you a bit.
Implementation of the new "join lines" command could use the bits of code already written for "reflow paragraph" (though, those bits need to be extracted/refactored first). Moreover, I already implemented it and, if the new command seems useful to anyone, I can put my implementation in a pull request.
Not sure I see a use case for this (read: I probably wouldn't use it if it simply removes the EOLs), but why not if some wants it. But maybe Scintilla already have a message for it and thus we'd only need to use it?
As Lex already wrote, there is SCI_LINESJOIN. And that's the command which is already used in Geany to implement "reflow paragraph" (to be more precise, two commands are used: LINESJOIN and LINESSPLIT).
By the way, here is a related discussion: "[Geany-devel] Support SCI_LINESJOIN and SCI_LINESSPLIT (patch included)" [1]. The first message is authored by me :)
I created pull request #26 [2] containing the implementation of the new "Join lines" command so that it won't get lost.
Good thing, since I finally took a look at it :) A few remarks on the PR
Cheers, Colomban
-- Best regards, Eugene.
Geany-devel mailing list Geany-devel@uvena.de https://lists.uvena.de/cgi-bin/mailman/listinfo/geany-devel
Hi there.
Quick overview - I posted a pull request [3] which removes `move_lines` function and uses commands already available in Scintilla. See below.
On Sun, 05 Feb 2012 20:50:38 +0100 Colomban Wendling lists.ban@herbesfolles.org wrote:
Le 05/02/2012 13:51, Eugene Arshinov a écrit :
Hello all.
Hey Eugene,
I have several suggestions and questions about certain line operations implemented in Geany.
- Recently I found that "move lines up/down" command does not work
properly for the last line not ending with a newline. You can easily check it yourself. Couple of minutes ago I made a pull request [1] with an implementation of `editor.c:move_lines()` which handles the problem. Dear unknown someone, please review and pull if it's okay.
I know this problem, but it needed a so big code refactoring it hurts (I want as a proof the fact your rewrite is... huge), so... I just postponed it. Ok, shame on me.
I haven't reviewed the new code yet, but I'll do. Just as a note, Scintilla has a command to do that that suffers (suffered?) of the exact same bug we had. Maybe it'd be good to fix their copy and use the SCI message in place of our code.
[snip]
I didn't find those Scintilla commands (SCI_MOVESELECTEDLINES{UP,DOWN} to be precise) because they are not mentioned in the official Scintilla documentation [1], though they are of course mentioned in Scintilla.h. For completeness, here is the feature request for Scintilla where those commands came from - [2].
The commands indeed work similarly to our own `move_lines` function. I posted pull request #24 [3] which removes `move_lines` function and leverages the commands. I hope it can be committed so that I can switch my effort of improving "Move lines" feature from Geany to Scintilla.
[1]:http://www.scintilla.org/ScintillaDoc.html [2]:https://sourceforge.net/tracker/?func=detail&aid=3304850&group_id=2439&atid=352439 [3]:https://github.com/geany/geany/pull/24
-- Best regards, Eugene.
Le 19/02/2012 08:02, Eugene Arshinov a écrit :
Hi there.
Hi Eugene,
Quick overview - I posted a pull request [3] which removes `move_lines` function and uses commands already available in Scintilla. See below.
On Sun, 05 Feb 2012 20:50:38 +0100 Colomban Wendling lists.ban@herbesfolles.org wrote:
Le 05/02/2012 13:51, Eugene Arshinov a écrit :
Hello all.
Hey Eugene,
I have several suggestions and questions about certain line operations implemented in Geany.
- Recently I found that "move lines up/down" command does not work
properly for the last line not ending with a newline. You can easily check it yourself. Couple of minutes ago I made a pull request [1] with an implementation of `editor.c:move_lines()` which handles the problem. Dear unknown someone, please review and pull if it's okay.
I know this problem, but it needed a so big code refactoring it hurts (I want as a proof the fact your rewrite is... huge), so... I just postponed it. Ok, shame on me.
I haven't reviewed the new code yet, but I'll do. Just as a note, Scintilla has a command to do that that suffers (suffered?) of the exact same bug we had. Maybe it'd be good to fix their copy and use the SCI message in place of our code.
[snip]
I didn't find those Scintilla commands (SCI_MOVESELECTEDLINES{UP,DOWN} to be precise) because they are not mentioned in the official Scintilla documentation [1], though they are of course mentioned in Scintilla.h. For completeness, here is the feature request for Scintilla where those commands came from - [2].
It is in the docs: http://www.scintilla.org/ScintillaDoc.html#SCI_MOVESELECTEDLINESUP
The commands indeed work similarly to our own `move_lines` function. I posted pull request #24 [3] which removes `move_lines` function and leverages the commands. I hope it can be committed so that I can switch my effort of improving "Move lines" feature from Geany to Scintilla.
Looks fine to -- apart of course it doesn't fix the initial problem yet. At least so there would be only one copy of the functionality.
So just to be sure we understand each other: I drop your previous pull request (#21), apply this one (#24) and wait for your patch to be in Scintilla?
Regards, Colomban
-- Best regards, Eugene.
On Sun, 19 Feb 2012 17:24:00 +0100 Colomban Wendling lists.ban@herbesfolles.org wrote:
Le 19/02/2012 08:02, Eugene Arshinov a écrit :
Hi there.
Hi Eugene,
Quick overview - I posted a pull request [3] which removes `move_lines` function and uses commands already available in Scintilla. See below.
On Sun, 05 Feb 2012 20:50:38 +0100 Colomban Wendling lists.ban@herbesfolles.org wrote:
Le 05/02/2012 13:51, Eugene Arshinov a écrit :
Hello all.
Hey Eugene,
I have several suggestions and questions about certain line operations implemented in Geany.
- Recently I found that "move lines up/down" command does not
work properly for the last line not ending with a newline. You can easily check it yourself. Couple of minutes ago I made a pull request [1] with an implementation of `editor.c:move_lines()` which handles the problem. Dear unknown someone, please review and pull if it's okay.
I know this problem, but it needed a so big code refactoring it hurts (I want as a proof the fact your rewrite is... huge), so... I just postponed it. Ok, shame on me.
I haven't reviewed the new code yet, but I'll do. Just as a note, Scintilla has a command to do that that suffers (suffered?) of the exact same bug we had. Maybe it'd be good to fix their copy and use the SCI message in place of our code.
[snip]
I didn't find those Scintilla commands (SCI_MOVESELECTEDLINES{UP,DOWN} to be precise) because they are not mentioned in the official Scintilla documentation [1], though they are of course mentioned in Scintilla.h. For completeness, here is the feature request for Scintilla where those commands came from - [2].
It is in the docs: http://www.scintilla.org/ScintillaDoc.html#SCI_MOVESELECTEDLINESUP
Stupid me. Probably I searched for MOVELINES, not MOVESELECTEDLINES
The commands indeed work similarly to our own `move_lines` function. I posted pull request #24 [3] which removes `move_lines` function and leverages the commands. I hope it can be committed so that I can switch my effort of improving "Move lines" feature from Geany to Scintilla.
Looks fine to -- apart of course it doesn't fix the initial problem yet. At least so there would be only one copy of the functionality.
So just to be sure we understand each other: I drop your previous pull request (#21), apply this one (#24) and wait for your patch to be in Scintilla?
Yes. But I don't promise that I'll post a request to Scintilla *soon*…
Le 19/02/2012 20:01, Eugene Arshinov a écrit :
On Sun, 19 Feb 2012 17:24:00 +0100 Colomban Wendling lists.ban@herbesfolles.org wrote:
[...]
The commands indeed work similarly to our own `move_lines` function. I posted pull request #24 [3] which removes `move_lines` function and leverages the commands. I hope it can be committed so that I can switch my effort of improving "Move lines" feature from Geany to Scintilla.
Looks fine to -- apart of course it doesn't fix the initial problem yet. At least so there would be only one copy of the functionality.
So just to be sure we understand each other: I drop your previous pull request (#21), apply this one (#24) and wait for your patch to be in Scintilla?
Yes. But I don't promise that I'll post a request to Scintilla *soon*…
No big deal IMO regarding how long this bug existed without any reports about it. See, while I use the feature quite often, I ran into the bug only once in real situation.
So, it's now applied; Waits for Scintilla update :)
Cheers, Colomba
On Mon, 20 Feb 2012 20:03:17 +0100 Colomban Wendling lists.ban@herbesfolles.org wrote:
Le 19/02/2012 20:01, Eugene Arshinov a écrit :
On Sun, 19 Feb 2012 17:24:00 +0100 Colomban Wendling lists.ban@herbesfolles.org wrote:
[...]
The commands indeed work similarly to our own `move_lines` function. I posted pull request #24 [3] which removes `move_lines` function and leverages the commands. I hope it can be committed so that I can switch my effort of improving "Move lines" feature from Geany to Scintilla.
Looks fine to -- apart of course it doesn't fix the initial problem yet. At least so there would be only one copy of the functionality.
So just to be sure we understand each other: I drop your previous pull request (#21), apply this one (#24) and wait for your patch to be in Scintilla?
Yes. But I don't promise that I'll post a request to Scintilla *soon*…
No big deal IMO regarding how long this bug existed without any reports about it. See, while I use the feature quite often, I ran into the bug only once in real situation.
So, it's now applied; Waits for Scintilla update :)
Hello.
There is some good news. Recently they applied a patch with an updated implementation of MOVESELECTEDLINES [1]. I guess, it will be in the next release.
[1]:http://sf.net/tracker/?func=detail&atid=102439&aid=3511023&group_id=2439
-- Best regards, Eugene.