The Geany automatic closing of typed ", ', (, [ and { isn't working very well. For e.g. typing ``` void main() { } ``` results in ``` void main() { }) ``` Above the automatically inserted closing parenthesis stays after the cursor when I type the closing parenthesis and so it is moving while typing at the very end, after the curly braces.
Or typing the following, the automatically inserted closing double quote moves with the cursor. `std::string d = "asdf";` results in `std::string d = "asdf";""`
This is only circumventable with using the arrow keys or clicking with the mouse after the automatically inserted closing character.
Furthermore automatically inserting isn't working if Geany somehow finds a closing character somewhere else, e.g. inside the above example curly braces `void main() {}` typing: `if() {` doesn't add an closing brace as it find the closing from the `main`.
As I love to use Geany in my everday work this would be a very valuable improvement. So I wouldn't need need to compile my own better Geany every now and then. You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/2943
-- Commit Summary --
* <a href="https://github.com/geany/geany/pull/2943/commits/d246870dbb081a6888f0a7a7dd8d3c1305567da3">Consume closing brackets, braces, parenthesis, single and double quotes</a> * <a href="https://github.com/geany/geany/pull/2943/commits/b3d0cdd1de364c253d18a36bb225a928feb10eff">Nice on-new-line bracket handling</a>
-- File Changes --
M doc/geany.txt (5) M src/editor.c (172) M src/editor.h (1) M src/sciwrappers.c (10) M src/sciwrappers.h (3)
-- Patch Links --
https://github.com/geany/geany/pull/2943.patch https://github.com/geany/geany/pull/2943.diff
IIUC the autoclose plugin works this way already, you can just use that.
I see a feature in Geany that isn't working as expected. So install a plugin that fixes it seems weird. Furthermore the plugin isn't right there if Geany is installed. So one has to look for a solution and may find the plugin or simply gets the impression Geany's auto-close feature is bad and deactivates it (as I did)
simply gets the impression Geany's auto-close feature is bad and deactivates it (as I did)
As did I, maybe a PR to remove it from Geany? [ducks] :grin:
The argument that any specific functionality should be built-in, not in plugins does not make sense. All current IDEs/editors use plugins to add/extend features, it allows different use-cases to be addressed without arguments about what should be built in, the user just loads the plugin/extension that suits them.
Its good that you actually made changes to the manual, although I don't think it actually explains the operation very well, and it doesn't explain your second {} commit. That means I may misunderstand what your proposal does, but it sounds quite similar to the way Eclipse works.
What I find is that Eclipse autoclose might make sense when writing new code, it is a pain in the [colloquial expression elided] when editing existing code. Even Eclipse with all its language smarts breaks roughly 50% of the time when editing existing C++ code, adding extra closing brackets in the wrong places or omitting them when it shouldn't.
With the best will in the world, Geany does not have the smarts of Eclipse, so I'm rather pessimistic about autoclose. Could you perhaps explain explicitly the logic after your changes, the OP and commit messages do not really clearly explain the intended logic?
As did I, maybe a PR to remove it from Geany? [ducks] 😁
Geany is a lightweight IDE with a feature for auto-closing built in. I don't think a thing like automatic inserting closing characters should be abandoned as it's not an text-editor, it's an IDE.
The argument that any specific functionality should be built-in, not in plugins does not make sense. All current IDEs/editors use plugins to add/extend features, it allows different use-cases to be addressed without arguments about what should be built in, the user just loads the plugin/extension that suits them.
As far I'am concerned I don't know an easy way inside Geany like an plugin-store to install plugins/extensions as in e.g. Chrome, Firefox, VS-Code or Eclipse. Even more inside Geany there is no hint, where plugins can be got from. Furthermore it may not be possible due to organisation/company restrictions to install anything else than Geany without plugins. So leaving "core"-functionality in an broken state with the reasoning it can be fixed by a plugin is short-sighted.
Its good that you actually made changes to the manual, although I don't think it actually explains the operation very well, and it doesn't explain your second {} commit. That means I may misunderstand what your proposal does, but it sounds quite similar to the way Eclipse works.
I made those changes 2(?) years ago and worked with a own-built version. But as I upgraded Geany recently I again found the situation with auto-closing. I rethinked my decision not to go further with the PR. So I must recap the changes from back then.
It's rather long ago I used Eclipse so I don' know.
What I find is that Eclipse autoclose might make sense when writing new code, it is a pain in the [colloquial expression elided] when editing existing code. Even Eclipse with all its language smarts breaks roughly 50% of the time when editing existing C++ code, adding extra closing brackets in the wrong places or omitting them when it shouldn't.
With the best will in the world, Geany does not have the smarts of Eclipse, so I'm rather pessimistic about autoclose. Could you perhaps explain explicitly the logic after your changes, the OP and commit messages do not really clearly explain the intended logic?
I will recap my changes from back then. I will try to do better.
Generally speaking, If the code is sound and it adds or improves a useful feature, I'm willing to accept it. I do not share the view that everything that can be in plugins should be implemented in a plugin. A decent out-of-the-box experience is important, if we want to be an IDE, and not just a dumb-but-extensible text editor. And more often than not, features are implemented in fire-and-forget plugins that become unmaintained quickly and create problems going forward.
Furthermore, I do share the concerns that plugin support is poor from and end-user perspective. We rely on distributions to discover and package all the plugins. There is no good way for users to discover and install them. Therefore it's not a good idea to become even more dependable on third-party plugins for even the most basic IDE features.
@jeremiaskleer do I understand correctly that you plan to update this PR or do you think I can start review?
@jeremiaskleer do I understand correctly that you plan to update this PR or do you think I can start review?
I think you can.
@kugel- commented on this pull request.
I still need to check the second commit in more detail.
I think it may be useful to compare your work with what the autoclose plugin already does. Perhaps it gives some insights w.r.t to the implementation.
isAutoClosed = TRUE;
+ break; + case ''': + if (editor_prefs.autoclose_chars & GEANY_AC_SQUOTE) + isAutoClosed = TRUE; + break; + case '"': + if (editor_prefs.autoclose_chars & GEANY_AC_DQUOTE) + isAutoClosed = TRUE; + break; + } + + gchar cNext = sci_get_char_at( sci, pos); + + if( isAutoClosed && cNext == c ) { + sci_delete_range(sci, pos, 1);
Instead of truly deleting the closing char don't you think it would be better to just replace the insertion by a "virtual" cursor movement? Might make a difference w.r.t. undo behavior.
@@ -138,6 +138,7 @@ typedef struct GeanyEditorPrefs
gint autocompletion_update_freq; gint scroll_lines_around_cursor; gint ime_interaction; /* input method editor's candidate window behaviour */ + guint autoclose_chars_consume;
unecessary
if (editor_prefs.autoclose_chars & GEANY_AC_PARENTHESIS)
+ isAutoClosed = TRUE; + break; + case '}': + if (editor_prefs.autoclose_chars & GEANY_AC_CBRACKET) + isAutoClosed = TRUE; + break; + case ']': + if (editor_prefs.autoclose_chars & GEANY_AC_SBRACKET) + isAutoClosed = TRUE; + break; + case ''': + if (editor_prefs.autoclose_chars & GEANY_AC_SQUOTE) + isAutoClosed = TRUE; + break; + case '"':
I wonder how Python multiline strings behave with this change? These ones enclosed by 3 quotoes `""" foo bar """`
@@ -2193,6 +2193,10 @@ Geany can automatically insert a closing bracket and quote characters when
you open them. For instance, you type a ``(`` and Geany will automatically insert ``)``. With the following options, you can define for which characters this should work. +For a more flowing writing (not move the typing hand to arrow keys) Geany +checks if the character written is the closing quote or bracket that was +previously inserted. In that case only one quote or bracket of the one written +and the one previously inserted remains.
This can be written in a much more concise way. E.g. "To prevent extraneous quotes or brackets Geany detects if you manually insert closing characters.". Then I would place that before the "With the following options, you …" sentence.
@kugel- commented on this pull request.
}
if (get_project_pref(auto_continue_multiline)) { /* " * " auto completion in multiline C/C++/D/Java comments */ - auto_multiline(editor, line); + if (auto_multiline(editor, line, FALSE) && doHandleBracket) {
Why do you change multiline comment behavior?
@elextr requested changes on this pull request.
This commit has nothing to do with autoclosing really, its indenting after brace and as such should be a separate PR
And it needs to be documented in the manual not (or as well as) the commit message.
@elextr commented on this pull request.
if (editor_prefs.autoclose_chars & GEANY_AC_PARENTHESIS)
+ isAutoClosed = TRUE; + break; + case '}': + if (editor_prefs.autoclose_chars & GEANY_AC_CBRACKET) + isAutoClosed = TRUE; + break; + case ']': + if (editor_prefs.autoclose_chars & GEANY_AC_SBRACKET) + isAutoClosed = TRUE; + break; + case ''': + if (editor_prefs.autoclose_chars & GEANY_AC_SQUOTE) + isAutoClosed = TRUE; + break; + case '"':
also when changing `foo("abc")` to `foo(bar("abc"))`
@elextr commented on this pull request.
isAutoClosed = TRUE;
+ break; + case ''': + if (editor_prefs.autoclose_chars & GEANY_AC_SQUOTE) + isAutoClosed = TRUE; + break; + case '"': + if (editor_prefs.autoclose_chars & GEANY_AC_DQUOTE) + isAutoClosed = TRUE; + break; + } + + gchar cNext = sci_get_char_at( sci, pos); + + if( isAutoClosed && cNext == c ) { + sci_delete_range(sci, pos, 1);
Nice thought, but then anywhere a user types `)` before `)` it won't be inserted.
@elextr requested changes on this pull request.
Sorry about the dual reviews, I wanted to see if the first one would attach to the commit (it didn't) but then I couldn't add more to it.
static void auto_close_chars(ScintillaObject *sci, gint pos, gchar c) { const gchar *closing_char = NULL; - gint end_pos = -1; - - if (utils_isbrace(c, 0)) - end_pos = sci_find_matching_brace(sci, pos - 1);
This was deliberately added in https://github.com/geany/geany/commit/ff8664150bab4a9f59120c8d14ca7240ab21a5... so it is desired by at least some people, and nobody (AFAICT) has wanted it changed since 2008. So removing it just because one person doesn't like it doesn't seem reasonable.
@@ -2193,6 +2193,10 @@ Geany can automatically insert a closing bracket and quote characters when
you open them. For instance, you type a ``(`` and Geany will automatically insert ``)``. With the following options, you can define for which characters this should work. +For a more flowing writing (not move the typing hand to arrow keys) Geany +checks if the character written is the closing quote or bracket that was +previously inserted. In that case only one quote or bracket of the one written +and the one previously inserted remains.
I disagree, it should be less concise and actually explain what happens, so its known that the behaviour is deliberate, especially as pretty much whatever autoclose behaviour is chosen there are edge cases that are annoying. It really needs to document it properly. The commit message is more explicit than the manual.
@kugel- commented on this pull request.
static void auto_close_chars(ScintillaObject *sci, gint pos, gchar c) { const gchar *closing_char = NULL; - gint end_pos = -1; - - if (utils_isbrace(c, 0)) - end_pos = sci_find_matching_brace(sci, pos - 1);
Right, but this makes autoclose also next to useless (I have it disabled and would welcome improvements). Since autoclosing now only works if Scintilla doesn't see a matching brace autoclosing does not work most of the time. E.g.:
``` int foo() { if (x) { // !! no autoclose for {, as Scintilla matches the } at the end of the function } ``` `()` is autoclosed in that example, though. So basically as of now autoclosing works reliably only at the end of documents, otherwise it may seem to work only randomly.
The second commit has nothing to do with autoclosing really, its indenting after brace and as such should be a separate PR
And it needs to be documented in the manual not (or as well as) the commit message.
I agree with @elextr here. I'm open to both improvements presented in this PR, but the second one should be a separate PR. So let's focus on "consume closing chars when typing them manually" for this PR please.
I have also conducted what Notepad++ does in that regards, and it's definitely better than what we do.
@elextr commented on this pull request.
static void auto_close_chars(ScintillaObject *sci, gint pos, gchar c) { const gchar *closing_char = NULL; - gint end_pos = -1; - - if (utils_isbrace(c, 0)) - end_pos = sci_find_matching_brace(sci, pos - 1);
I investigated what Eclipse does, it autocompletes `{` only when return is typed after it so ``` if(blah){ ``` becomes ``` if(blah){ | } ``` where | is cursor.
This works pretty well, for me anyway, but its significantly different to the current Geany autocomplete. Maybe the second commit using return after `{` can be modified to do autocomplete instead of on `{` additional alternative to current behaviour.
Eclipse behaviour for parentheses is interesting, it autocompletes `)` if the character after the typed `(` is not a value or identifier.
So when typing `foo(` at the cursor |.
`if(|xxx)` becomes `if(foo(|xxx)` or `if(|1)` becomes `if(foo(|1)`
but
`if(|+xxx)` becomes `if(foo(|)+xxx)` or `if(|)` becomes `if(foo(|))`
It works fairly well when typing new code and is avoids some (but not all) mistakes during editing, at the expense of having to manually add or delete the closing `)` in some cases.
Note this is observed behaviour because it isn't documented, which makes the behaviour just plain annoying until its worked out, which is why I want it documented for Geany, whichever decisions we end up with.
@xiota commented on this pull request.
static void auto_close_chars(ScintillaObject *sci, gint pos, gchar c) { const gchar *closing_char = NULL; - gint end_pos = -1; - - if (utils_isbrace(c, 0)) - end_pos = sci_find_matching_brace(sci, pos - 1);
As far as I can tell, the autoclose plugin does not do any checking for whether Geany is already autoclosing. Yet when both the plugin and Geany autoclose are enabled, only a single closing brace appears in the editor. It's possible that Geany not closing braces when a matching brace is found is why the plugin does not "conflict" with built-in autoclose. (See #2970.)
As did I, maybe a PR to remove it from Geany? [ducks] grin
While I realize that was written in jest, from a user perspective, I would prefer that auto close be removed from Geany and improvements redirected toward the auto-close plugin.
* The Geany implementation doesn't do anything that can't be done in a plugin. * The preferences dialog is complicated. It's difficult to find features I know exist. Removing features that are implemented in plugins would make other features easier to find. * The auto-close plugin is clearly named, so users who want it are likely to find it. Chances are good that people who have seen the plugin manager know that the auto-close plugin exists, but don't know about the Geany implementation. * The plugin implementation is more capable than the Geany implementation. * Even if the Geany implementation is improved, it will be disabled by people who *still* think it's broken or who want the more advanced features that the plugin provides.
From the perspective of someone who has looked at the source (but is *not* a developer)... removing auto close from Geany would make the source easier to understand and work with, while the plugin ensures that no functionality is lost.
The reason I ducked was to avoid the missiles from those who do not want to have their existing functionality (no matter how bad) removed :grin:. I was expecting more comments, but perhaps @kugel- said it so eloquently that nobody could add anything.
Since Geany autocomplete can already be disabled to allow the Autoclose plugin freedom to do its thing the improvements could be made in either. You should do whichever you find easiest, its more important that the improvements actually happen than where.
If made in the plugin they can always be moved into Geany later if they are found to be better and would likely be more easily accepted if they had been in use for a while without major issues. But I don't actually care so long as the improvements are actual improvements, not just a reshuffling of which situations are right and which are wrong. That would just be annoying change.
Autoclose is difficult to do right, as the example suggestions of possible improvements stolen from Eclipse that I gave above show, they are improvements IMO but still often get it wrong. Thats another point, that preferred autoclose behaviour is also part personal preference. And can vary between languages.
I suspect Geany built in autoclose is popular (as best we can tell) because it does the least it can, and is deterministic, and individual options to disable each bracket type make it controllable.
I suspect Geany built in autoclose is popular (as best we can tell)
How do you tell without the spyware?
Can you please stop talking about "spyware" on every other PR? If you talk about "collecting usage data", then name it like that.
@kugel- I didn't start calling it spyware, but I'll stop. So how do you tell the feature is popular without telemetry?
(as best we can tell)
Just to be clear I wasn't referring to any gathered data, but to various conversations on IRC and elsewhere over the years which is the only way to know _why_ something is popular (as in not turned off) with the particular people who happened to talk about it, not how many use it, perhaps popular was the wrong word.
And in those conversations, was there a clear distinction between built-in autoclose and the plugin? If I didn't know they both existed, I'd probably refer to whichever one I knew about as *the* autoclose feature.
In conversations there is no precision, it was an impression I formed and right, wrong, or indifferent that impression, gathered over time, is the only information I have about what other users think of autoclose.
@jeremiaskleer commented on this pull request.
}
if (get_project_pref(auto_continue_multiline)) { /* " * " auto completion in multiline C/C++/D/Java comments */ - auto_multiline(editor, line); + if (auto_multiline(editor, line, FALSE) && doHandleBracket) {
The output of /** <ENTER>int main() {<ENTER> with auto-close enabled (<ENTER> == pressing enter-key on keyboard) in an c++ document is
with the change ("|" shows the cursor afterwards: ``` /** * int main() { * | * } ```
without: ``` /** * int main() { |* } ``` and in original Geany 1.37 on Windows: ``` /** * int main() { *|})
```
@jeremiaskleer Will you post an update to this PR or can we close it?
Hallo.
I would love to but I am not sure what to change as I didn't see the consensus in comments in reviews and therefor I am a little intimdated.
Best regards
Thomas Martitz ***@***.***> schrieb am Mo., 15. Nov. 2021, 07:45:
@jeremiaskleer https://github.com/jeremiaskleer Will you post an update to this PR or can we close it?
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/geany/geany/pull/2943#issuecomment-968584456, or unsubscribe https://github.com/notifications/unsubscribe-auth/ALJR57IE4FMYGIGKKS3PDUDUMCT67ANCNFSM5GBHXS3Q . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.
github-comments@lists.geany.org