This commit provides 1 correction and 2.5 enhancements for Geany's context action:
1. The correction is:
- In case you hadn't set the context action, calling a popup menu on a selected text/word brings up a menu with all 'goto & context' items being enabled. The context doesn't work at that, of course. It should not be enabled: 1) when no text is selected 2) even more important: when there is no context action set. This commit corrects this.
2. The enhancements are:
- This commit introduces %f, %d, %e, %p, %l placeholders into the context string, i.e. the same ones as in Compile/ Build/ Run. Without them the context is somehow (indeed much) restricted.
- (As consequence) the set context action is active when there is no selected text or a word under caret. Even if there are no placeholders in the context string it may contain a really useful command to access with a fast hotkey, all the more the context is specific to the file type. This commit always enables the context if it's defined in Geany's settings no matter there is or no placeholder in it.
- (0.5 of enhancement) if there would be some new 'goto & context' menu items with their own requirements for access, this commit would facilitate their implementation.
There are a few bla-bla left. This new Geany's context can respond not only to %s, %f, %d etc., but to all Geany's surroundings. E.g. it can show a current state of the project from different viewpoints. Or perform the various actions for maintainance of it. All that done without leaving Geany IDE. One of innumerable possible implementations is here: https://wiki.geany.org/howtos/using_with_tcl_tk As a matter of fact this commit was born of it.
----
About the modifications of code.
1. For the access to 'build_replace_placeholder' function (introducing %f, %d .. placeholders) I need to modify build.c and build.h.
2. It is included in the proper place of callbacks.c.
3. To unlock the context (and move its access check to other place) I need to comment a line of keybindings.c. Actually this unlocks the hotkey of context.
4. To enable a separate (from other 'goto' items) checking of context access, I need to modify ui_utils.c (and its ui_utils.h).
5. The real access to all 'goto' items (the context including) is checked in editor.c.
You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany/pull/1841
-- Commit Summary --
* Correction and enhancements for context action
-- File Changes --
M src/build.c (3) M src/build.h (1) M src/callbacks.c (10) M src/editor.c (18) M src/keybindings.c (2) M src/ui_utils.c (10) M src/ui_utils.h (2)
-- Patch Links --
https://github.com/geany/geany/pull/1841.patch https://github.com/geany/geany/pull/1841.diff
The context doesn't work at that, of course. It should not be enabled: 1) when no text is selected
IIUC its a deliberate feature to select the word under the cursor if no selection exists, so this will need an option defaulting to off so it won't affect the current behaviour unless the user wants it to.
- even more important: when there is no context action set.
Making the menu item insensitive in this case makes sense.
This commit introduces %f, %d, %e, %p, %l placeholders into the context string, i.e. the same ones as in Compile/ Build/ Run.
Seems a worthwhile improvement.
the set context action is active when there is no selected text or a word under caret.
Erm, didn't your item 1. just propose disabling it in this case?
Finally, this will __NOT__ be accepted without the manual being modified to reflect the changes.
I didn't thoroughly review it, but some of the indentation looks messed up and some places don't follow [Geany's coding conventions/styles](https://github.com/geany/geany/blob/1.33.0/HACKING#L246).
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.
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@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 maximumfor (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 maximumfor (gint i=0; i<64; i++)cangos[i]=(can_goto?'1':'0'); // flag the whole groupIf 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 .
To be consistent with Compile/ Run/ Build the following modification should be done:
gchar *command_orig = g_strdup(command); gchar *command_line = build_replace_placeholder(doc, command_orig); gint isfilecontext = strcmp(command_orig, command_line) != 0; g_free(command_orig); if (isfilecontext && doc && doc->changed) //save file if it's in context { if (!document_save_file(doc, FALSE)) return; }
This provides 'save before run' feature for the edited file if the context string contains %f, %d etc. placeholders. I don't know how to explain this more clearly. Perhaps I'm a bad explainer and a bad boy at that. You folks are much more clear about a separate PR from context action etc.
This provides 'save before run' feature for the edited file if the context string contains %f, %d etc. placeholders.
Thats sensible for %f, the command likely wants to access it, but it need a confirmatory dialog, not to be forced, some users don't want to accidentally write over their files and not all commands will read the file just because it gets the name. Thats why builds only save the current file and even thats got an option to disable it. Not so sure its at all sensible for %d or any other substitutions, they are probably not accessing the file.
OK. What's about a separate PR. Who is the beast?
Not me, I swear.
Ah, then it's the 'hedgehog in the fog' I use as my chromium theme. Or that silly bird with flower.
github-comments@lists.geany.org