Hi,
"David Necas (Yeti)" <yeti@xxxxxxxxxxxxxxx> writes:
> I fixed and improved a few thing in the IFS Compose plugin.
> I know I should use bugzilla. The reasons I write here are:
> The existing bugreports are 1.2, while I fixed it in 1.3
> (except #82472 fix, which I also backported to 1.2), I'm not
> sure how to handle this. Some issues were just side effects
> of an underlying real problem (#82466, #82473). Bug #82470
> I fixed as a side effect of a larger change. The other
> things are not in bugzilla at all (like Save/Load), I don't
> have separate patches for them, and don't like the
> perspective of creating a dozen of individual reports and
> separating the patches only for the sake of bugzillization
> of the whole thing.
>
> Please critize/check/apply the attached patch (against
> 1.3.11) and tell me what to do with:
>
> nor #82472 Flip button doesn't act on all selected objects
> (I attached the backport to this one)
> min #82466 Toggling between Simple/Full in IfsCompose changes preview
> min #82473 IfsCompose disable Undo/Redo button when nothing to be undo
> enh #82470 IfsCompose filter: location of 'New' and 'Delete' button not...
>
> and what to do generally in situations like this.
I think it's fine to fix these in 1.3 only. Only severe bugs should be
fixed in 1.2. An exception can be made if the fix is trivial and thus
certainly doesn't break things.
> * set alpha of all colors to 1.0 -- ifscompose ignores alpha anyway,
> and default value (122???) makes the color selection dialog pretty
> confusing
The color selection dialog shouldn't show the alpha value. If it does,
the color buttons are set up incorrectly. Make sure they are created
with a type of GIMP_COLOR_AREA_FLAT.
I had a quick look at your patch and I have a few comments:
> + g_signal_connect(G_OBJECT(ifsD->redo_button), "clicked",
> + G_CALLBACK(redo), NULL);
> + gtk_widget_set_sensitive(ifsD->redo_button, FALSE);
> + gtk_widget_show(ifsD->redo_button);
I'd appreciate if you could try to follow the GIMP coding style which
asks for a space before the opening bracket of a function call.
> +void
> +yeti_message_dialog(GtkMessageType type, GtkWindow *parent,
> + gchar *title, gchar *message)
> +{
> + GtkWidget *dlg;
> +
> + dlg = gtk_message_dialog_new(parent, GTK_DIALOG_MODAL,
> + type, GTK_BUTTONS_OK, message);
> + if (title)
> + gtk_window_set_title(GTK_WINDOW(dlg), title);
> + gtk_window_set_wmclass(GTK_WINDOW(dlg), "message", "Gimp");
> + gtk_dialog_run(GTK_DIALOG(dlg));
> + gtk_widget_destroy(dlg);
> +}
yeti_message_dialog() is a rather unintuitive name for a function that
is supposed to create a modal dialog.
> +static void
> +ifsfile_save(GtkWidget *widget, GtkWidget *file_select)
> +{
> + gchar *str = ifsvals_stringify(&ifsvals, elements);
> + FILE *fh;
> + char *warning = NULL;
> +
> + strcpy(ifsfile_path,
> + gtk_file_selection_get_filename(GTK_FILE_SELECTION(file_select)));
> + fh = fopen(ifsfile_path, "w");
> + if (fh) {
> + fputs(str, fh);
> + fclose(fh);
> + }
> + else
> + warning = g_strdup_printf(_("Cannot save into file `%s'.\n"
> + "Check the path and permissions.\n"),
> + ifsfile_path);
> +
> + free(str);
> + if (warning) {
> + yeti_message_dialog(GTK_MESSAGE_ERROR, GTK_WINDOW(file_select),
> + "Save failed", warning);
Why do you use a modal dialog here? We usually don't use modal dialogs
in gimp. You should rather make the message dialog transient for the
file selection dialog and set destroy_with_parent on it.
> +/* load an ifs file */
> +static void
> +ifsfile_load(GtkWidget *widget, GtkWidget *file_select)
> +{
> + FILE *fh;
> + guint size = 4096;
> + guint len, pos;
> + guchar *buffer;
> + AffElement **new_elements;
> + IfsComposeVals new_ifsvals;
> + gchar *warning = NULL;
> +
> + buffer = g_new(guchar, size);
> + strcpy(ifsfile_path,
> + gtk_file_selection_get_filename(GTK_FILE_SELECTION(file_select)));
Please, don't ever use strcpy(). It's dangerous and should never be used.
> + fh = fopen(ifsfile_path, "r");
> + if (fh) {
> + pos = 0;
> + while ((len = fread(buffer + pos, 1, size - pos, fh)) == size-pos) {
> + pos = size;
> + size *= 2;
> + buffer = g_realloc(buffer, size);
> + }
> + fclose(fh);
This code looks like a reimplementation of g_file_get_contents(). You
might want to use the latter instead.
Salut, Sven
