Why is this a GIMP bug and why is this a problem?

I am tempted to close this bug as INVALID but I guess you had a reason to open it.

Created attachment 28905 [details]
Tar file with patch and new files

This is a first try to make a replacement for the
deprecated GtkPreview widget.

Argument bpp of gimp_preview_area_draw_buf() determines also if a checker
is to be drawn or not.

contains :

/libgimpwidgets/gimppreviewarea.c
/libgimpwidgets/gimppreviewarea.h
/libgimpwidgets/gimpwidgets.h
/libgimpwidgets/gimpwidgetstypes.h


replacing GtkPreview should be in most cases as simple as :


-  preview = gtk_preview_new (GTK_PREVIEW_COLOR);
+  preview = gimp_preview_area_new ();
   gtk_preview_size (GTK_PREVIEW (preview), preview_width, preview_height);

and a lot of plugins use something simular to :

  buffer = g_new (guchar, width * height * bpp);
  gimp_pixel_rgn_get_rect(&destPR,buffer,
			  x1, y1,
			  width, height);
  /*
  * Draw the preview image on the screen...
  */
-  for (y=0 ; y < buf_y ; y++ )
-    {
-      offset = (x*bytes)+(y*preview_buf_width*bytes);
-      gtk_preview_draw_row(preview, buffer+offset, 0, row++, width);
-    }
+   gimp_preview_area_draw_buf(preview, buffer, 0,0,width,  height, bpp);
    gtk_widget_queue_draw (preview);

I will attach the diff and the (slightly altered) files here to make it easier
to review the API in Bugzilla.

Created attachment 28940 [details]
gimppreviewarea.h

Created attachment 28941 [details]
gimppreviewarea.c

Created attachment 28942 [details] [review]
gimppreviewarea.diff

We discussed this topic on GimpCon and came to the conclusion that it's a good
idea to start with a simple widget as the one you proposed here. I will post
more details about the plans for the preview widget to the mailing-list. Let me
add some comments about the GimpPreviewArea here though:

gimp_preview_area_draw_buf() shouldn't take a gint bpp parameter but a
GimpImageType. The API is also missing a function to set a colormap for indexed
images. I suggest we add

  void gimp_preview_area_set_cmap (GimpPreviewArea *area,
                                   const guchar   *cmap,
                                   gint            num_colors);

I've choosen cmap because that's what we use in the GimpImage API. Calling
gimp_preview_area_draw_buf() with GIMP_INDEXED_IMAGE or GIMP_INDEXEDA_IMAGE
should issue a g_warning() unless gimp_preview_area_set_cmap() was used beforehand.

Does that make sense?

There's one more thing: The preview area should use the user-configured
checkerboard settings. We don't need to do that immidiately but it would be good
to keep in mind that we need to add a PDB API to access these settings. In order
to access the PDB the preview area widget will have to live in libgimpui.

If gimp_preview_area_draw_buf() should take GimpImageType as parameter.
and gimp_preview_area_set_cmap this also suggests using

gdk_draw_rgb_image () 
gdk_draw_indexed_image ()
gdk_draw_gray_image ()

Shouldn't it be better to provide a second API call beeing :
gimp_preview_area_draw_image();
        
Got nothing against issue-ing a g_warning() unless gimp_preview_area_set_cmap() 
is used befor for image type GIMP_INDEXED_IMAGE or GIMP_INDEXEDA_IMAGE.

That's implementation details but you won't be able to use these GDK function if
there's an alpha channel because of the checkerboard.

I don't understand your comment about gimp_preview_area_draw_image() though. It
is not the job of the preview area to deal with GimpImage. This is certainly
left to a higher-level API that draws previews of a GimpDrawable. I only
suggested not to use an integer to specify the pixel format but to use an enum.

Mitch mentioned that we can solve the checkerboard configuration issue by
extending the vtable in libgimpwidgets. That would allow us to put
GimpPreviewArea into libgimpwidgets where it can also be used by the core.

What's the vtable in libgimpwidgets

Some code that takes care of the differences when these widgets are used from a
plug-in or from the core. Take a look at gimp_widgets_init() and how it is
called from libgimpui.

A simple GimpPreviewArea widget that can be used for plug-in previews but also
for the brush, pattern, gradient selection widgets has the advantage that we can
implement color correction for this widget. The default stack of display filters
should be applied here. Most probably we need a GimpFilterableInterface (or some
similar name).

However this, as well as configurable checkerboard settings, can be added as
soon as the basic widgets is in place.

I won't be able to pick this up until the end of August.
In case I forget just remind me in September.

Geert, I think David Odin was recently working on something in this direction.
Perhaps you could collaborate on this over mail, if he has time to finish this
off in the meantime?

In September, GIMP will already be feature frozen for 2.2. Actually, we need at
least a first version of the new widget in CVS by the end of this month or it
won't make it into the 2.2 release. Fortunately the task is relatively simple so
I am still quite optimistic about it.

Actually, I already have something in my tree. I will post it shortly in the
mailing-list for approval. I will have plenty of time in late july and the whole
month of august to polish this.

Created attachment 29871 [details]
new version of gimppreviewarea.c

This patch resync with the current comments on this bug.

Created attachment 29872 [details]
new version of gimppreviewarea.h

Created attachment 29873 [details] [review]
new diff.

This patch also includes a diff for the sharpen plug-in

quick remark 

  /* Fill checkerboard if needed */
  if (type == GIMP_RGBA_IMAGE || type == GIMP_GRAY_IMAGE /* FIXME: what about
GIMP_INDEXA_IMAGE? */)
    {
      for (rgb_row = 0 ; rgb_row <  widget_height ; rgb_row++)

should't GIMP_GRAY_IMAGE not be GIMP_GRAYA_IMAGE

You're absolutely right. Hopefully, this code will soon be into the CVS, this
kind of typo could then be fixed immediatly

This looks very good already but I think we decided that we want this code to be
in libgimpwidgets so that it can also be used from the core.

Another thing: the expose_event() handler should only redraw the exposed area,
not always the full widget. Have a look at gimp_display_shell_canvas_expose() to
see how to get the exposed rectangles from a GdkRegion.

I will look into putting this code into CVS tomorrow. We can then continue to
improve it further. The important thing right now is just to get the API right.

I've tried to put it in libgimpwidgets, but then, I have a conflict building the
main app, because of gimpenums.h (needed for GimpImageType).

The only solution I found was not to include gimppreviewarea.h into
gimpwidgets.h, which seems wrong to me.

Yes, that is indeed a problem. It could for example be solved by introducing a
GimpPreviewImageType enum. I'll find a solution tomorrow together with Mitch...

I ended up rewriting quite a bit of the patch but now it's finally in CVS and
awaits to be used by more plug-ins (and later by a full-featured GimpPreview).

2004-07-29  Sven Neumann  <sven@gimp.org>

	* libgimpwidgets/Makefile.am
	* libgimpwidgets/gimpwidgets.def
	* libgimpwidgets/gimpwidgets.h
	* libgimpwidgets/gimpwidgetstypes.h
	* libgimpwidgets/gimppreviewarea.[ch]: added GimpPreviewArea, a
	replacement for GtkPreview, loosely based on patches from Geert
	Jordaens and David Odin. Fixes bug #144759.

	* plug-ins/common/sharpen.c: use the new widget instead of a
	GtkPreview; saves about 100 lines of rather complex code :)