[Github-comments] [geany/geany] Better snippets (#1470)

[Colomban Wendling]

b4n commented on this pull request.

After quick testing I like the new behavior and the use of indicators :)  I'm not sure I like not being able to jump at the end of several snippets (like `def` for Python, which IMO would be handier with a `%cursor%` at the end), but it might require some good testing for pros and cons.

The uninitialized var issue might be serious though.  See #1554.

>  	g_string_free(buf, TRUE);
 }
 
 
+static gboolean find_next_snippet_indicator(GeanyEditor *editor, SelectionRange *sel)
+{
+	ScintillaObject *sci = editor->sci;
+	gint val;

unused variable (see 871e72cc03c2d9501a54652ebeb0ca15acf14a20)

> +{
+	Sci_Position start, len;
+} SelectionRange;
+
+
+#define CURSOR_PLACEHOLDER "_" /* Would rather use … but not all docs are unicode */
+/* Replaces the internal cursor markers with the placeholder suitable for
+ * display. Except for the first cursor if indicator_for_first is FALSE,
+ * which is simply deleted.
+ *
+ * Returns insertion points as SelectionRange list, so that the caller
+ * can use the positions (currently for indicators). */
+static GSList *replace_cursor_markers(GeanyEditor *editor, GString *template,
+									  gboolean indicator_for_first)
+{
+	gssize cur_index = -1;

Unused variable (871e72cc03c2d9501a54652ebeb0ca15acf14a20)

> +
+
+#define CURSOR_PLACEHOLDER "_" /* Would rather use … but not all docs are unicode */
+/* Replaces the internal cursor markers with the placeholder suitable for
+ * display. Except for the first cursor if indicator_for_first is FALSE,
+ * which is simply deleted.
+ *
+ * Returns insertion points as SelectionRange list, so that the caller
+ * can use the positions (currently for indicators). */
+static GSList *replace_cursor_markers(GeanyEditor *editor, GString *template,
+									  gboolean indicator_for_first)
+{
+	gssize cur_index = -1;
+	gint i = 0;
+	GSList *temp_list = NULL;
+	gint cursor_steps = 0, old_cursor = 0;

`old_cursor` is unused (871e72cc03c2d9501a54652ebeb0ca15acf14a20)

>  /* Move the cursor to the next specified cursor position in an inserted snippet.
  * Can, and should, be optimized to give better results */
-void editor_goto_next_snippet_cursor(GeanyEditor *editor)
+gboolean editor_goto_next_snippet_cursor(GeanyEditor *editor)
 {
 	ScintillaObject *sci = editor->sci;
 	gint current_pos = sci_get_current_position(sci);

This is now unused (871e72cc03c2d9501a54652ebeb0ca15acf14a20)

>  
-	snippet_cursor_insert_pos = sci_get_current_position(sci);
+	/* Set cursor to the requested index, or by default to after the snippet */
+	if (cursor_index >= 0)
+		sci_set_current_position(sci, insert_pos + idx, FALSE);

`idx` can be left uninitialized if both `newline_indent_size` and `cursor_index` are not `-1`.
I tried to fix it in 734e0604254bcb9f12285c54309e7e71394f75c8, but I'm not 100% certain initializing to `0` is correct in all other cases.

> @@ -2394,6 +2388,50 @@ static void fix_indentation(GeanyEditor *editor, GString *buf)
 }
 
 
+typedef struct
+{
+	Sci_Position start, len;
+} SelectionRange;
+
+
+#define CURSOR_PLACEHOLDER "_" /* Would rather use … but not all docs are unicode */

Why not use `...` (ASCII ellipsis)?  I tried to do that in 957b49b868214a4aea81641dedeabb98a497e4c1 and it seems to work great, and looks better to me.

>  	{
-		gint offset;
-
-		offset = GPOINTER_TO_INT(g_queue_pop_head(snippet_offsets));
-		if (current_pos > snippet_cursor_insert_pos)
-			snippet_cursor_insert_pos = offset + current_pos;
-		else
-			snippet_cursor_insert_pos += offset;
-
-		sci_set_current_position(sci, snippet_cursor_insert_pos, TRUE);
+		sci_indicator_set(sci, GEANY_INDICATOR_SNIPPET);
+		sci_set_selection(sci, sel.start, sel.start + sel.len);
+		return TRUE;
 	}
 	else
 	{
 		utils_beep();

Now the keybinding can be shared (and IIUC that's your indention, and it seems to work pretty nicely with <kbd>Tab</kbd> otherwise), this function probably shouldn't beep here.  It's pretty annoying to get a beep every time you press <kbd>Tab</kbd> (if bound to this).
Ideally I guess it would beep if there's no other handling of the keybinding, but that's not really something that we can do without huge amounts of hackery -- which is not worth it.

Done in dcea941af17b4ede20d4da87ac60cf90efd82448

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany/pull/1470#pullrequestreview-51674789
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.geany.org/pipermail/github-comments/attachments/20170723/91d81833/attachment.html>