@b4n requested changes on this pull request.
> +#define HEXA_NONE -1 /* hexadecimal number not found */
+#define HEXA_MAYBE 0 /* in progress, need scan more
characters */
+#define HEXA_REQUIRE_x 1 /* need character x/X */
+#define HEXA_REQUIRE_0 2 /* need character 0 */
+#define HEXA_REQUIRE_xnumber 3 /* need at least one number */
+#define HEXA_ACCEPT_xnumber 4 /* there may be more numbers */
+#define HEXA_FOUND 5 /* ok, hexadecimal number found */
Sounds like good candidates for an enum
> +#define DECIMAL_NONE -1 /* decimal number not found */
+#define DECIMAL_MAYBE 0 /* in progress, need scan more
characters */
+#define DECIMAL_REQUIRE_number 1 /* need at least one number */
+#define DECIMAL_FOUND 5 /* ok, decimal number found */
Ditto (although less so)
> +#define HEXA_CASE_UNKNOWN -1 /* hexadicmal case not yey known */
+#define HEXA_CASE_UPPER 1 /* upper case detected */
+#define HEXA_CASE_LOWER 0 /* lower case detected */
And ditto
> +
+static gint sci_get_position_after(ScintillaObject *sci, gint start)
+{
+ return (gint) SSM(sci, SCI_POSITIONAFTER, (uptr_t) start, 0);
+}
+
+
+static gint sci_get_position_before(ScintillaObject *sci, gint start)
+{
+ return (gint) SSM(sci, SCI_POSITIONBEFORE, (uptr_t) start, 0);
+}
+
+
+static gboolean on_change_number(gint step)
+{
+ ScintillaObject *sci = document_get_current()->editor->sci;
`document_get_current()` can return `NULL`, you need to guard this against that
case. E.g. ~~I believe~~ you'll get a crash if you hit one of the keybindings
when no document is open (which is a possibility).
```suggestion
GeanyDocument *doc = document_get_current();
ScintillaObject *sci;
if (! doc)
{
return FALSE;
}
sci = doc->editor->sci;
```
> +
+ /* when the number changes sign, the format is reset to avoid a
display shift */
+ if ((positive == FALSE && guessed_number >= 0) || (positive ==
TRUE && guessed_number < 0))
+ {
+ format_length = 0;
+ }
+ else
+ {
+ format_length = digit_end - digit_start;
+ if (format_length > 12)
+ format_length = 0;
+ }
+
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wformat-nonliteral"
+ g_snprintf(format_buf, sizeof(format_buf)-1, "%%0%d%c",
format_length, use_hexa ? (hexaCase == HEXA_CASE_UPPER ? 'X' : 'x') : 'd');
possible nitpick, or real issue: `%d` expects `int`, while `%x` expects
`unsigned int`. `guessed_number` is `gint64` (so signed), so you should
probably use `G_GINT64_FORMAT` for the decimal case. Ideally, you'd derive the
hexadecimal one from it as well (there is `G_GUINT64_FORMAT`, but nothing for
hexadecimal.
> +
+ if ((buf = g_strdup_printf(format_buf, guessed_number)))
+ {
+ sci_set_target_start(sci, digit_start);
+ sci_set_target_end(sci, digit_end);
+ gint nLen = sci_replace_target(sci, buf, FALSE);
+ sci_set_current_position(sci, digit_start + nLen - 1,
TRUE);
+
+ g_free(buf);
+ }
+ }
+
+ return TRUE;
+}
+
+static void configure_response(GtkDialog *dialog, gint response, gpointer data)
You probably should rename this function, at a glance it looks like a callback
for `plugin_configure()`, which is confusing.
> + GTK_DIALOG_MODAL |
> GTK_DIALOG_DESTROY_WITH_PARENT, _("Cancel"),
+ GTK_RESPONSE_CANCEL, _("OK"),
GTK_RESPONSE_OK,
Just for clarity of the label/response pairs
```suggestion
GTK_DIALOG_MODAL |
GTK_DIALOG_DESTROY_WITH_PARENT,
_("Cancel"),
GTK_RESPONSE_CANCEL,
_("OK"), GTK_RESPONSE_OK,
```
> + on_change_number(increment);
+}
+
+static gboolean on_change_number_x(G_GNUC_UNUSED GtkMenuItem * menuitem,
G_GNUC_UNUSED gpointer gdata)
+{
+ if (plugin_data._dialog == NULL)
+ {
+ plugin_data._dialog = gtk_dialog_new_with_buttons(_("Increment
or Decrement number by ..."),
+
GTK_WINDOW(geany->main_widgets->window),
+ GTK_DIALOG_MODAL |
GTK_DIALOG_DESTROY_WITH_PARENT, _("Cancel"),
+ GTK_RESPONSE_CANCEL, _("OK"),
GTK_RESPONSE_OK,
+ NULL);
+
+
gtk_dialog_set_default_response(GTK_DIALOG(plugin_data._dialog),
GTK_RESPONSE_OK);
+
+ GtkWidget *okButton =
gtk_dialog_get_widget_for_response(GTK_DIALOG(plugin_data._dialog),
GTK_RESPONSE_OK);
style nitpick: you seem to use snake_case, so
```suggestion
GtkWidget *ok_button =
gtk_dialog_get_widget_for_response(GTK_DIALOG(plugin_data._dialog),
GTK_RESPONSE_OK);
```
> +
+ GtkWidget *hbox = gtk_box_new(GTK_ORIENTATION_HORIZONTAL, 0);
+ GtkWidget *vbox = gtk_box_new(GTK_ORIENTATION_VERTICAL, 0);
+ gtk_box_pack_start(GTK_BOX(vbox), label, FALSE, FALSE, 10);
+ gtk_box_pack_start(GTK_BOX(vbox), plugin_data._entry, FALSE,
FALSE, 10);
+ gtk_box_pack_start(GTK_BOX(hbox), vbox, FALSE, FALSE, 10);
+
+ gtk_container_add(GTK_CONTAINER(content_area), hbox);
+
+ g_signal_connect(plugin_data._dialog, "response",
G_CALLBACK(configure_response), NULL);
+ }
+
+ gtk_widget_show_all(plugin_data._dialog);
+ gtk_widget_grab_focus(plugin_data._entry);
+
+ while (gtk_dialog_run(GTK_DIALOG(plugin_data._dialog)) ==
GTK_RESPONSE_ACCEPT);
this is a tad bizarre:
* you don't define anything that would return `GTK_RESPONSE_ACCEPT` AFAICT, so
this is effectively not a loop.
* you use a modal dialog with response callback, so you could merely let the
main loop run normally, and just drop this [^1]
So what about:
* drop this line altogether
* perform the `gtk_widget_hide()` in the response callback
Just a suggestion, but I think it'd be best especially because
`gtk_dialog_run()` is on its way out (not to worry, but I think it's gone GTK4
-- not that it's something we plan on anytime soon).
[^1]: but for very specific corner cases, e.g. with IPC communication, but you
probably shouldn't care.
> + g_free(filename);
+ g_free(dirname);
+
+ return status;
+}
+
+
+static void on_configure_response(GtkDialog *dialog, gint response, gpointer
user_data)
+{
+ if (response != GTK_RESPONSE_OK && response != GTK_RESPONSE_APPLY)
+ return;
+
+ plugin_data.show_in_menu =
gtk_toggle_button_get_active(GTK_TOGGLE_BUTTON(plugin_data._display_in_popup));
+
+ if (! save_config())
+ dialogs_show_msgbox(GTK_MESSAGE_ERROR, _("Plugin configuration
directory could not be saved."));
That's a bit of a weird sentence, maybe *Plugin configuration could not be
saved*?
> + keybindings_set_item(key_group, KB_INCREMENT_NUMBER, NULL, GDK_KEY_A,
> GDK_SHIFT_MASK,
+ "increment_number",
+ _("Increment Number By 1"), NULL);
+ keybindings_set_item(key_group, KB_DECREMENT_NUMBER, NULL, GDK_KEY_X,
GDK_SHIFT_MASK,
+ "decrement_number",
+ _("Decrement Number By 1"), NULL);
+ keybindings_set_item(key_group, KB_INCREMENT_DECREMENT_NUMBER_X, NULL,
GDK_KEY_M, GDK_SHIFT_MASK,
+ "increment_decrement_number_x",
+ _("Increment or Decrement Number X times"),
NULL);
I'll have to check, but doesn't this break typing capital A, X and M? That
would be a big problem…
> + gtk_container_add(GTK_CONTAINER(geany->main_widgets->editor_menu),
> plugin_data._menu_item_change_number);
+
+ configuration_apply();
+
+ g_signal_connect(plugin_data._menu_item_change_number, "activate",
G_CALLBACK(on_change_number_x), NULL);
+}
+
+
+void plugin_cleanup(void)
+{
+ if (plugin_data._dialog)
+ {
+ gtk_widget_destroy(GTK_WIDGET(plugin_data._dialog));
+ plugin_data._dialog = NULL;
+ plugin_data._entry = NULL;
+ }
don't you need to destroy `plugin_data._menu_item_sep` and
`plugin_data._menu_item_change_number`?
> + gtk_container_add(GTK_CONTAINER(geany->main_widgets->editor_menu),
> plugin_data._menu_item_sep);
+
+ plugin_data._menu_item_change_number =
gtk_menu_item_new_with_mnemonic(_("_Increment or Decrement number"));
+ gtk_container_add(GTK_CONTAINER(geany->main_widgets->editor_menu),
plugin_data._menu_item_change_number);
+
+ configuration_apply();
+
+ g_signal_connect(plugin_data._menu_item_change_number, "activate",
G_CALLBACK(on_change_number_x), NULL);
+}
+
+
+void plugin_cleanup(void)
+{
+ if (plugin_data._dialog)
+ {
+ gtk_widget_destroy(GTK_WIDGET(plugin_data._dialog));
No need to cast, it's already a `GtkWidget`
```suggestion
gtk_widget_destroy(plugin_data._dialog);
```
> + if (use_hexa)
+ {
+ guessed_number = g_ascii_strtoll(txt, NULL, 16);
+ }
+ else
+ {
+ guessed_number = atoi(txt);
+ }
any reason not to do this?:
```suggestion
guessed_number = g_ascii_strtoll(txt, NULL, use_hexa ? 16 : 10);
```
> + gint line = sci_get_line_from_position(sci, pos);
+ gint line_start = sci_get_position_from_line(sci, line);
+ gint line_end = sci_get_line_end_position(sci, line);
+ gchar ch = 0, first_ch = 0;
+ gint64 guessed_number = 0;
+ gchar *buf;
+
+ /* position of number in decimal or hexadecimal format */
+ gint decimal_start = -1, decimal_end = -1;
+ gint hexadecimal_start = -1, hexadecimal_end = -1;
+
+ gint i, prev_i;
+
+ gint hexa_state = 0;
+ gint decimal_sate = 0;
+ gint hexaCase = HEXA_CASE_UNKNOWN;
Another one that probably should use snake_case `hexa_case`
> +#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wformat-nonliteral"
+ g_snprintf(format_buf, sizeof(format_buf)-1, "%%0%d%c",
format_length, use_hexa ? (hexaCase == HEXA_CASE_UPPER ? 'X' : 'x') : 'd');
+#pragma GCC diagnostic pop
+
+ if ((buf = g_strdup_printf(format_buf, guessed_number)))
Maybe do this:
```suggestion
if (use_hexa)
{
buf = g_strdup_printf(hexaCase == HEXA_CASE_UPPER
? "%0*" G_GINT64_MODIFIER "X"
: "%0*" G_GINT64_MODIFIER "x",
format_length, (guint64)
guessed_number);
}
else
{
buf = g_strdup_printf("%0*" G_GINT64_FORMAT,
format_length, guessed_number);
}
if (buf)
```
> + if (plugin_data._dialog == NULL)
+ {
+ plugin_data._dialog = gtk_dialog_new_with_buttons(_("Increment
or Decrement number by ..."),
+
GTK_WINDOW(geany->main_widgets->window),
+ GTK_DIALOG_MODAL |
GTK_DIALOG_DESTROY_WITH_PARENT, _("Cancel"),
+ GTK_RESPONSE_CANCEL, _("OK"),
GTK_RESPONSE_OK,
+ NULL);
+
+
gtk_dialog_set_default_response(GTK_DIALOG(plugin_data._dialog),
GTK_RESPONSE_OK);
+
+ GtkWidget *okButton =
gtk_dialog_get_widget_for_response(GTK_DIALOG(plugin_data._dialog),
GTK_RESPONSE_OK);
+ gtk_widget_set_can_default(okButton, TRUE);
+
+ GtkWidget *content_area =
gtk_dialog_get_content_area(GTK_DIALOG(plugin_data._dialog));
+
+ GtkWidget *label = gtk_label_new("Enter the number of positive
or negative increments to apply.\n");
This string isn't marked as translatable. Also, no need for the `\n` at the end
> + GTK_DIALOG_MODAL |
> GTK_DIALOG_DESTROY_WITH_PARENT, _("Cancel"),
+ GTK_RESPONSE_CANCEL, _("OK"),
GTK_RESPONSE_OK,
+ NULL);
+
+
gtk_dialog_set_default_response(GTK_DIALOG(plugin_data._dialog),
GTK_RESPONSE_OK);
+
+ GtkWidget *okButton =
gtk_dialog_get_widget_for_response(GTK_DIALOG(plugin_data._dialog),
GTK_RESPONSE_OK);
+ gtk_widget_set_can_default(okButton, TRUE);
+
+ GtkWidget *content_area =
gtk_dialog_get_content_area(GTK_DIALOG(plugin_data._dialog));
+
+ GtkWidget *label = gtk_label_new("Enter the number of positive
or negative increments to apply.\n");
+
+ plugin_data._entry = gtk_spin_button_new_with_range(-64000,
64000,1);
+ gtk_spin_button_set_value(GTK_SPIN_BUTTON(plugin_data._entry),
1);
+ gtk_widget_set_tooltip_text(plugin_data._entry, _("Enter the
number of positive or negative increments to apply then press Enter key.\n"));
```suggestion
gtk_widget_set_tooltip_text(plugin_data._entry, _("Enter the
number of positive or negative increments to apply then press Enter key."));
```
> +
+ /* when the number changes sign, the format is reset to avoid a
display shift */
+ if ((positive == FALSE && guessed_number >= 0) || (positive ==
TRUE && guessed_number < 0))
+ {
+ format_length = 0;
+ }
+ else
+ {
+ format_length = digit_end - digit_start;
+ if (format_length > 12)
+ format_length = 0;
+ }
+
+ g_snprintf(format_buf, sizeof(format_buf)-1, "%%0%d%c",
format_length, use_hexa ? ( hexaCase == HEXA_CASE_UPPER ? 'X' : 'x' ) : 'd');
+
+ if ((buf = g_strdup_printf(format_buf, guessed_number)))
Aren't the pragmas around the wrong call?
> +#define HEXA_NONE -1 /* hexadecimal number not found */
+#define HEXA_MAYBE 0 /* in progress, need scan more
characters */
+#define HEXA_REQUIRE_x 1 /* need character x/X */
+#define HEXA_REQUIRE_0 2 /* need character 0 */
+#define HEXA_REQUIRE_xnumber 3 /* need at least one number */
+#define HEXA_ACCEPT_xnumber 4 /* there may be more numbers */
+#define HEXA_FOUND 5 /* ok, hexadecimal number found */
Something like this:
```suggestion
enum {
HEXA_NONE = -1, /* hexadecimal number not found */
HEXA_MAYBE, /* in progress, need scan more characters */
HEXA_REQUIRE_x, /* need character x/X */
HEXA_REQUIRE_0, /* need character 0 */
HEXA_REQUIRE_xnumber, /* need at least one number */
HEXA_ACCEPT_xnumber, /* there may be more numbers */
HEXA_FOUND /* ok, hexadecimal number found */
};
```
Just a suggestion though, current code isn't a problem
> +
+ /* when the number changes sign, the format is reset to avoid a
display shift */
+ if ((positive == FALSE && guessed_number >= 0) || (positive ==
TRUE && guessed_number < 0))
+ {
+ format_length = 0;
+ }
+ else
+ {
+ format_length = digit_end - digit_start;
+ if (format_length > 12)
+ format_length = 0;
+ }
+
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wformat-nonliteral"
+ g_snprintf(format_buf, sizeof(format_buf)-1, "%%0%d%c",
format_length, use_hexa ? (hexaCase == HEXA_CASE_UPPER ? 'X' : 'x') : 'd');
And something else: padding with 0 *seems* cute, but there are cases where `0`
prefix means octal (e.g. C is one of those). This means 2 things: if you
ignore the `0` prefix, you might not increment as expected. And it you add it
back when it wasn't there, you might not yield what you think.
A few examples:
* `"100" - 1` gives `099`, which is just not valid in C (neither is it in
Python -- but Python actually just disallows `0` prefix because of the octal
confusion and mandates `0o` for octal)
* `"0777" + 1` gives `0778`, which is again not valid C (it should probably
become `01000`)
So maybe you either want to support octal, or you don't want to deal with
anything that can yield these kind of issues.
This said, maybe the use case you have in mind is more for datasets or
something and it makes sense, but I'm wondering if there shouldn't be an option
for this.
The easy option might be not to deal with octal, and simply space-pad instead
of 0-pad.
> + keybindings_set_item(key_group, KB_INCREMENT_NUMBER, NULL, GDK_KEY_A,
> GDK_SHIFT_MASK,
+ "increment_number",
+ _("Increment Number By 1"), NULL);
+ keybindings_set_item(key_group, KB_DECREMENT_NUMBER, NULL, GDK_KEY_X,
GDK_SHIFT_MASK,
+ "decrement_number",
+ _("Decrement Number By 1"), NULL);
+ keybindings_set_item(key_group, KB_INCREMENT_DECREMENT_NUMBER_X, NULL,
GDK_KEY_M, GDK_SHIFT_MASK,
+ "increment_decrement_number_x",
+ _("Increment or Decrement Number X times"),
NULL);
Hum actually I just tested and for some reason this just doesn't work for me.
It does if I rebind it to something else.
> + gtk_container_add(GTK_CONTAINER(geany->main_widgets->editor_menu),
> plugin_data._menu_item_change_number);
+
+ configuration_apply();
+
+ g_signal_connect(plugin_data._menu_item_change_number, "activate",
G_CALLBACK(on_change_number_x), NULL);
+}
+
+
+void plugin_cleanup(void)
+{
+ if (plugin_data._dialog)
+ {
+ gtk_widget_destroy(GTK_WIDGET(plugin_data._dialog));
+ plugin_data._dialog = NULL;
+ plugin_data._entry = NULL;
+ }
```suggestion
}
gtk_widget_destroy(plugin_data._menu_item_sep);
gtk_widget_destroy(plugin_data._menu_item_change_number);
plugin_data._menu_item_sep = NULL;
plugin_data._menu_item_change_number = NULL;
```
> + if (plugin_data._dialog == NULL)
+ {
+ plugin_data._dialog = gtk_dialog_new_with_buttons(_("Increment
or Decrement number by ..."),
+
GTK_WINDOW(geany->main_widgets->window),
+ GTK_DIALOG_MODAL |
GTK_DIALOG_DESTROY_WITH_PARENT, _("Cancel"),
+ GTK_RESPONSE_CANCEL, _("OK"),
GTK_RESPONSE_OK,
+ NULL);
+
+
gtk_dialog_set_default_response(GTK_DIALOG(plugin_data._dialog),
GTK_RESPONSE_OK);
+
+ GtkWidget *okButton =
gtk_dialog_get_widget_for_response(GTK_DIALOG(plugin_data._dialog),
GTK_RESPONSE_OK);
+ gtk_widget_set_can_default(okButton, TRUE);
+
+ GtkWidget *content_area =
gtk_dialog_get_content_area(GTK_DIALOG(plugin_data._dialog));
+
+ GtkWidget *label = gtk_label_new("Enter the number of positive
or negative increments to apply.\n");
```suggestion
GtkWidget *label = gtk_label_new(_("Enter the number of
positive or negative increments to apply:"));
```
> + if (plugin_data._dialog == NULL)
+ {
+ plugin_data._dialog = gtk_dialog_new_with_buttons(_("Increment
or Decrement number by ..."),
+
GTK_WINDOW(geany->main_widgets->window),
+ GTK_DIALOG_MODAL |
GTK_DIALOG_DESTROY_WITH_PARENT, _("Cancel"),
+ GTK_RESPONSE_CANCEL, _("OK"),
GTK_RESPONSE_OK,
+ NULL);
+
+
gtk_dialog_set_default_response(GTK_DIALOG(plugin_data._dialog),
GTK_RESPONSE_OK);
+
+ GtkWidget *okButton =
gtk_dialog_get_widget_for_response(GTK_DIALOG(plugin_data._dialog),
GTK_RESPONSE_OK);
+ gtk_widget_set_can_default(okButton, TRUE);
+
+ GtkWidget *content_area =
gtk_dialog_get_content_area(GTK_DIALOG(plugin_data._dialog));
+
+ GtkWidget *label = gtk_label_new("Enter the number of positive
or negative increments to apply.\n");
Also, you should make the entry as the "mnemonic widget" for this label
somewhere below, which allows a semantic relation between the two, which in
turn allows e.g. a screen reader to properly announce the widgets:
```c
gtk_label_set_mnemonic_widget(GTK_LABEL(label),
plugin_data._entry);
```
--
Reply to this email directly or view it on GitHub:
https://github.com/geany/geany-plugins/pull/1351#pullrequestreview-2994635153
You are receiving this because you are subscribed to this thread.
Message ID: <geany/geany-plugins/pull/1351/review/2994635...@github.com>
