This PR will fix #915 You can view, comment on, or merge this pull request online at:
https://github.com/geany/geany-plugins/pull/922
-- Commit Summary --
* Disable menu general menu item in case of unsupported file type
-- File Changes --
M tableconvert/src/tableconvert.c (42) M tableconvert/src/tableconvert.h (4) M tableconvert/src/tableconvert_ui.c (2)
-- Patch Links --
https://github.com/geany/geany-plugins/pull/922.patch https://github.com/geany/geany-plugins/pull/922.diff
LarsGit223 requested changes on this pull request.
@@ -338,6 +351,31 @@ void cb_table_convert_type(G_GNUC_UNUSED GtkMenuItem *menuitem, G_GNUC_UNUSED gp
convert_to_table(TRUE, GPOINTER_TO_INT(gdata)); }
+void cb_table_convert_change_document(G_GNUC_UNUSED GtkMenuItem *menuitem, G_GNUC_UNUSED gpointer gdata)
This is a local function so IMHO it should be static and only declared in ```tableconvert.c``` not in ```tableconvert.h```.
@@ -338,6 +351,31 @@ void cb_table_convert_type(G_GNUC_UNUSED GtkMenuItem *menuitem, G_GNUC_UNUSED gp
convert_to_table(TRUE, GPOINTER_TO_INT(gdata)); }
+void cb_table_convert_change_document(G_GNUC_UNUSED GtkMenuItem *menuitem, G_GNUC_UNUSED gpointer gdata) +{ + set_activate_state(); +} + +void set_activate_state() +{
The same here. This is only a static function.
@@ -71,5 +71,7 @@ extern TableConvertRule tablerules[];
extern void cb_table_convert(G_GNUC_UNUSED GtkMenuItem *menuitem, G_GNUC_UNUSED gpointer gdata); extern void cb_table_convert_type(G_GNUC_UNUSED GtkMenuItem *menuitem, gpointer gdata); extern void convert_to_table(gboolean header, gint file_type); +extern void cb_table_convert_change_document(G_GNUC_UNUSED GtkMenuItem *menuitem, gpointer gdata); +extern void set_activate_state();
I suggest to move this two function declarations above to the beginning of ```tableconvert.c```. And I would remove the not required ```extern```.
@@ -338,6 +351,31 @@ void cb_table_convert_type(G_GNUC_UNUSED GtkMenuItem *menuitem, G_GNUC_UNUSED gp
convert_to_table(TRUE, GPOINTER_TO_INT(gdata)); }
+void cb_table_convert_change_document(G_GNUC_UNUSED GtkMenuItem *menuitem, G_GNUC_UNUSED gpointer gdata) +{ + set_activate_state(); +} + +void set_activate_state() +{ + // getting document + GeanyDocument *doc = NULL; + doc = document_get_current(); + + if ( + doc != NULL && (
This should also check if there is a current selection or not. The function ```convert_to_table()``` is only doing something if there is a selection (```if (sci_has_selection(doc->editor->sci)) ...```).
@@ -35,6 +35,19 @@ PLUGIN_SET_TRANSLATABLE_INFO(
GeanyPlugin *geany_plugin; GeanyData *geany_data;
+ +PluginCallback plugin_callbacks[] = +{ + { "editor-notify", (GCallback) &cb_table_convert_change_document, FALSE, NULL }, + { "document-activate", (GCallback) &cb_table_convert_change_document, FALSE, NULL }, + { "document-filetype-set", (GCallback) &cb_table_convert_change_document, FALSE, NULL }, + { "document-new", (GCallback) &cb_table_convert_change_document, FALSE, NULL}, + { "geany-startup-complete", (GCallback) &cb_table_convert_change_document, FALSE, NULL }, + { "document-close", (GCallback) &cb_table_convert_change_document, FALSE, NULL}, + { NULL, NULL, FALSE, NULL } +};
Do you really need all of this callbacks? I guess at least ```geany-startup-complete``` is not required or am I overseeing something?
@frlan: it would be nice if you could prefix the commit message with ```tableconvert: ```
frlan commented on this pull request.
@@ -338,6 +351,31 @@ void cb_table_convert_type(G_GNUC_UNUSED GtkMenuItem *menuitem, G_GNUC_UNUSED gp
convert_to_table(TRUE, GPOINTER_TO_INT(gdata)); }
+void cb_table_convert_change_document(G_GNUC_UNUSED GtkMenuItem *menuitem, G_GNUC_UNUSED gpointer gdata) +{ + set_activate_state(); +} + +void set_activate_state() +{ + // getting document + GeanyDocument *doc = NULL; + doc = document_get_current(); + + if ( + doc != NULL && (
Good idea -- but not yet sure it's really needed.
LarsGit223 commented on this pull request.
@@ -338,6 +351,31 @@ void cb_table_convert_type(G_GNUC_UNUSED GtkMenuItem *menuitem, G_GNUC_UNUSED gp
convert_to_table(TRUE, GPOINTER_TO_INT(gdata)); }
+void cb_table_convert_change_document(G_GNUC_UNUSED GtkMenuItem *menuitem, G_GNUC_UNUSED gpointer gdata) +{ + set_activate_state(); +} + +void set_activate_state() +{ + // getting document + GeanyDocument *doc = NULL; + doc = document_get_current(); + + if ( + doc != NULL && (
Well, the menu item is doing nothing if there is no selection. IMHO it should check the selection or if not the user should get a message that a selection is needed before selecting the menu item.
frlan commented on this pull request.
@@ -338,6 +351,31 @@ void cb_table_convert_type(G_GNUC_UNUSED GtkMenuItem *menuitem, G_GNUC_UNUSED gp
convert_to_table(TRUE, GPOINTER_TO_INT(gdata)); }
+void cb_table_convert_change_document(G_GNUC_UNUSED GtkMenuItem *menuitem, G_GNUC_UNUSED gpointer gdata) +{ + set_activate_state(); +} + +void set_activate_state() +{ + // getting document + GeanyDocument *doc = NULL; + doc = document_get_current(); + + if ( + doc != NULL && (
It's the wrong place for getting a selection
LarsGit223 commented on this pull request.
@@ -338,6 +351,31 @@ void cb_table_convert_type(G_GNUC_UNUSED GtkMenuItem *menuitem, G_GNUC_UNUSED gp
convert_to_table(TRUE, GPOINTER_TO_INT(gdata)); }
+void cb_table_convert_change_document(G_GNUC_UNUSED GtkMenuItem *menuitem, G_GNUC_UNUSED gpointer gdata) +{ + set_activate_state(); +} + +void set_activate_state() +{ + // getting document + GeanyDocument *doc = NULL; + doc = document_get_current(); + + if ( + doc != NULL && (
Can you explain what you mean please? I mean it would be good to check if there is a selection, not really getting it (there is the ```doc``` pointer and that's all we need?).
frlan commented on this pull request.
@@ -338,6 +351,31 @@ void cb_table_convert_type(G_GNUC_UNUSED GtkMenuItem *menuitem, G_GNUC_UNUSED gp
convert_to_table(TRUE, GPOINTER_TO_INT(gdata)); }
+void cb_table_convert_change_document(G_GNUC_UNUSED GtkMenuItem *menuitem, G_GNUC_UNUSED gpointer gdata) +{ + set_activate_state(); +} + +void set_activate_state() +{ + // getting document + GeanyDocument *doc = NULL; + doc = document_get_current(); + + if ( + doc != NULL && (
At this point yes. It's either the menu is active or not. The actually handling i done in convert_to_table().
@frlan pushed 1 commit.
b3cb294f589d7cd6651f6faaf58f0d7964c2df4b Tableconvert: Disable menu general menu item in case of unsupported file type
LarsGit223 requested changes on this pull request.
Fine for me, except that ```cb_table_convert_change_document``` should also be a static function.
@@ -35,6 +35,19 @@ PLUGIN_SET_TRANSLATABLE_INFO(
GeanyPlugin *geany_plugin; GeanyData *geany_data;
+ +PluginCallback plugin_callbacks[] = +{ + { "editor-notify", (GCallback) &cb_table_convert_change_document, FALSE, NULL }, + { "document-activate", (GCallback) &cb_table_convert_change_document, FALSE, NULL }, + { "document-filetype-set", (GCallback) &cb_table_convert_change_document, FALSE, NULL }, + { "document-new", (GCallback) &cb_table_convert_change_document, FALSE, NULL}, + { "geany-startup-complete", (GCallback) &cb_table_convert_change_document, FALSE, NULL }, + { "document-close", (GCallback) &cb_table_convert_change_document, FALSE, NULL}, + { NULL, NULL, FALSE, NULL } +};
I guess that's fine now.
@@ -338,6 +351,31 @@ void cb_table_convert_type(G_GNUC_UNUSED GtkMenuItem *menuitem, G_GNUC_UNUSED gp
convert_to_table(TRUE, GPOINTER_TO_INT(gdata)); }
+void cb_table_convert_change_document(G_GNUC_UNUSED GtkMenuItem *menuitem, G_GNUC_UNUSED gpointer gdata)
Should still be static. Unless you prefer it that way.
@@ -338,6 +351,31 @@ void cb_table_convert_type(G_GNUC_UNUSED GtkMenuItem *menuitem, G_GNUC_UNUSED gp
convert_to_table(TRUE, GPOINTER_TO_INT(gdata)); }
+void cb_table_convert_change_document(G_GNUC_UNUSED GtkMenuItem *menuitem, G_GNUC_UNUSED gpointer gdata) +{ + set_activate_state(); +} + +void set_activate_state() +{ + // getting document + GeanyDocument *doc = NULL; + doc = document_get_current(); + + if ( + doc != NULL && (
Ok, when the events are fired, a document is most likely just opened or switched to so there most likely cannot be a selection (yet).
@@ -70,6 +70,6 @@ extern TableConvertRule tablerules[];
extern void cb_table_convert(G_GNUC_UNUSED GtkMenuItem *menuitem, G_GNUC_UNUSED gpointer gdata); extern void cb_table_convert_type(G_GNUC_UNUSED GtkMenuItem *menuitem, gpointer gdata); -extern void convert_to_table(gboolean header, gint file_type); +extern void cb_table_convert_change_document(G_GNUC_UNUSED GtkMenuItem *menuitem, gpointer gdata);
See above. Should be static and removed from the header file.
LarsGit223 commented on this pull request.
@@ -27,7 +27,7 @@
#include <geanyplugin.h> #include <gtk/gtk.h> - +#include <geany/filetypes.h>
This is not required. I removed it and can still build Geany-Plugins. Also it makes the Travis-CI build fail for some reason (but I don't know why).
github-comments@lists.geany.org