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

[elextr]

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.

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

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

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

> @@ -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 `#define`d symbol.  And 64 seems silly large.

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

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

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

>  {
-	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 are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany/pull/1841#pullrequestreview-114995112
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.geany.org/pipermail/github-comments/attachments/20180424/dbe7000d/attachment.html>