Fixes & changes for #1470 #1554

b4n · 2017-07-24T05:36:24Z

Followups on #1470.

b4n · 2017-07-24T05:38:41Z

src/editor.c

@@ -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

Looking at it more closely, the issue is more serious, and your fix seems insufficient. I'll look into it.

kugel- · 2017-07-24T21:09:43Z

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.

kugel- · 2017-07-26T20:42:35Z

See #1561

b4n · 2017-07-28T03:39:25Z

src/editor.c

@@ -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 957b49b#comments

What's the resolution on this? Still not a fan.

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.

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.

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.

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?

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 957b49b#commitcomment-23346900, but if the consensus is to keep _ I won't argue.

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).

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.

Since we don't agree let's post-pone this very change. The rest of the PR is good and should be merged.

kugel- · 2017-09-18T06:05:26Z

I still don't really like b998236 but otherwise looks good.

elextr · 2017-09-19T01:53:15Z

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.

kugel- · 2017-09-19T06:02:16Z

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.

elextr · 2017-09-19T06:21:31Z

@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.

codebrainz · 2017-09-19T10:55:50Z

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.

elextr · 2017-09-19T11:57:29Z

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.

kugel- · 2017-09-19T13:27:37Z

I'd say _ is equally likely to exist, and would prefer not to change it.

codebrainz · 2017-09-19T14:36:08Z

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.

kugel- · 2017-09-19T15:05:28Z

I'm more concerned about the sidebar becoming busted.

kugel-

Let's back out b998236 and merge the rest, until we agree on a placeholder.

kugel- · 2017-11-10T15:22:53Z

src/editor.c

@@ -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.

These were introduced in geany#1470 and a fix was also provided in geany#1554.

It makes more semantic sense than underscore, and is still full ASCII unlike the proper Unicode ellipsis.

Now the keybinding can be overridden (e.g. using Tab for it as well as normal behavior), beeping when there is no next cursor is more annoying than useful.

Now the keybinding can be overridden (e.g. using Tab for it as well as normal behavior), beeping when there is no next cursor is more annoying than useful. Part of #1554.

b4n · 2018-08-04T14:41:38Z

I re-pushed after fixing the conflicts now @codebrainz committed 36f4474 (which is basically exactly the commit I had).

I also cherry-picked 2128bf1 into master, so the only thing left is the controversial placeholder change. Closing for now.

b4n added this to the 1.32 milestone Jul 24, 2017

b4n requested a review from kugel- July 24, 2017 05:36

b4n changed the title ~~Kugel /better snippets/follow up~~ Fixes & changes for #1470 Jul 24, 2017

b4n commented Jul 24, 2017

View reviewed changes

kugel- mentioned this pull request Jul 27, 2017

editor: fix incorrect variable reference #1561

Merged

b4n force-pushed the kugel-/better-snippets/follow-up branch from dcea941 to e7cd97f Compare July 28, 2017 03:38

b4n commented Jul 28, 2017

View reviewed changes

kugel- requested changes Nov 10, 2017

View reviewed changes

elextr modified the milestones: 1.32, 1.33 Nov 23, 2017

codebrainz added a commit to codebrainz/geany that referenced this pull request Dec 21, 2017

Remove some unused variables

36f4474

These were introduced in geany#1470 and a fix was also provided in geany#1554.

kugel- modified the milestones: 1.33, 1.34 Apr 3, 2018

b4n added 2 commits August 4, 2018 16:21

Snippets: Use ASCII ellipsis for the cursor placeholder

68904e0

It makes more semantic sense than underscore, and is still full ASCII unlike the proper Unicode ellipsis.

Don't beep if there is no next snippet cursor

2128bf1

Now the keybinding can be overridden (e.g. using Tab for it as well as normal behavior), beeping when there is no next cursor is more annoying than useful.

b4n force-pushed the kugel-/better-snippets/follow-up branch from e7cd97f to 2128bf1 Compare August 4, 2018 14:21

b4n added a commit that referenced this pull request Aug 4, 2018

Don't beep if there is no next snippet cursor

3f20ad3

Now the keybinding can be overridden (e.g. using Tab for it as well as normal behavior), beeping when there is no next cursor is more annoying than useful. Part of #1554.

b4n closed this Aug 4, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes & changes for #1470 #1554

Fixes & changes for #1470 #1554

b4n commented Jul 24, 2017

b4n Jul 24, 2017

kugel- Jul 24, 2017

kugel- commented Jul 24, 2017

kugel- commented Jul 26, 2017

b4n Jul 28, 2017

kugel- Sep 18, 2017

codebrainz Sep 18, 2017

kugel- Sep 18, 2017

codebrainz Sep 18, 2017

kugel- Sep 18, 2017

b4n Sep 18, 2017

codebrainz Sep 19, 2017

codebrainz Sep 19, 2017

kugel- Nov 10, 2017

kugel- commented Sep 18, 2017

elextr commented Sep 19, 2017

kugel- commented Sep 19, 2017

elextr commented Sep 19, 2017

codebrainz commented Sep 19, 2017

elextr commented Sep 19, 2017

kugel- commented Sep 19, 2017

codebrainz commented Sep 19, 2017

kugel- commented Sep 19, 2017

kugel- left a comment

kugel- Nov 10, 2017

b4n commented Aug 4, 2018

Fixes & changes for #1470 #1554

Fixes & changes for #1470 #1554

Conversation

b4n commented Jul 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kugel- commented Jul 24, 2017

kugel- commented Jul 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kugel- commented Sep 18, 2017

elextr commented Sep 19, 2017

kugel- commented Sep 19, 2017

elextr commented Sep 19, 2017

codebrainz commented Sep 19, 2017

elextr commented Sep 19, 2017

kugel- commented Sep 19, 2017

codebrainz commented Sep 19, 2017

kugel- commented Sep 19, 2017

kugel- left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b4n commented Aug 4, 2018