Right now, when a config file from Tools->Configuration Files is edited for the first time, the global config file is taken and all lines in it are commented-out. When modified, such a file is copied into the user's config directory. The logic behind this is that the user typically wants to modify just one or few config options while leaving the rest in the default state.
However, commenting-out all lines is problemmatic. Consider for instance the beginning of filetypes.common when all lines are commented-out by the current code:
```ini #~ # For complete documentation of this file, please see Geany's main documentation #~ [styling] #~ # use foreground;background;bold;italic or named_style,bold,italic #~ #~ # used for filetype All/None #~ #default=default ```
1. Already commented-out lines are commented out once more which is ugly 2. Sections are commented out. This is problematic because users don't have to just comment-out the variable they want to modify, they must also comment-out the section in which the variable is which is easy to forget. 3. Empty lines are commented out which is unnecessary
This patch goes through the config file line by line and keeps sections, comments and empty lines unmodified so the result of the above example is this:
```ini # For complete documentation of this file, please see Geany's main documentation [styling] # use foreground;background;bold;italic or named_style,bold,italic
# used for filetype All/None #default=default ```
The patch does this only for .conf filetype and uses the original code for other file types. You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/3413
-- Commit Summary --
* Improve code commenting-out global config files
-- File Changes --
M src/ui_utils.c (38)
-- Patch Links --
https://github.com/geany/geany/pull/3413.patch https://github.com/geany/geany/pull/3413.diff
Just a comment, the original concept was that it used the toggle comment marker so blocks of settings could just be toggled.
But `Edit->Format->Toggle line Commentation` is pretty well hidden and obscure so that only experienced users know about it and they already have their configs fettled the way they want them, editing a conf file the first time is rare.
And newer users seem to only set one setting at a time, and they do forget the heading all the time.
So although this removes the toggle ability IMO in real life use its likely to be better.
LGBI
Just a comment [Edit: pun intended :-)], the original concept was that it used the toggle comment marker so blocks of settings could just be toggled.
I could do it too with this patch too - right now the patch only inserts `#` but it could also be `#~ `. Maybe it's a good idea also because it makes it more obvious which line was commented out.
Maybe it's a good idea also because it makes it more obvious which line was commented out.
Well with the current patch there are three types of lines, headings which are uncommented, comments which are "# " commented and settings that are "#" no space are commented[^1].
Probably not to new users who are the ones fiddling with conf files, using `#` for a comment is "well known", but whats this `#~` thing? And do I leave the `~` or what? Have had all those during various support interactions.
There is really no universally obvious solution, and if we add instructions in a comment at the top when we copy the file "Only remove the comment `#~`/`#` marker from lines you intend to change" on the off chance that users will read it, well either is as good/bad as the other.
[^1]: its not clear from the G* docs for [keyfile](https://docs.gtk.org/glib/struct.KeyFile.html) (can you see where the file format is described?) if space at the start of the setting line is ok or not, so not having a space on lines which are settings is best, so only the comment marker is removed.
Probably not to new users who are the ones fiddling with conf files, using # for a comment is "well known", but whats this #~ thing? And do I leave the ~ or what? Have had all those during various support interactions.
This is definitely a good reason to use just the simple `#` then.
its not clear from the G* docs for [keyfile](https://docs.gtk.org/glib/struct.KeyFile.html) (can you see where the file format is described?) if space at the start of the setting line is ok or not, so not having a space on lines which are settings is best, so only the comment marker is removed. [↩](https://github.com/geany/geany/pull/3413#user-content-fnref-1-91e667f444008f...)
It seems to be somewhat documented here
https://gitlab.gnome.org/GNOME/glib/-/blob/main/glib/gkeyfile.c#L76
but I'm not sure if keys can start with spaces. I used the `isspace()` check skipping spaces to be on the safe side.
@techee pushed 1 commit.
766f756495e0704e7656773bdbf29f096ef45d16 fixup! Improve code commenting-out global config files
The reference to the desktop spec was what I was looking for, wonder why that doesn't appear in the docs, it used to? Unfortunately it is not explicit about leading spaces either, but [the docodementation](https://gitlab.gnome.org/GNOME/glib/-/blob/main/glib/gkeyfile.c#L1277) says leading spaces are ignored, so it won't matter if the user only deletes a `#` and leaves a space. Still probably best to just comment settings with a `#` no space.
I wouldn't make the setting variable comments too similar to the actual comments - they then become hard to distinguish, see:
```ini # set to false if a comment character/string should start at column 0 of a line, true uses any # indentation of the line, e.g. setting to true causes the following on pressing CTRL+d #command_example(); # setting to false would generate this # command_example(); # This setting works only for single line comments #comment_use_indent=true ```
where only the last line is the actual settings. Actually I think we should distinguish these more. What about adding an extra `#` to the existing comments? Things become more clear I think:
```ini ## set to false if a comment character/string should start at column 0 of a line, true uses any ## indentation of the line, e.g. setting to true causes the following on pressing CTRL+d # #command_example(); ## setting to false would generate this ## command_example(); ## This setting works only for single line comments #comment_use_indent=true ``` Maybe I should put `#` in front of the existing `#` so the `command_example()` is consistent with the rest.
``` ## set to false if a comment character/string should start at column 0 of a line, true uses any ## indentation of the line, e.g. setting to true causes the following on pressing CTRL+d # #command_example(); ## setting to false would generate this ## command_example(); ## This setting works only for single line comments #comment_use_indent=true ```
Looks good to me, and its simple, just add `#` to every line except headings.
I would not worry about the indented example, after adding a `#`, it is no longer an accurate example whichever way its done.
[begin rant] In fact the example should not be there, its in the manual, the filetype files should not be turned into documentation surrogates. If it wasn't for the fact that its in every filetype file I would say remove the example, its not like its some horrendously important setting that everyone uses, why should it alone have a copy of the manual above it? It should just say "# false to place comment at start of line, true to place after indent, see manual". Just one line like most other settings!! [end rant] Anyhow its not part of this PR whatever is done with it.
@techee pushed 1 commit.
94c3e110e1a9b5da60faaa81be42759d95365886 fixup! Improve code commenting-out global config files
Looks good to me, and its simple, just add # to every line except headings.
And also except empty lines.
I would not worry about the indented example, after adding a #, it is no longer an accurate example whichever way its done.
I actually don't think this is even a valid conf file syntax - Scintilla at least doesn't syntax-highlight the indented line as a comment and I think it "works" simply because the parser just discards it as it is no key-value pair.
[begin rant] In fact the example should not be there, its in the manual, the filetype files should not be turned into documentation surrogates. If it wasn't for the fact that its in every filetype file I would say remove the example, its not like its some horrendously important setting that everyone uses, why should it alone have a copy of the manual above it? It should just say "# false to place comment at start of line, true to place after indent, see manual". Just one line like most other settings!! [end rant] Anyhow its not part of this PR whatever is done with it.
Agree, the conf files would deserve a huge cleanup.
There's another problem (and not talking about the horrible wording of the comment below). Some values are already commented-out in the conf files so they end up with the double `#` as well: ```ini ## the following characters are these which a "word" can contains, see documentation ##wordchars=_abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789 ``` We could try to do something smart and detect `=` in the line which would indicate it's a key-value pair and not to insert the extra `#` in this case, or just ignore this problem.
I just had a look at a few filetype's configurations and it's really a mixed bag of styles. I think it would make most sense to document all the config options in `filetypes.common` _only_ and language-specific config files would contain only those values they override to some other value than `filetypes.common` without any extra comments - apart maybe some introductory ```ini # For complete documentation of this file, please see Geany's main documentation and filetypes.common ```
What do you think?
I just had a look at a few filetype's configurations and it's really a mixed bag of styles.
Thats the problem with software development that is contributed by lots of different people :-D
I am not sure about the suggestion to document only in `filetypes.common`, in one way I see your point, its all in one place and the individual filetype files then work as intended, only containing settings that need to be overridden. But on the other hand it means that two files need to be open, the individual filetype to be edited and `filetypes.common` to read the comments and to find settings that the filetype file does not yet use, and since Geany doesn't do multi-pane very well nor multi-window, it will mean switching back and forward between them, not ideal.
The other way is to make all filetypes files a copy of `filetypes.common`, comments and all, with the unused settings already commented out. Then no commenting code is needed when the filetype file is copied, and users can add settings simply by uncommenting the one they want instead of having to copy it from `filetypes.common`. Of course that means maintaining them all when `filetypes.common` changes.
(FWIW, I didn't read everything yet nor looked at the implementation, but I like the idea and it probably would have saved the author of #3425 some trouble)
[read the thread]
IMO, just use one single `#` as you did at the start, and don't worry about the `comment_use_indent` comment ~~comment comment~~ (oops, got carried away :upside_down_face:). * `# ` as documentation comments in conffiles is most common * `#key=value` as commented-out default/suggestion is common (many things in e.g. /etc do this) * `## ` is a *lot* less common, so is `#~ ` indeed.
The one thing you possibly could do is still add a leading `#` to the `\t#comment` line, e.g. "if line is an indented comment, add a documentation comment prefix", but that might be an overly specific case to warrant it, and special cases have a tendency to come back to bite you…
and language-specific config files would contain only those values they override to some other value than `filetypes.common`
Hum, I'm not sure it's a great idea. Some settings make sense to be modified per-filetype rather than necessarily globally, and I think those are the ones showing up in filetypes definitions. E.g. `comment_use_indent`: it's both a user preference, but also not all languages are happy about it, or their canonical style isn't. Similarly, `wordchars` has some use per-filetype, where identifiers are not limited to the usual (say, they contain `-` for example). So yeah, there surely is room for improvement, but I don't think going whole "no non-overriden settings can ever be mentioned in filetype definitions" is the best move.
So yeah, there surely is room for improvement, but I don't think going whole "no non-overriden settings can ever be mentioned in filetype definitions" is the best move.
Yeah, that provides no guidance to users.
I think those are the ones showing up in filetypes definitions. E.g. comment_use_indent: it's both a user preference, but also not all languages are happy about it, or their canonical style isn't. Similarly, wordchars has some use per-filetype, where identifiers are not limited to the usual (say, they contain - for example).
Which kind of argues against my suggestion above of having all of `filetypes.common` in each filetype file commented out.
(the "usual" is rapidly becoming Unicode, eg C++ can now start with [XID_Start](https://www.unicode.org/reports/tr31/#Table_Lexical_Classes_for_Identifiers) and continue with zero or more [XID_Continue](https://www.unicode.org/reports/tr31/#Table_Lexical_Classes_for_Identifiers) and C is planned for version after C++23 IIRC, but thats another issue [@elextr clambers down off his "not all languages are C" soapbox] )
So ultimately I agree with @b4n that the simplest solution (add `#` to any line not starting with `[`) is best.
The one thing you possibly could do is still add a leading `#` to the `\t#comment` line, e.g. "if line is an indented comment, add a documentation comment prefix", but that might be an overly specific case to warrant it, and special cases have a tendency to come back to bite you…
Wait, if we don't need to handle leading whitespaces (which we don't *need* AFAIK, although it might be nice), the logic is merely `if (*line && *line != '[' && *line != '#') comment();`, isn't it?
IMO, just use one single # as you did at the start, and don't worry about the comment_use_indent comment comment comment (oops, got carried away 🙃). `#` as documentation comments in conffiles is most common `#`key=value as commented-out default/suggestion is common (many things in e.g. /etc do this) `##` is a lot less common, so is #~ indeed.
OK.
The one thing you possibly could do is still add a leading # to the \t#comment line, e.g. "if line is an indented comment, add a documentation comment prefix", but that might be an overly specific case to warrant it, and special cases have a tendency to come back to bite you…
I don't think we should do anything about this in the code - but I think we should modify this comment in all the conf files so it already contains the leading `#` so we ship conf files with valid syntax. I'll create a separate PR for this.
Wait, if we don't need to handle leading whitespaces (which we don't need AFAIK, although it might be nice), the logic is merely if (*line && *line != '[' && *line != '#') comment();, isn't it?
Yes, but I also think we shouldn't add `#` on empty lines or lines containing only whitespaces.
Hum, I'm not sure it's a great idea. Some settings make sense to be modified per-filetype rather than necessarily globally, and I think those are the ones showing up in filetypes definitions. E.g. comment_use_indent: it's both a user preference, but also not all languages are happy about it, or their canonical style isn't. Similarly, wordchars has some use per-filetype, where identifiers are not limited to the usual (say, they contain - for example).
So yeah, there surely is room for improvement, but I don't think going whole "no non-overriden settings can ever be mentioned in filetype definitions" is the best move.
OK.
Wait, if we don't need to handle leading whitespaces (which we don't need AFAIK, although it might be nice), the logic is merely if (*line && *line != '[' && *line != '#') comment();, isn't it?
Yes, but I also think we shouldn't add # on empty lines or lines containing only whitespaces.
I could modify the code to use `sci_get_line()` and work on lines instead of individual characters, it just mens allocating/deallocating strings for each line (not that it matters much performance-wise though). Not sure which option is better here.
Wait, if we don't need to handle leading whitespaces (which we don't need AFAIK, although it might be nice), the logic is merely if (*line && *line != '[' && *line != '#') comment();, isn't it?
Yes, but I also think we shouldn't add # on empty lines or lines containing only whitespaces.
I could modify the code to use `sci_get_line()` and work on lines instead of individual characters, it just mens allocating/deallocating strings for each line (not that it matters much performance-wise though). Not sure which option is better here.
It doesn't matter, you don't have to switch to fetching lines to implement either, my "code" was merely laying out my thoughts, not really a suggestion of implementation *per se*.
And it's be easy enough to support all-whitespace lines if you really want to, just do something like (again, this has *not* to be the actual implementation): ```c for (line in document) { // see, pseudo-code :) const char *p = line; while (*p && isspace(*p)) p++; if (! *p) continue; // empty line if (*p == '[') continue; // section, leave it alone if (*line == '#') continue; // already commented at column 0, leave it alone comment(&line); } ```
which, given you have all the info, simplifies to `if (*p && *p != '[' && *line != '#') comment()`.
@b4n commented on this pull request.
@@ -2096,6 +2096,33 @@ void ui_table_add_row(GtkTable *table, gint row, ...)
}
+/* comment-out all lines that are not already commented out except sections */ +static void comment_conf_files(ScintillaObject *sci) +{ + gint line, line_count; + + line_count = sci_get_line_count(sci); + for (line = 0; line < line_count - 1; line++) + { + gint pos_start = sci_get_position_from_line(sci, line); + gint pos_end = sci_get_position_from_line(sci, line + 1);
shouldn't you use [`sci_get_line_end_position()`](https://scintilla.org/ScintillaDoc.html#SCI_GETLINEENDPOSITION) instead? It'd avoid checking EOL (which might or might not be faster, depending on the implementation of the above), and be ever so slightly more robust (in case EOL doesn't pass `isspace()`, but that's rather theoretical).
@b4n commented on this pull request.
@@ -2096,6 +2096,33 @@ void ui_table_add_row(GtkTable *table, gint row, ...)
}
+/* comment-out all lines that are not already commented out except sections */ +static void comment_conf_files(ScintillaObject *sci) +{ + gint line, line_count; + + line_count = sci_get_line_count(sci); + for (line = 0; line < line_count - 1; line++) + { + gint pos_start = sci_get_position_from_line(sci, line);
You could also use [`SCI_GETLINEINDENTPOSITION`](https://scintilla.org/ScintillaDoc.html#SCI_GETLINEINDENTPOSITION) instead of manually doing the maths
@b4n commented on this pull request.
gint pos_start = sci_get_position_from_line(sci, line);
+ gint pos_end = sci_get_position_from_line(sci, line + 1); + gint pos; + + for (pos = pos_start; pos < pos_end; pos++) + { + gchar c = sci_get_char_at(sci, pos); + if (c == '[') + break; + if (!isspace(c)) + { + sci_insert_text(sci, pos_start, "#"); + break; + } + }
Maybe something like this? :warning: ATTENTION: this is entirely not reviewed or tested in any way. ```suggestion gint pos_start = sci_get_position_from_line(sci, line); gint pos_end = sci_get_line_end_position(sci, line); gint pos_indent = SSM(sci, SCI_GETLINEINDENTPOSITION, line, 0);
if (pos_indent < pos_end && sci_get_char_at(sci, pos_start) != '#' && sci_get_char_at(sci, pos_indent) != '[') sci_insert_text(sci, pos_start, "#"); ```
@techee commented on this pull request.
@@ -2096,6 +2096,33 @@ void ui_table_add_row(GtkTable *table, gint row, ...)
}
+/* comment-out all lines that are not already commented out except sections */ +static void comment_conf_files(ScintillaObject *sci) +{ + gint line, line_count; + + line_count = sci_get_line_count(sci); + for (line = 0; line < line_count - 1; line++) + { + gint pos_start = sci_get_position_from_line(sci, line); + gint pos_end = sci_get_position_from_line(sci, line + 1);
Yeah, this is better.
@techee commented on this pull request.
gint pos_start = sci_get_position_from_line(sci, line);
+ gint pos_end = sci_get_position_from_line(sci, line + 1); + gint pos; + + for (pos = pos_start; pos < pos_end; pos++) + { + gchar c = sci_get_char_at(sci, pos); + if (c == '[') + break; + if (!isspace(c)) + { + sci_insert_text(sci, pos_start, "#"); + break; + } + }
Yeah, something like that, but again, your code doesn't detect empty lines where we shouldn't insert `#` which is the purpose of the `isspace()` test in my code. I also think we don't really have to check for the indented `[` or `#` since these aren't valid conf files.
@b4n commented on this pull request.
gint pos_start = sci_get_position_from_line(sci, line);
+ gint pos_end = sci_get_position_from_line(sci, line + 1); + gint pos; + + for (pos = pos_start; pos < pos_end; pos++) + { + gchar c = sci_get_char_at(sci, pos); + if (c == '[') + break; + if (!isspace(c)) + { + sci_insert_text(sci, pos_start, "#"); + break; + } + }
but again, your code doesn't detect empty lines where we shouldn't insert `#` which is the purpose of the `isspace()` test in my code.
The intent of `if (pos_indent < pos_end)` is to detect that (again, not tested whether that actually works, and what those positions actually are)
I also think we don't really have to check for the indented `[` or `#` since these aren't valid conf files.
Aren't they? So you say the *only* case that matters for checking whitespaces is whether it makes a blank line? That would make the code look like that:
```c if (pos_indent < pos_end) { gchar c = sci_get_char_at(sci, pos_start); if (c != '#' && c != '[') sci_insert_text(sci, pos_start, "#"); } ```
Or indeed your loop, whichever you like best.
@b4n commented on this pull request.
gint pos_start = sci_get_position_from_line(sci, line);
+ gint pos_end = sci_get_position_from_line(sci, line + 1); + gint pos; + + for (pos = pos_start; pos < pos_end; pos++) + { + gchar c = sci_get_char_at(sci, pos); + if (c == '[') + break; + if (!isspace(c)) + { + sci_insert_text(sci, pos_start, "#"); + break; + } + }
I also think we don't really have to check for the indented `[` or `#` since these aren't valid conf files.
I just checked and they actually [are valid per `GKeyFile` handling](https://gitlab.gnome.org/GNOME/glib/-/blob/main/glib/gkeyfile.c#L1277).
@techee pushed 1 commit.
1adb0baec35b054eb6bce275b94a32589ddf4347 fixup! Improve code commenting-out global config files
@techee commented on this pull request.
gint pos_start = sci_get_position_from_line(sci, line);
+ gint pos_end = sci_get_position_from_line(sci, line + 1); + gint pos; + + for (pos = pos_start; pos < pos_end; pos++) + { + gchar c = sci_get_char_at(sci, pos); + if (c == '[') + break; + if (!isspace(c)) + { + sci_insert_text(sci, pos_start, "#"); + break; + } + }
It's actually a simpler code when we also detect the indented `[` and `#` so let's leave it that way. Interesting that it's valid for GKeyFile - they are definitely not valid for the Scintilla lexer.
I just re-pushed the fix for the patch - I used the `sci_get_line_end_position()` which is definitely better and removed the comment for `#` as the decision seems to be not to have the `##` comments for the original comments. The rest is just my original patch - I find it simple enough but maybe there's a better way to write it or there's maybe something I'm still overlooking.
@b4n commented on this pull request.
gint pos_start = sci_get_position_from_line(sci, line);
+ gint pos_end = sci_get_position_from_line(sci, line + 1); + gint pos; + + for (pos = pos_start; pos < pos_end; pos++) + { + gchar c = sci_get_char_at(sci, pos); + if (c == '[') + break; + if (!isspace(c)) + { + sci_insert_text(sci, pos_start, "#"); + break; + } + }
OK, looks fine to me
@b4n approved this pull request.
This needs squashing, but apart from that, LGTM and WFM :+1:
Merged #3413 into master.
Great, thanks!
github-comments@lists.geany.org