<p><b>@b4n</b> commented on this pull request.</p>
<p>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 <code>def</code> for Python, which IMO would be handier with a <code>%cursor%</code> at the end), but it might require some good testing for pros and cons.</p>
<p>The uninitialized var issue might be serious though. See <a href="https://github.com/geany/geany/pull/1554" class="issue-link js-issue-link" data-url="https://github.com/geany/geany/issues/1554" data-id="244976429" data-error-text="Failed to load issue title" data-permission-text="Issue title is private">#1554</a>.</p><hr>
<p>In <a href="https://github.com/geany/geany/pull/1470#discussion_r128950898">src/editor.c</a>:</p>
<pre style='color:#555'>> g_string_free(buf, TRUE);
}
+static gboolean find_next_snippet_indicator(GeanyEditor *editor, SelectionRange *sel)
+{
+ ScintillaObject *sci = editor->sci;
+ gint val;
</pre>
<p>unused variable (see <a href="https://github.com/geany/geany/commit/871e72cc03c2d9501a54652ebeb0ca15acf14a20" class="commit-link"><tt>871e72c</tt></a>)</p>
<hr>
<p>In <a href="https://github.com/geany/geany/pull/1470#discussion_r128950966">src/editor.c</a>:</p>
<pre style='color:#555'>> +{
+ 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;
</pre>
<p>Unused variable (<a href="https://github.com/geany/geany/commit/871e72cc03c2d9501a54652ebeb0ca15acf14a20" class="commit-link"><tt>871e72c</tt></a>)</p>
<hr>
<p>In <a href="https://github.com/geany/geany/pull/1470#discussion_r128951014">src/editor.c</a>:</p>
<pre style='color:#555'>> +
+
+#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;
</pre>
<p><code>old_cursor</code> is unused (<a href="https://github.com/geany/geany/commit/871e72cc03c2d9501a54652ebeb0ca15acf14a20" class="commit-link"><tt>871e72c</tt></a>)</p>
<hr>
<p>In <a href="https://github.com/geany/geany/pull/1470#discussion_r128951058">src/editor.c</a>:</p>
<pre style='color:#555'>> /* 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);
</pre>
<p>This is now unused (<a href="https://github.com/geany/geany/commit/871e72cc03c2d9501a54652ebeb0ca15acf14a20" class="commit-link"><tt>871e72c</tt></a>)</p>
<hr>
<p>In <a href="https://github.com/geany/geany/pull/1470#discussion_r128951237">src/editor.c</a>:</p>
<pre style='color:#555'>>
- 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);
</pre>
<p><code>idx</code> can be left uninitialized if both <code>newline_indent_size</code> and <code>cursor_index</code> are not <code>-1</code>.<br>
I tried to fix it in <a href="https://github.com/geany/geany/commit/734e0604254bcb9f12285c54309e7e71394f75c8" class="commit-link"><tt>734e060</tt></a>, but I'm not 100% certain initializing to <code>0</code> is correct in all other cases.</p>
<hr>
<p>In <a href="https://github.com/geany/geany/pull/1470#discussion_r128951377">src/editor.c</a>:</p>
<pre style='color:#555'>> @@ -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 */
</pre>
<p>Why not use <code>...</code> (ASCII ellipsis)? I tried to do that in <a href="https://github.com/geany/geany/commit/957b49b868214a4aea81641dedeabb98a497e4c1" class="commit-link"><tt>957b49b</tt></a> and it seems to work great, and looks better to me.</p>
<hr>
<p>In <a href="https://github.com/geany/geany/pull/1470#discussion_r128951889">src/editor.c</a>:</p>
<pre style='color:#555'>> {
- 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();
</pre>
<p>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).<br>
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.</p>
<p>Done in <a href="https://github.com/geany/geany/commit/dcea941af17b4ede20d4da87ac60cf90efd82448" class="commit-link"><tt>dcea941</tt></a></p>
<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />You are receiving this because you are subscribed to this thread.<br />Reply to this email directly, <a href="https://github.com/geany/geany/pull/1470#pullrequestreview-51674789">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/ABDrJy-1GPD1EA-hoPdX2lrK1R4PbVfAks5sRC1ngaJpZM4NCkaj">mute the thread</a>.<img alt="" height="1" src="https://github.com/notifications/beacon/ABDrJ7cT6n4x9QggPwNhMtZRFcrT00txks5sRC1ngaJpZM4NCkaj.gif" width="1" /></p>
<div itemscope itemtype="http://schema.org/EmailMessage">
<div itemprop="action" itemscope itemtype="http://schema.org/ViewAction">
<link itemprop="url" href="https://github.com/geany/geany/pull/1470#pullrequestreview-51674789"></link>
<meta itemprop="name" content="View Pull Request"></meta>
</div>
<meta itemprop="description" content="View this Pull Request on GitHub"></meta>
</div>
<script type="application/json" data-scope="inboxmarkup">{"api_version":"1.0","publisher":{"api_key":"05dde50f1d1a384dd78767c55493e4bb","name":"GitHub"},"entity":{"external_key":"github/geany/geany","title":"geany/geany","subtitle":"GitHub repository","main_image_url":"https://cloud.githubusercontent.com/assets/143418/17495839/a5054eac-5d88-11e6-95fc-7290892c7bb5.png","avatar_image_url":"https://cloud.githubusercontent.com/assets/143418/15842166/7c72db34-2c0b-11e6-9aed-b52498112777.png","action":{"name":"Open in GitHub","url":"https://github.com/geany/geany"}},"updates":{"snippets":[{"icon":"PERSON","message":"@b4n commented on #1470"}],"action":{"name":"View Pull Request","url":"https://github.com/geany/geany/pull/1470#pullrequestreview-51674789"}}}</script>