Followups on #1470. You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/1554
-- Commit Summary --
* Remove now unused variables * Fix a possible use of an uninitialized variable * Snippets: Use ASCII ellipsis for the cursor placeholder * Don't beep if there is no next snippet cursor
-- File Changes --
M src/editor.c (10)
-- Patch Links --
https://github.com/geany/geany/pull/1554.patch https://github.com/geany/geany/pull/1554.diff
b4n commented on this pull request.
@@ -2458,7 +2457,7 @@ void editor_insert_text_block(GeanyEditor *editor, const gchar *text, gint inser
gint line_start = sci_get_line_from_position(sci, insert_pos); GString *buf; const gchar *eol = editor_get_eol_char(editor); - gint idx; + gint idx = 0;
I'm not 100% sure this is the Right Fix™, and I didn't test it thoroughly enough. @kugel- you might know better than I do maybe
kugel- commented on this pull request.
@@ -2458,7 +2457,7 @@ void editor_insert_text_block(GeanyEditor *editor, const gchar *text, gint inser
gint line_start = sci_get_line_from_position(sci, insert_pos); GString *buf; const gchar *eol = editor_get_eol_char(editor); - gint idx; + gint idx = 0;
Looking at it more closely, the issue is more serious, and your fix seems insufficient. I'll look into it.
I wonder why I didn't see any warnings. I understand that my CFLAGS may not be pedantic enough for the unused variables, but I should have seen the uninitialized access one.
See #1561
b4n commented on this pull request.
@@ -2394,7 +2394,7 @@ typedef struct
} SelectionRange;
-#define CURSOR_PLACEHOLDER "_" /* Would rather use … but not all docs are unicode */ +#define CURSOR_PLACEHOLDER "..." /* Would rather use … but not all docs are unicode */
S/A https://github.com/geany/geany/commit/957b49b868214a4aea81641dedeabb98a497e4...
I still don't really like b9982368 but otherwise looks good.
kugel- commented on this pull request.
@@ -2394,7 +2394,7 @@ typedef struct
} SelectionRange;
-#define CURSOR_PLACEHOLDER "_" /* Would rather use … but not all docs are unicode */ +#define CURSOR_PLACEHOLDER "..." /* Would rather use … but not all docs are unicode */
What's the resolution on this? Still not a fan.
codebrainz commented on this pull request.
@@ -2394,7 +2394,7 @@ typedef struct
} SelectionRange;
-#define CURSOR_PLACEHOLDER "_" /* Would rather use … but not all docs are unicode */ +#define CURSOR_PLACEHOLDER "..." /* Would rather use … but not all docs are unicode */
Why not use Unicode ellipsis? All documents are UTF-8 in memory and since placeholders markers shouldn't ever be saved (they aren't actual contents of the document), it seems like it would work fine.
kugel- commented on this pull request.
@@ -2394,7 +2394,7 @@ typedef struct
} SelectionRange;
-#define CURSOR_PLACEHOLDER "_" /* Would rather use … but not all docs are unicode */ +#define CURSOR_PLACEHOLDER "..." /* Would rather use … but not all docs are unicode */
I had that before. Since nothing prevents you from saving a doc containing the placeholders, I didn't want to enforce utf8 for saved files.
codebrainz commented on this pull request.
@@ -2394,7 +2394,7 @@ typedef struct
} SelectionRange;
-#define CURSOR_PLACEHOLDER "_" /* Would rather use … but not all docs are unicode */ +#define CURSOR_PLACEHOLDER "..." /* Would rather use … but not all docs are unicode */
Shouldn't the placeholders not be saved since they aren't meant to be literally inserted into the document? IIRC other editors I've tried that do this (namely XCode and QtCreator) don't treat placeholders as text in the document, but rather just as a graphical cue for where you'll bounce when tabbing to the next placeholder in the edit.
kugel- commented on this pull request.
@@ -2394,7 +2394,7 @@ typedef struct
} SelectionRange;
-#define CURSOR_PLACEHOLDER "_" /* Would rather use … but not all docs are unicode */ +#define CURSOR_PLACEHOLDER "..." /* Would rather use … but not all docs are unicode */
They are, in fact, inserted literally into the buffer. This is required for Scintilla markers. Since we don't currently filter the document upon save, they are also saved to the file.
What do other editors insert when you save a file containing the placeholder?
b4n commented on this pull request.
@@ -2394,7 +2394,7 @@ typedef struct
} SelectionRange;
-#define CURSOR_PLACEHOLDER "_" /* Would rather use … but not all docs are unicode */ +#define CURSOR_PLACEHOLDER "..." /* Would rather use … but not all docs are unicode */
I guess yeah ideally the placeholder wouldn't be any actual character but simply a display thing, but I'm not sure Scintilla allows for this, does it? It would need be able to have an indicator that takes more room than the range it's on, which doesn't sounds likely, but I might miss an existing feature.
Otherwise for what the placeholder is I don't care that much; I just didn't feel like `_` looked straightforward to me when using the thing, but I don't have a string opinion either. I still feel like https://github.com/geany/geany/commit/957b49b868214a4aea81641dedeabb98a497e4..., but if the consensus is to keep `_` I won't argue.
If the placeholder is a string that will get saved it has to be something that will save in all encodings since the user can select that at save time, hence @kugel- used ASCII.
But actually adding ANY characters to the buffer may result in illegal syntax. It would be better if the placeholder was not inserted at all.
What about using the `INDIC_POINT` indicator at the relevant positions? That seems to be the only one that marks a position not a character.
codebrainz commented on this pull request.
@@ -2394,7 +2394,7 @@ typedef struct
} SelectionRange;
-#define CURSOR_PLACEHOLDER "_" /* Would rather use … but not all docs are unicode */ +#define CURSOR_PLACEHOLDER "..." /* Would rather use … but not all docs are unicode */
I guess yeah ideally the placeholder wouldn't be any actual character but simply a display thing, but I'm not sure Scintilla allows for this, does it?
Since the placeholders are marked with a (presumably) unique indicator, there's no reason they couldn't be removed from the saved contents (mere matter of programming).
codebrainz commented on this pull request.
@@ -2394,7 +2394,7 @@ typedef struct
} SelectionRange;
-#define CURSOR_PLACEHOLDER "_" /* Would rather use … but not all docs are unicode */ +#define CURSOR_PLACEHOLDER "..." /* Would rather use … but not all docs are unicode */
What do other editors insert when you save a file containing the placeholder?
IIRC XCode and QtCreator enter some kind of special placeholder-filling mode where you can tab between them and fill them out, and then you commit them.
IMO just marking the position is too narrow.
In fact, inserting no character is even more likely to produce invalid syntax. Think of empty for-loop specification or empty rhs of an assignment. Actually _ is probably least problematic because in many languages it's the only valid non-alphanumeric character in an identifier.
I also like b4n's idea, but it's more work and should be a separate PR.
@kugel- yeah, the little zit below the line is pretty easy to miss. Maybe underscore with an indicator in the INDIC_BOX family would be visible with minimal (but not zero) likelyhood of upsetting encodings and syntax and still being visible.
Or let the snippet define the characters and display them with the indicator so the snippet has `%|xxx%` the characters `xxx` would be shown with the indicator. Then you just search for indicators to move cursor positions.
As I don't use snippets I am only concerned that we don't get complaints about encoding errors by using non-ASCII characters, the rest is just trying to be helpful, whatever you prefer is ok by me.
I am only concerned that we don't get complaints about encoding errors by using non-ASCII characters
While most encodings are roughly US-ASCII compatible, you'd have to survey every single supported encoding to ensure that character 95 is used for underscore and not something else. Some encodings are known to replace various US-ASCII characters with currency symbols, for example.
While most encodings are roughly US-ASCII compatible, you'd have to survey every single supported encoding to ensure that character 95 is used for underscore and not something else.
So long as the character exists in the encoding there won't be any problems, but if the character does not exist in the target encoding then the file won't save. Doesn't matter if it encodes to something different, but if it does not exist `g_convert()` will fail and the save will be aborted.
Having symbol parsing temporarily incorrect is common while writing code, so incorrect syntax is probably less of a problem than being unable to save the file.
Certainly ASCII alphabetics are the most likely to exist, and simple punctuation like dot (.). So maybe stick with the three character string "..." even if it will not be legal syntax in most languages.
I'd say _ is equally likely to exist, and would prefer not to change it.
So maybe stick with the three character string "..." even if it will not be legal syntax in most languages.
Seems like for programming languages, being illegal would be an advantage. If something is inserting text I neither typed directly nor into the snippet, into my document, I'd at least like a compiler warning to track it down, rather than potentially compiling and doing something weird at runtime.
I'm more concerned about the sidebar becoming busted.
kugel- requested changes on this pull request.
Let's back out b9982368889354cfa010e5906cd5f477f7b90f35 and merge the rest, until we agree on a placeholder.
@@ -2394,7 +2394,7 @@ typedef struct
} SelectionRange;
-#define CURSOR_PLACEHOLDER "_" /* Would rather use … but not all docs are unicode */ +#define CURSOR_PLACEHOLDER "..." /* Would rather use … but not all docs are unicode */
Since we don't agree let's post-pone this very change. The rest of the PR is good and should be merged.
Closed #1554.
I re-pushed after fixing the conflicts now @codebrainz committed 36f44741b5daa67f08c4251c147457b95545ef14 (which is basically exactly the commit I had).
I also cherry-picked 2128bf1be647dfa8f20fd6e00029c9f842e84e77 into master, so the only thing left is the controversial placeholder change. Closing for now.
github-comments@lists.geany.org