About<br>
<br>
case GEANY_KEYS_EDITOR_CONTEXTACTION:<br>
- if (check_current_word(doc, FALSE))<br>
+ //do not check here: if (check_current_word(doc, FALSE))<br>
<br>
Why?<br>
I explained this. Without this correction the hotkeying on the empty<br>
selection doesn't work. It's not the place to check the access to the<br>
function. The function should do it.<br>
<br>
On Wed, Apr 25, 2018 at 2:23 AM, elextr <notifications@github.com> wrote:<br>
<br>
> *@elextr* requested changes on this pull request.<br>
><br>
> As said in specific comments, the goto changes should be a separate PR<br>
> from the context action changes. Other specific comments.<br>
><br>
> Documentation needed.<br>
> ------------------------------<br>
><br>
> In src/build.c<br>
> <https://github.com/geany/geany/pull/1841#discussion_r183905312>:<br>
><br>
> > @@ -664,7 +664,8 @@ static void clear_all_errors(void)<br>
><br>
> /* Replaces occurrences of %e and %p with the appropriate filenames and<br>
> * %l with current line number. %d and %p replacements should be in UTF8 */<br>
> -static gchar *build_replace_placeholder(const GeanyDocument *doc, const gchar *src)<br>
> +GEANY_API_SYMBOL<br>
><br>
> This doesn't need to be in the API, just remove the static.<br>
> ------------------------------<br>
><br>
> In src/callbacks.c<br>
> <https://github.com/geany/geany/pull/1841#discussion_r183905945>:<br>
><br>
> > @@ -1467,6 +1467,9 @@ void on_context_action1_activate(GtkMenuItem *menuitem, gpointer user_data)<br>
> }<br>
> else<br>
> {<br>
> + /* take a current word under caret */<br>
><br>
> IIUC editor_info.current_word is kept updated with the current word, you<br>
> don't need to find it again.<br>
> ------------------------------<br>
><br>
> In src/callbacks.c<br>
> <https://github.com/geany/geany/pull/1841#discussion_r183906397>:<br>
><br>
> > @@ -1486,8 +1489,11 @@ void on_context_action1_activate(GtkMenuItem *menuitem, gpointer user_data)<br>
> /* substitute the wildcard %s and run the command if it is non empty */<br>
> if (G_LIKELY(!EMPTY(command)))<br>
> {<br>
> - gchar *command_line = g_strdup(command);<br>
> -<br>
> + /* add %f, %d, %e, %p, %l placeholders */<br>
> + gchar *command_tmp = g_strdup(command);<br>
><br>
> build_replace_placeholder() does not modify the input string, so no need<br>
> to copy it.<br>
> ------------------------------<br>
><br>
> In src/editor.c<br>
> <https://github.com/geany/geany/pull/1841#discussion_r183906784>:<br>
><br>
> > @@ -340,7 +340,23 @@ static gboolean on_editor_button_press_event(GtkWidget *widget, GdkEventButton *<br>
> current_word, sizeof current_word, NULL);<br>
><br>
> can_goto = sci_has_selection(editor->sci) || current_word[0] != '\0';<br>
> - ui_update_popup_goto_items(can_goto);<br>
> +<br>
> +<br>
> + /* create a list of locks/unlocks for the goto and context actions */<br>
> + gchar cangos[64]; // never reachable maximum<br>
> + for (gint i=0; i<64; i++)<br>
><br>
> Don't use a manifest constant, use a #defined symbol. And 64 seems silly<br>
> large.<br>
> ------------------------------<br>
><br>
> In src/editor.c<br>
> <https://github.com/geany/geany/pull/1841#discussion_r183906955>:<br>
><br>
> > @@ -340,7 +340,23 @@ static gboolean on_editor_button_press_event(GtkWidget *widget, GdkEventButton *<br>
> current_word, sizeof current_word, NULL);<br>
><br>
> can_goto = sci_has_selection(editor->sci) || current_word[0] != '\0';<br>
> - ui_update_popup_goto_items(can_goto);<br>
> +<br>
> +<br>
> + /* create a list of locks/unlocks for the goto and context actions */<br>
> + gchar cangos[64]; // never reachable maximum<br>
> + for (gint i=0; i<64; i++)<br>
> + cangos[i]=(can_goto?'1':'0'); // flag the whole group<br>
><br>
> If its meant to be a boolean use the gboolean type not characters '1' and<br>
> '0'.<br>
> ------------------------------<br>
><br>
> In src/editor.c<br>
> <https://github.com/geany/geany/pull/1841#discussion_r183907296>:<br>
><br>
> > @@ -340,7 +340,23 @@ static gboolean on_editor_button_press_event(GtkWidget *widget, GdkEventButton *<br>
> current_word, sizeof current_word, NULL);<br>
><br>
> can_goto = sci_has_selection(editor->sci) || current_word[0] != '\0';<br>
> - ui_update_popup_goto_items(can_goto);<br>
> +<br>
> +<br>
><br>
> This whole section fixing/changing the existing goto functionality should<br>
> be in a separate PR from context action. Don't mix different things in one<br>
> PR, if one of them is rejected/delayed both will be.<br>
> ------------------------------<br>
><br>
> In src/keybindings.c<br>
> <https://github.com/geany/geany/pull/1841#discussion_r183907360>:<br>
><br>
> > @@ -2196,7 +2196,7 @@ static gboolean cb_func_editor_action(guint key_id)<br>
> editor_show_calltip(doc->editor, -1);<br>
> break;<br>
> case GEANY_KEYS_EDITOR_CONTEXTACTION:<br>
> - if (check_current_word(doc, FALSE))<br>
> + //do not check here: if (check_current_word(doc, FALSE))<br>
><br>
> Why?<br>
> ------------------------------<br>
><br>
> In src/ui_utils.c<br>
> <https://github.com/geany/geany/pull/1841#discussion_r183907690>:<br>
><br>
> > {<br>
> - guint i, len;<br>
> - len = G_N_ELEMENTS(widgets.popup_goto_items);<br>
> - for (i = 0; i < len; i++)<br>
> - ui_widget_set_sensitive(widgets.popup_goto_items[i], enable);<br>
> + guint i, len;<br>
> + len = G_N_ELEMENTS(widgets.popup_goto_items);<br>
> + for (i = 0; i < len; i++)<br>
> + ui_widget_set_sensitive(widgets.popup_goto_items[i], enable[i]=='1');<br>
><br>
> Indentation is wrong, Geany is tab only indentation. See above comments re<br>
> boolean and separate PR.<br>
><br>
> —<br>
> You are receiving this because you authored the thread.<br>
> Reply to this email directly, view it on GitHub<br>
> <https://github.com/geany/geany/pull/1841#pullrequestreview-114995112>,<br>
> or mute the thread<br>
> <https://github.com/notifications/unsubscribe-auth/Ajmr5NyTlA-GCxwT3pDesmEsKWTPb3iyks5tr7PigaJpZM4Thz7Y><br>
> .<br>
><br>
<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/1841#issuecomment-384151455">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/ABDrJ3IHEILyNu_uRgJWVZJfgnpXeT3Pks5tr_AugaJpZM4Thz7Y">mute the thread</a>.<img src="https://github.com/notifications/beacon/ABDrJ-rvHhbYeB8D-6SGqn5PX27mYk_9ks5tr_AugaJpZM4Thz7Y.gif" height="1" width="1" alt="" /></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/1841#issuecomment-384151455"></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":"@aplsimple in #1841: About\n\n \t\tcase GEANY_KEYS_EDITOR_CONTEXTACTION:\n-\t\t\tif (check_current_word(doc, FALSE))\n+\t\t\t//do not check here: if (check_current_word(doc, FALSE))\n\nWhy?\nI explained this. Without this correction the hotkeying on the empty\nselection doesn't work. It's not the place to check the access to the\nfunction. The function should do it.\n\nOn Wed, Apr 25, 2018 at 2:23 AM, elextr \u003cnotifications@github.com\u003e wrote:\n\n\u003e *@elextr* requested changes on this pull request.\n\u003e\n\u003e As said in specific comments, the goto changes should be a separate PR\n\u003e from the context action changes. Other specific comments.\n\u003e\n\u003e Documentation needed.\n\u003e ------------------------------\n\u003e\n\u003e In src/build.c\n\u003e \u003chttps://github.com/geany/geany/pull/1841#discussion_r183905312\u003e:\n\u003e\n\u003e \u003e @@ -664,7 +664,8 @@ static void clear_all_errors(void)\n\u003e\n\u003e /* Replaces occurrences of %e and %p with the appropriate filenames and\n\u003e * %l with current line number. %d and %p replacements should be in UTF8 */\n\u003e -static gchar *build_replace_placeholder(const GeanyDocument *doc, const gchar *src)\n\u003e +GEANY_API_SYMBOL\n\u003e\n\u003e This doesn't need to be in the API, just remove the static.\n\u003e ------------------------------\n\u003e\n\u003e In src/callbacks.c\n\u003e \u003chttps://github.com/geany/geany/pull/1841#discussion_r183905945\u003e:\n\u003e\n\u003e \u003e @@ -1467,6 +1467,9 @@ void on_context_action1_activate(GtkMenuItem *menuitem, gpointer user_data)\n\u003e \t}\n\u003e \telse\n\u003e \t{\n\u003e + /* take a current word under caret */\n\u003e\n\u003e IIUC editor_info.current_word is kept updated with the current word, you\n\u003e don't need to find it again.\n\u003e ------------------------------\n\u003e\n\u003e In src/callbacks.c\n\u003e \u003chttps://github.com/geany/geany/pull/1841#discussion_r183906397\u003e:\n\u003e\n\u003e \u003e @@ -1486,8 +1489,11 @@ void on_context_action1_activate(GtkMenuItem *menuitem, gpointer user_data)\n\u003e \t/* substitute the wildcard %s and run the command if it is non empty */\n\u003e \tif (G_LIKELY(!EMPTY(command)))\n\u003e \t{\n\u003e -\t\tgchar *command_line = g_strdup(command);\n\u003e -\n\u003e + /* add %f, %d, %e, %p, %l placeholders */\n\u003e + gchar *command_tmp = g_strdup(command);\n\u003e\n\u003e build_replace_placeholder() does not modify the input string, so no need\n\u003e to copy it.\n\u003e ------------------------------\n\u003e\n\u003e In src/editor.c\n\u003e \u003chttps://github.com/geany/geany/pull/1841#discussion_r183906784\u003e:\n\u003e\n\u003e \u003e @@ -340,7 +340,23 @@ static gboolean on_editor_button_press_event(GtkWidget *widget, GdkEventButton *\n\u003e \t\t\tcurrent_word, sizeof current_word, NULL);\n\u003e\n\u003e \t\tcan_goto = sci_has_selection(editor-\u003esci) || current_word[0] != '\\0';\n\u003e -\t\tui_update_popup_goto_items(can_goto);\n\u003e +\n\u003e +\n\u003e +\t\t/* create a list of locks/unlocks for the goto and context actions */\n\u003e + gchar cangos[64]; // never reachable maximum\n\u003e + for (gint i=0; i\u003c64; i++)\n\u003e\n\u003e Don't use a manifest constant, use a #defined symbol. And 64 seems silly\n\u003e large.\n\u003e ------------------------------\n\u003e\n\u003e In src/editor.c\n\u003e \u003chttps://github.com/geany/geany/pull/1841#discussion_r183906955\u003e:\n\u003e\n\u003e \u003e @@ -340,7 +340,23 @@ static gboolean on_editor_button_press_event(GtkWidget *widget, GdkEventButton *\n\u003e \t\t\tcurrent_word, sizeof current_word, NULL);\n\u003e\n\u003e \t\tcan_goto = sci_has_selection(editor-\u003esci) || current_word[0] != '\\0';\n\u003e -\t\tui_update_popup_goto_items(can_goto);\n\u003e +\n\u003e +\n\u003e +\t\t/* create a list of locks/unlocks for the goto and context actions */\n\u003e + gchar cangos[64]; // never reachable maximum\n\u003e + for (gint i=0; i\u003c64; i++)\n\u003e + cangos[i]=(can_goto?'1':'0'); // flag the whole group\n\u003e\n\u003e If its meant to be a boolean use the gboolean type not characters '1' and\n\u003e '0'.\n\u003e ------------------------------\n\u003e\n\u003e In src/editor.c\n\u003e \u003chttps://github.com/geany/geany/pull/1841#discussion_r183907296\u003e:\n\u003e\n\u003e \u003e @@ -340,7 +340,23 @@ static gboolean on_editor_button_press_event(GtkWidget *widget, GdkEventButton *\n\u003e \t\t\tcurrent_word, sizeof current_word, NULL);\n\u003e\n\u003e \t\tcan_goto = sci_has_selection(editor-\u003esci) || current_word[0] != '\\0';\n\u003e -\t\tui_update_popup_goto_items(can_goto);\n\u003e +\n\u003e +\n\u003e\n\u003e This whole section fixing/changing the existing goto functionality should\n\u003e be in a separate PR from context action. Don't mix different things in one\n\u003e PR, if one of them is rejected/delayed both will be.\n\u003e ------------------------------\n\u003e\n\u003e In src/keybindings.c\n\u003e \u003chttps://github.com/geany/geany/pull/1841#discussion_r183907360\u003e:\n\u003e\n\u003e \u003e @@ -2196,7 +2196,7 @@ static gboolean cb_func_editor_action(guint key_id)\n\u003e \t\t\teditor_show_calltip(doc-\u003eeditor, -1);\n\u003e \t\t\tbreak;\n\u003e \t\tcase GEANY_KEYS_EDITOR_CONTEXTACTION:\n\u003e -\t\t\tif (check_current_word(doc, FALSE))\n\u003e +\t\t\t//do not check here: if (check_current_word(doc, FALSE))\n\u003e\n\u003e Why?\n\u003e ------------------------------\n\u003e\n\u003e In src/ui_utils.c\n\u003e \u003chttps://github.com/geany/geany/pull/1841#discussion_r183907690\u003e:\n\u003e\n\u003e \u003e {\n\u003e -\tguint i, len;\n\u003e -\tlen = G_N_ELEMENTS(widgets.popup_goto_items);\n\u003e -\tfor (i = 0; i \u003c len; i++)\n\u003e -\t\tui_widget_set_sensitive(widgets.popup_goto_items[i], enable);\n\u003e + guint i, len;\n\u003e + len = G_N_ELEMENTS(widgets.popup_goto_items);\n\u003e + for (i = 0; i \u003c len; i++)\n\u003e + ui_widget_set_sensitive(widgets.popup_goto_items[i], enable[i]=='1');\n\u003e\n\u003e Indentation is wrong, Geany is tab only indentation. See above comments re\n\u003e boolean and separate PR.\n\u003e\n\u003e —\n\u003e You are receiving this because you authored the thread.\n\u003e Reply to this email directly, view it on GitHub\n\u003e \u003chttps://github.com/geany/geany/pull/1841#pullrequestreview-114995112\u003e,\n\u003e or mute the thread\n\u003e \u003chttps://github.com/notifications/unsubscribe-auth/Ajmr5NyTlA-GCxwT3pDesmEsKWTPb3iyks5tr7PigaJpZM4Thz7Y\u003e\n\u003e .\n\u003e\n"}],"action":{"name":"View Pull Request","url":"https://github.com/geany/geany/pull/1841#issuecomment-384151455"}}}</script>