[Gimp-developer] IFSCompose and bugzilla question

2003-02-01 Thread David Necas (Yeti)

Hello,

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.

IFS Compose changes:
* implemented Save and Load
* made Flip to act on all selected transformations, fixes #82472
* moved New/Delete from action area below other buttons, fixes #82470
* fixed Delete to be insensitive when num of transforms == 2
* Undo/Redo made insensitive when there's nothing to update, and avoided
  creation of bogus undo levels during color updates, fixes #82466, #82473,
  and other queer undo/redo behaviour
* set alpha of all colors to 1.0 -- ifscompose ignores alpha anyway, and
  default value (122???) makes the color selection dialog pretty confusing
* made Undo, Redo, Select All, Recompute Center menu items visible as buttons,
  so people who don't try right clicking in the design area know about them
* changed confusing numeric frame title to Transformation n
* changed the button layout to two centered groups (it looked approximately
  this way before too, but probably only incidentally)

Thanks in advance,

Yeti


--- gimp-1.3.11.orig/plug-ins/ifscompose/ifscompose.c   2002-12-03 20:42:41.0 
+0100
+++ gimp-1.3.11/plug-ins/ifscompose/ifscompose.c2003-02-01 23:25:34.0 
++0100
@@ -169,6 +169,12 @@
   GtkWidget *stretch_button;
   gint   stretch_handler;
 
+  GtkWidget *undo_button;
+  GtkWidget *undo_menu_item;
+  GtkWidget *redo_button;
+  GtkWidget *redo_menu_item;
+  GtkWidget *delete_button;
+
   GtkWidget *preview;
   guchar*preview_data;
   gint   preview_iterations;
@@ -278,6 +284,10 @@
   gpointer   data);
 static void ifs_compose_delete_callback   (GtkWidget *widget,
   gpointer   data);
+static void ifs_compose_load_callback (GtkWidget *widget,
+  gpointer   data);
+static void ifs_compose_save_callback (GtkWidget *widget,
+  gpointer   data);
 
 static void ifs_compose_ok_callback   (GtkWidget *widget,
   GtkWidget *window);
@@ -300,6 +310,8 @@
 static gint undo_num = 0;
 static gint undo_start = 0;
 
+static gchar ifsfile_path[PATH_MAX] = { '\0' };
+
 /* num_elements = 0, signals not inited */
 static IfsComposeVals ifsvals =
 {
@@ -387,8 +399,6 @@
 
   INIT_I18N_UI(); 
 
-  /* kill (getpid(), 19); */
-
   /*  Get the active drawable  */
   active_drawable = gimp_drawable_get (param[2].data.d_drawable);
 
@@ -786,10 +796,10 @@
 GIMP_STOCK_RESET, ifs_compose_defaults_callback,
 NULL, NULL, NULL, FALSE, FALSE,
 
-GTK_STOCK_DELETE, ifs_compose_delete_callback,
+GTK_STOCK_OPEN, ifs_compose_load_callback,
 NULL, NULL, NULL, FALSE, FALSE,
 
-GTK_STOCK_NEW, ifs_compose_new_callback,
+GTK_STOCK_SAVE, ifs_compose_save_callback,
 NULL, NULL, NULL, FALSE, FALSE,
 
 GTK_STOCK_CANCEL, gtk_widget_destroy,
@@ -850,10 +860,10 @@
 
   /* Iterations and preview options */
 
-  hbox = gtk_hbox_new (FALSE, 4);
+  hbox = gtk_hbox_new (TRUE, 4);
   gtk_box_pack_start (GTK_BOX (main_vbox), hbox, FALSE, FALSE, 0);
 
-  alignment = gtk_alignment_new (1.0, 0.5, 0.5, 0.0);
+  alignment = gtk_alignment_new (0.5, 0.5, 0.0, 0.0);
   gtk_box_pack_start (GTK_BOX (hbox), alignment, TRUE, TRUE, 0);
 
   util_hbox = gtk_hbox_new (FALSE, 4);
@@ -891,7 +901,7 @@
   gtk_widget_show (alignment);
   gtk_widget_show (util_hbox);
 
-  alignment = gtk_alignment_new (1.0, 0.5, 0.5, 0.0);
+  alignment = gtk_alignment_new (0.5, 0.5, 0.0, 0.0);
   gtk_box_pack_start (GTK_BOX (hbox), alignment, TRUE, TRUE, 0);
 
   util_hbox = gtk_hbox_new (FALSE, 4);
@@ -924,6 +934,70 @@
   gtk_widget_show (alignment);
   gtk_widget_show (hbox);
 
+  /* second util row 

Re: [Gimp-developer] IFSCompose and bugzilla question

2003-02-01 Thread Sven Neumann
Hi,

David Necas (Yeti) [EMAIL PROTECTED] 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
___
Gimp-developer mailing list
[EMAIL PROTECTED]
http://lists.xcf.berkeley.edu/mailman/listinfo/gimp-developer



Re: [Gimp-developer] IFSCompose and bugzilla question

2003-02-01 Thread Sven Neumann
Hi,

myself wrote:

 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.

actually the bug was in GimpColorButton itself. It used to enable
opacity control in the GTK+ color selection dialog unconditonally.
This is now fixed in CVS.


Salut, Sven
___
Gimp-developer mailing list
[EMAIL PROTECTED]
http://lists.xcf.berkeley.edu/mailman/listinfo/gimp-developer