Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Tableconvert: Disable menu general menu item in case of unsupported file type #922

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Tableconvert: Disable menu general menu item in case of unsupported file type #922

wants to merge 1 commit into from

Conversation

frlan
Copy link
Member

@frlan frlan commented Oct 6, 2019

This PR will fix #915

lpaulsen93
lpaulsen93 requested changes Oct 6, 2019
View reviewed changes
tableconvert/src/tableconvert.c
@@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a local function so IMHO it should be static and only declared in tableconvert.c not in tableconvert.h.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should still be static. Unless you prefer it that way.

tableconvert/src/tableconvert.c
}

void set_activate_state()
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same here. This is only a static function.

tableconvert/src/tableconvert.h Outdated
@@ -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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest to move this two function declarations above to the beginning of tableconvert.c. And I would remove the not required extern.

tableconvert/src/tableconvert.c
doc = document_get_current();

if (
doc != NULL && (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea -- but not yet sure it's really needed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's the wrong place for getting a selection

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this point yes. It's either the menu is active or not. The actually handling i done in convert_to_table().

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

tableconvert/src/tableconvert.c
{ "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 }
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you really need all of this callbacks? I guess at least geany-startup-complete is not required or am I overseeing something?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that's fine now.

@lpaulsen93
Copy link
Contributor

@frlan: it would be nice if you could prefix the commit message with tableconvert:

@frlan frlan changed the title Disable menu general menu item in case of unsupported file type Tableconvert: Disable menu general menu item in case of unsupported file type Oct 7, 2019
@frlan frlan force-pushed the feature/tableconvert/disable_on_wrong_filetype branch from e82c24c to b3cb294 Compare October 9, 2019 16:29
@frlan frlan requested a review from lpaulsen93 October 9, 2019 16:29
lpaulsen93
lpaulsen93 requested changes Oct 9, 2019
View reviewed changes
Copy link
Contributor

@lpaulsen93 lpaulsen93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine for me, except that cb_table_convert_change_document should also be a static function.

tableconvert/src/tableconvert.c
{ "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 }
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that's fine now.

tableconvert/src/tableconvert.c
@@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should still be static. Unless you prefer it that way.

tableconvert/src/tableconvert.c
doc = document_get_current();

if (
doc != NULL && (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

tableconvert/src/tableconvert.h
@@ -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);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above. Should be static and removed from the header file.

lpaulsen93
lpaulsen93 reviewed Oct 9, 2019
View reviewed changes
tableconvert/src/tableconvert.h
@@ -27,7 +27,7 @@

#include <geanyplugin.h>
#include <gtk/gtk.h>

#include <geany/filetypes.h>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@hg2581 hg2581 mentioned this pull request May 30, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lpaulsen93 lpaulsen93 lpaulsen93 requested changes

Assignees
No one assigned
Labels
pending fixes plugin/tableconvert
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

tableconvert: disable menu item on not supported filetypes
2 participants
@frlan @lpaulsen93