[Github-comments] [geany/geany] Correction and enhancements for context action (#1841)

Alex Plotnikov notifications at xxxxx
Wed Apr 25 03:40:30 UTC 2018


About

 		case GEANY_KEYS_EDITOR_CONTEXTACTION:
-			if (check_current_word(doc, FALSE))
+			//do not check here: if (check_current_word(doc, FALSE))

Why?
I explained this. Without this correction the hotkeying on the empty
selection doesn't work. It's not the place to check the access to the
function. The function should do it.

On Wed, Apr 25, 2018 at 2:23 AM, elextr <notifications at github.com> wrote:

> *@elextr* requested changes on this pull request.
>
> As said in specific comments, the goto changes should be a separate PR
> from the context action changes. Other specific comments.
>
> Documentation needed.
> ------------------------------
>
> In src/build.c
> <https://github.com/geany/geany/pull/1841#discussion_r183905312>:
>
> > @@ -664,7 +664,8 @@ static void clear_all_errors(void)
>
>  /* Replaces occurrences of %e and %p with the appropriate filenames and
>   * %l with current line number. %d and %p replacements should be in UTF8 */
> -static gchar *build_replace_placeholder(const GeanyDocument *doc, const gchar *src)
> +GEANY_API_SYMBOL
>
> This doesn't need to be in the API, just remove the static.
> ------------------------------
>
> In src/callbacks.c
> <https://github.com/geany/geany/pull/1841#discussion_r183905945>:
>
> > @@ -1467,6 +1467,9 @@ void on_context_action1_activate(GtkMenuItem *menuitem, gpointer user_data)
>  	}
>  	else
>  	{
> +        /* take a current word under caret */
>
> IIUC editor_info.current_word is kept updated with the current word, you
> don't need to find it again.
> ------------------------------
>
> In src/callbacks.c
> <https://github.com/geany/geany/pull/1841#discussion_r183906397>:
>
> > @@ -1486,8 +1489,11 @@ void on_context_action1_activate(GtkMenuItem *menuitem, gpointer user_data)
>  	/* substitute the wildcard %s and run the command if it is non empty */
>  	if (G_LIKELY(!EMPTY(command)))
>  	{
> -		gchar *command_line = g_strdup(command);
> -
> +        /* add %f, %d, %e, %p, %l placeholders */
> +        gchar *command_tmp = g_strdup(command);
>
> build_replace_placeholder() does not modify the input string, so no need
> to copy it.
> ------------------------------
>
> In src/editor.c
> <https://github.com/geany/geany/pull/1841#discussion_r183906784>:
>
> > @@ -340,7 +340,23 @@ static gboolean on_editor_button_press_event(GtkWidget *widget, GdkEventButton *
>  			current_word, sizeof current_word, NULL);
>
>  		can_goto = sci_has_selection(editor->sci) || current_word[0] != '\0';
> -		ui_update_popup_goto_items(can_goto);
> +
> +
> +		/* create a list of locks/unlocks for the goto and context actions */
> +        gchar cangos[64];  // never reachable maximum
> +        for (gint i=0; i<64; i++)
>
> Don't use a manifest constant, use a #defined symbol. And 64 seems silly
> large.
> ------------------------------
>
> In src/editor.c
> <https://github.com/geany/geany/pull/1841#discussion_r183906955>:
>
> > @@ -340,7 +340,23 @@ static gboolean on_editor_button_press_event(GtkWidget *widget, GdkEventButton *
>  			current_word, sizeof current_word, NULL);
>
>  		can_goto = sci_has_selection(editor->sci) || current_word[0] != '\0';
> -		ui_update_popup_goto_items(can_goto);
> +
> +
> +		/* create a list of locks/unlocks for the goto and context actions */
> +        gchar cangos[64];  // never reachable maximum
> +        for (gint i=0; i<64; i++)
> +            cangos[i]=(can_goto?'1':'0'); // flag the whole group
>
> If its meant to be a boolean use the gboolean type not characters '1' and
> '0'.
> ------------------------------
>
> In src/editor.c
> <https://github.com/geany/geany/pull/1841#discussion_r183907296>:
>
> > @@ -340,7 +340,23 @@ static gboolean on_editor_button_press_event(GtkWidget *widget, GdkEventButton *
>  			current_word, sizeof current_word, NULL);
>
>  		can_goto = sci_has_selection(editor->sci) || current_word[0] != '\0';
> -		ui_update_popup_goto_items(can_goto);
> +
> +
>
> This whole section fixing/changing the existing goto functionality should
> be in a separate PR from context action. Don't mix different things in one
> PR, if one of them is rejected/delayed both will be.
> ------------------------------
>
> In src/keybindings.c
> <https://github.com/geany/geany/pull/1841#discussion_r183907360>:
>
> > @@ -2196,7 +2196,7 @@ static gboolean cb_func_editor_action(guint key_id)
>  			editor_show_calltip(doc->editor, -1);
>  			break;
>  		case GEANY_KEYS_EDITOR_CONTEXTACTION:
> -			if (check_current_word(doc, FALSE))
> +			//do not check here: if (check_current_word(doc, FALSE))
>
> Why?
> ------------------------------
>
> In src/ui_utils.c
> <https://github.com/geany/geany/pull/1841#discussion_r183907690>:
>
> >  {
> -	guint i, len;
> -	len = G_N_ELEMENTS(widgets.popup_goto_items);
> -	for (i = 0; i < len; i++)
> -		ui_widget_set_sensitive(widgets.popup_goto_items[i], enable);
> +    guint i, len;
> +    len = G_N_ELEMENTS(widgets.popup_goto_items);
> +    for (i = 0; i < len; i++)
> +        ui_widget_set_sensitive(widgets.popup_goto_items[i], enable[i]=='1');
>
> Indentation is wrong, Geany is tab only indentation. See above comments re
> boolean and separate PR.
>
>> You are receiving this because you authored the thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/geany/geany/pull/1841#pullrequestreview-114995112>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/Ajmr5NyTlA-GCxwT3pDesmEsKWTPb3iyks5tr7PigaJpZM4Thz7Y>
> .
>


-- 
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/1841#issuecomment-384151455
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.geany.org/pipermail/github-comments/attachments/20180424/844b0569/attachment-0001.html>


More information about the Github-comments mailing list