Hi,
I sent in a patch on April 23rd, but have not received any accept / reject notice, possibly because I didn't tag it with [patch], so here it is again. Resolves feature request #2990915 as illustrated here:
When comment_use_indent is set to true in per-filetype config, comments are no longer indented on a line-by-line basis, but have the indentation of the least indented line in the block. This looks a lot better.
In addition, empty lines are commented out by default. In the original patch, I introduced a new option for it, but Thomas Martitz suggested that this is not preferable.
Best regards, Tambet
Hi,
On Tue, 8 Jun 2010 17:47:47 +0300 Tambet Arak tambet.arak@gmail.com wrote:
I sent in a patch on April 23rd, but have not received any accept / reject notice, possibly because I didn't tag it with [patch], so here it is again. Resolves feature request #2990915 as illustrated here:
When comment_use_indent is set to true in per-filetype config, comments are no longer indented on a line-by-line basis, but have the indentation of the least indented line in the block. This looks a lot better.
In addition, empty lines are commented out by default. In the original patch, I introduced a new option for it, but Thomas Martitz suggested that this is not preferable.
I like the behavior introduced with this patch but I'm not sure whether its ready for committing. Do you have some special reason why you are introducing this:
+ sci_get_text_range(editor->sci, line_start, line_start + buf_len, sel); + sel[buf_len] = '\0';
I'm a bit confused about adding the \0 as from my understanding the string is already \0-terminating. Please correct me if I'm wrong.
Also sci_get_contents_range() should be used in favor of sci_get_text_range() IIRC.
Cheers, Frank
Hi,
I like the behavior introduced with this patch but I'm not sure whether its ready for committing. Do you have some special reason why you are introducing this:
- sci_get_text_range(editor->sci, line_start, line_start + buf_len, sel);
- sel[buf_len] = '\0';
This is just copied from below in the same function. sel is a gchar array and therefore
sel = sci_get_contents_range(editor->sci, line_start, line_start + buf_len);
wouldn't work:
error: incompatible types when assigning to type ‘gchar[256]’ from type ‘gchar *’
It seems that C doesn't allow casting into an array type either. Changing sel to gchar* would mean updating the whole function, but I wouldn't want to undertake that as it's only my first patch to Geany. For the sake of consistency, I think it makes sense to keep sci_get_text_range for now. Similarily, appending '\0' happens below as well and it can't hurt in any case.
Best regards,
Tambet
On Mon, 21 Jun 2010 18:40:52 +0300 Tambet Arak tambet.arak@gmail.com wrote:
Hi,
I like the behavior introduced with this patch but I'm not sure whether its ready for committing. Do you have some special reason why you are introducing this:
- sci_get_text_range(editor->sci, line_start, line_start + buf_len, sel);
- sel[buf_len] = '\0';
This is just copied from below in the same function. sel is a gchar array and therefore
sel = sci_get_contents_range(editor->sci, line_start, line_start + buf_len);
wouldn't work:
error: incompatible types when assigning to type ‘gchar[256]’ from type ‘gchar *’
It seems that C doesn't allow casting into an array type either. Changing sel to gchar* would mean updating the whole function, but I wouldn't want to undertake that as it's only my first patch to Geany. For the sake of consistency, I think it makes sense to keep sci_get_text_range for now. Similarily, appending '\0' happens below as well and it can't hurt in any case.
I see. Maybe we will need to change some more code in this context. l44 and l47 of your patch looks also a bit weird to me. I hope to find some more motivation to have a deeper look onto this upcoming days.
Cheers, Frank
On 4 July 2010 19:43, Frank Lanitz frank@frank.uvena.de wrote:
On Mon, 21 Jun 2010 18:40:52 +0300 Tambet Arak tambet.arak@gmail.com wrote:
Hi,
I like the behavior introduced with this patch but I'm not sure whether its ready for committing. Do you have some special reason why you are introducing this:
- sci_get_text_range(editor->sci, line_start, line_start + buf_len, sel);
- sel[buf_len] = '\0';
This is just copied from below in the same function. sel is a gchar array and therefore
sel = sci_get_contents_range(editor->sci, line_start, line_start + buf_len);
wouldn't work:
error: incompatible types when assigning to type ‘gchar[256]’ from type ‘gchar *’
It seems that C doesn't allow casting into an array type either. Changing sel to gchar* would mean updating the whole function, but I wouldn't want to undertake that as it's only my first patch to Geany. For the sake of consistency, I think it makes sense to keep sci_get_text_range for now. Similarily, appending '\0' happens below as well and it can't hurt in any case.
I see. Maybe we will need to change some more code in this context. l44 and l47 of your patch looks also a bit weird to me. I hope to find some more motivation to have a deeper look onto this upcoming days.
As it seems to be trying to get the line, can't it just use sci_get_line which is length safe and null terminated?
Cheers Lex
Cheers, Frank -- Frank Lanitz frank@frank.uvena.de
Geany-devel mailing list Geany-devel@uvena.de http://lists.uvena.de/cgi-bin/mailman/listinfo/geany-devel
Hi,
As it seems to be trying to get the line, can't it just use sci_get_line which is length safe and null terminated?
Thanks for the tip. This does make the code more readable and arguably safer. Attached is a new patch.
Best regards, Tambet
Hi again,
Any word on whether the patch will be rejected or accepted? It's almost two months since the last correspondence.
Best regards, Tambet
On Thu, Jul 8, 2010 at 5:51 PM, Tambet Arak tambet.arak@gmail.com wrote:
Hi,
As it seems to be trying to get the line, can't it just use sci_get_line which is length safe and null terminated?
Thanks for the tip. This does make the code more readable and arguably safer. Attached is a new patch.
Best regards, Tambet
Hi,
On Tue, 31 Aug 2010 17:54:45 +0300 Tambet Arak tambet.arak@gmail.com wrote:
Any word on whether the patch will be rejected or accepted? It's almost two months since the last correspondence.
Sorry for the delay.
I think the idea is good, but I've just tried the patch and it doesn't work if there are empty lines in the middle of the selected block. Then the comments are all inserted at column 0.
Also if some lines have tab indentation and some spaces then the comments could be inserted at the wrong position on some lines. Not sure how big an issue this is.
Regards, Nick
On Wed, 8 Sep 2010 15:32:17 +0100 Nick Treleaven nick.treleaven@btinternet.com wrote:
I think the idea is good, but I've just tried the patch and it doesn't work if there are empty lines in the middle of the selected block. Then the comments are all inserted at column 0.
Forgot to mention: this is a common use case because some people (us included) use the 'strip trailing spaces' option.
Regards, Nick
Hi,
I think the idea is good, but I've just tried the patch and it doesn't work if there are empty lines in the middle of the selected block. Then the comments are all inserted at column 0.
Yes, this happens because the empty lines are not indented. In a sense, it is the correct behavior. Do you think the patch should be modified to insert suitable padding to the beginning of empty lines?
Best regards, Tambet
On Wed, 8 Sep 2010 17:55:36 +0300 Tambet Arak tambet.arak@gmail.com wrote:
I think the idea is good, but I've just tried the patch and it doesn't work if there are empty lines in the middle of the selected block. Then the comments are all inserted at column 0.
Yes, this happens because the empty lines are not indented. In a sense, it is the correct behavior. Do you think the patch should be modified to insert suitable padding to the beginning of empty lines?
Not really, but the problem is that users with comment_use_indent set are not getting indented comments for non-empty lines in this case.
Regards, Nick
On Wed, 8 Sep 2010 15:32:17 +0100 Nick Treleaven nick.treleaven@btinternet.com wrote:
Also if some lines have tab indentation and some spaces then the comments could be inserted at the wrong position on some lines. Not sure how big an issue this is.
I've just realized, for the tabs & spaces mode this would be completely broken as 8 spaces are compressed into tabs, e.g. when one line starts with 4 spaces and the next line starts with a tab.
So it seems the patch implementation is a no-go.
Regards, Nick
On Wed, Sep 8, 2010 at 6:25 PM, Nick Treleaven nick.treleaven@btinternet.com wrote:
I've just realized, for the tabs & spaces mode this would be completely broken as 8 spaces are compressed into tabs, e.g. when one line starts with 4 spaces and the next line starts with a tab.
So it seems the patch implementation is a no-go.
This is unfortunate. Current 'toggle comment' tends to make the code less readable.
Regards Liviu
On Fri, 24 Sep 2010 00:15:49 +0300 Liviu Andronic landronimirc@gmail.com wrote:
On Wed, Sep 8, 2010 at 6:25 PM, Nick Treleaven nick.treleaven@btinternet.com wrote:
I've just realized, for the tabs & spaces mode this would be completely broken as 8 spaces are compressed into tabs, e.g. when one line starts with 4 spaces and the next line starts with a tab.
So it seems the patch implementation is a no-go.
This is unfortunate. Current 'toggle comment' tends to make the code less readable.
The implementation needs tweaking. Maybe it could revert to the old behaviour if a mixture of tabs and spaces are found.
Also it should ignore empty lines with no indentation, as these lines may have been stripped. Those lines make the patch ignore indentation when commenting.
Finally, the pre-patched comment function was too long. Ideally any new behaviour would make use of helper functions.
Regards, Nick