Created attachment 27028 [details] [review]
Patch adds a preview window to the unsharp plug-in.

Geert:  I tried your patch, and I think it is a great improvement to the
plug-in.    There is a change you could make that I think would make it even
better, though.  This is unfortunately a rather slow plug-in, particularly if
the radius is large, so I think it would be better to have an "update" button
for the user to press in order to update the preview, rather than having it
update continuously.  You could also, if you like, have a check box for "update
continuously" that would produce the current behavior if the user wants it.

There are two issues I see with the coding, one minor and one major.  The minor
one is that you have introduced a bunch of new global variables, which is not
good coding practice.  The major one is that you have used the GtkPreview
widget, which is deprecated and ought not to be used in new code.  It might not
be worth changing this, though, since Gimp is likely to have its own preview
widget in the not-too-distant future.

I need to add that I am not a member of the Gimp development team, just somebody
who is interested, so you should not take my suggestions as having any authority
or predicting what others will say.

issue 1: Agree with the global variables, this was a quick fix.
First time I ever looked at gimp code and over 5 that I've looked at any C code.

issue 2: Is there a reference implementation for the new preview?
If there exists some doc or reference impelementation I'would like to try and
modify the plug-in.

>...There is a change you could make ...it would be better to have an "update"
>button ... rather than having it update continuously.

I'd prefer to set the policy of the radius/amount/treshold to update delayed.
However it seems that it's not that easy as for the preview scrollbars.
Any sugesstions how to set the update policy on objects created via
gimp_scale_entry_new

There are macros (listed in the API docs) that let you access the components of
the scale entry:  I think GIMP_SCALE_ENTRY_SCALE (adj) is the one you want, but
it might be  GIMP_SCALE_ENTRY_SCALE_ADJ (adj). 

You can find the discussion of preview widgets in the Gimp Developer's List
archive; a good place to begin might be:
http://www.mail-archive.com/gimp-developer@lists.xcf.berkeley.edu/msg06115.html

See also bug #52374.

*** Bug 141810 has been marked as a duplicate of this bug. ***

BTW, it's perfectly fine to continue to use GtkPreview. At least until we can
provide a reasonable replacement. There was actually no good reason for
deprecating GtkPreview and IMHO it's OK to continue using it.

... I've been struggling with the GdkDrawing area to replace the GtkPreview.
Perhaps it's enough to make change the update policy of the 
radius/threshold/amount sliders to UPDATE_DELAYED. 

At the moment the patch only unsharpens the displayed part of the buffer.
Perhaps rendering the whole buffer and scrolling trough it would be better 
(less speedy). 

Having the whole image in the unsharpen preview area (as suggested in bug 
report 141810) is only a good idea if the image can be displayed unscaled.
It makes no sense to apply a unsharp mask before scaling the image to it's 
final size as scaling modifies the effect. IOW a unsharp mask should only be 
applied after the final size of a photo/image is determined. 

It would be nice to have an updated patch attached to this report. Don't worry
if it uses GtkPreview.

Created attachment 27408 [details] [review]
Updated patch for unsharp plugin

Changes to previous version :

- Whole selection is redered (slow) to buffer.
- Preview area allows scrolling through whole buffer window without rendering.

Created attachment 27409 [details] [review]
Patch is first attempt to add preview for blur plugin

First attempt, some what annoying with a large repeat count.
Maybe a button that triggers the preview update instead of the value chaged
signals could solve this.

any other suggestions.

New patch available thet renders the preview on a buffer.
The buffer depends on the radius size.

Created attachment 27548 [details] [review]
patch from preview windoww

A couple of comments:

- What does gimp_get_data ("plug_in_unsharp_mask", &unsharp_params) do in the
  preview_params() function? I guess you want to pass the parameter struct to
  the preview function instead.
- You don't need the callback functions you introduce at the bottom. Simply
  connect to the standard callbacks as provided by libgimpwidgets and do
  additionally connect all controls that invalidate the preview to
  preview_update().
- Please try to adhere to the GIMP coding style. That will reduce the amount
  of work that I will have to put into this when applying the patch.
- You cannot declare variables in the middle of a scope. That's a C99 feature
  and we don't depend on that (nor do we like this style).

Created attachment 27560 [details] [review]
updated patch

modified as suggested in comment :

- no call to gimp_get_data for unsharp_params
- removed callback functions and repplaced with connact_after.
- Gimp coding style : changed (I think I've got that part right)
- Declaration C99 style : changed 

please have some patience I'm learning.

Thanks for the update. I will look into applying this later.

Created attachment 27565 [details] [review]
Preview window for blur plugin (same principle as for unsharp plugin)

Did the same for blur as for unsharp

Please open a separate bug report for the blur plug-in.

Comment on attachment 27565 [details] [review]
Preview window for blur plugin (same principle as for unsharp plugin)

Created new bug report : 142318

I would have applied this patch by now but Federico picked it up and said he
wanted to improve it further.

Geert, I'd appreciate if you could have a look at the modifications I did and
could apply them to your patch for the blur plug-in.

Applied to the HEAD branch:

2004-06-05  Sven Neumann  <sven@gimp.org>

	* plug-ins/common/unsharp.c: applied a modified patch from Geert
	Jordaens that adds a preview to the Unsharp Mask plug-in. Fixes
	bug #140974.