Bug 155733 - discussion of approach (bug: need to check return values of gimp_drawable_mask_bounds())
This discussion is connected to the gimp-developer-list.gnome.org mailing list which is provided by the GIMP developers and not related to gimpusers.com.
This is a read-only list on gimpusers.com so this discussion thread is read-only, too.
Bug 155733 - discussion of approach (bug: need to check return values of gimp_drawable_mask_bounds())
Hi...
Am just getting started with gimp hacking, and decided to pick a gnome-love
bug to have a look at :)
I have a fairly clear idea of how to fix plugins which don't have any dialog box - they should just basically do nothing at all, and exit silently. (I'd have thought they ought to take the first opportunity to exit; I'm not sure this is true of the already-fixed ones but that's not the current issue.)
For plugins with dialog boxes, things are less obvious. I chose "grid" to
look at first. I could change it in the fashion suggested in the bugzilla
discussion, so that instead of calling gimp_drawable_mask_bounds() it calls
gimp_drawable_mask_intersect() and checks the return value. However, this
would still run the dialog, with a (totally spurious) preview. To me, it
would make far more sense to bail out before the dialog is even called.
However, when I had a look at blur_gauss.c, which is already fixed (i.e. it
calls gimp_drawable_mask_intersect()), I saw that it does allow the plugin
dialog to run.
In contrast, iwarp bails out immediately, with a warning that the affected
region is empty (in a status bar below the image window).
I was tempted to infer that blur_gauss was fixed in a really minimalist way (just to avoid nasty stuff happening, e.g. crashes) while leaving the code in a slightly icky state (i.e. the dialog box running even though it can't do anything at all to the image), while iwarp was fixed in a "better" way. However, when I look at the git commit attributed to Luidnel Maignan (9b6c9e1fe4f46d2d47c6d97d4147cf060abd07f8) I can see that *both* approaches were used (iwarp was fixed in this patch, and a bunch of others too). So I'm confused!
Some guidance on which approach is best (and why) would be welcome...
ta Neil
Bug 155733 - discussion of approach (bug: need to check return values of gimp_drawable_mask_bounds())
On 01/03/12 01:09, Neil wrote:
Hi...
Am just getting started with gimp hacking, and decided to pick a gnome-love bug to have a look at :)I have a fairly clear idea of how to fix plugins which don't have any dialog box - they should just basically do nothing at all, and exit silently. (I'd have thought they ought to take the first opportunity to exit; I'm not sure this is true of the already-fixed ones but that's not the current issue.)
For plugins with dialog boxes, things are less obvious. I chose "grid" to look at first. I could change it in the fashion suggested in the bugzilla discussion, so that instead of calling gimp_drawable_mask_bounds() it calls gimp_drawable_mask_intersect() and checks the return value. However, this would still run the dialog, with a (totally spurious) preview. To me, it would make far more sense to bail out before the dialog is even called. However, when I had a look at blur_gauss.c, which is already fixed (i.e. it calls gimp_drawable_mask_intersect()), I saw that it does allow the plugin dialog to run.
In contrast, iwarp bails out immediately, with a warning that the affected region is empty (in a status bar below the image window).I was tempted to infer that blur_gauss was fixed in a really minimalist way (just to avoid nasty stuff happening, e.g. crashes) while leaving the code in a slightly icky state (i.e. the dialog box running even though it can't do anything at all to the image), while iwarp was fixed in a "better" way. However, when I look at the git commit attributed to Luidnel Maignan (9b6c9e1fe4f46d2d47c6d97d4147cf060abd07f8) I can see that *both* approaches were used (iwarp was fixed in this patch, and a bunch of others too). So I'm confused!
Some guidance on which approach is best (and why) would be welcome...
ta Neil
_______________________________________________ gimp-developer-list mailing list
gimp-developer-list@gnome.org
http://mail.gnome.org/mailman/listinfo/gimp-developer-list
I seem to recall posting to this list , a couple of weeks back that Enhance | Sharpen was effectively not doing anything , maybe this one has been "fixed" as well ??
/gg
Bug 155733 - discussion of approach (bug: need to check return values of gimp_drawable_mask_bounds())
On 01/03/12 01:09, Neil wrote:
Hi...
Am just getting started with gimp hacking, and decided to pick a gnome-love bug to have a look at :)I have a fairly clear idea of how to fix plugins which don't have any dialog box - they should just basically do nothing at all, and exit silently. (I'd have thought they ought to take the first opportunity to exit; I'm not sure this is true of the already-fixed ones but that's not the current issue.)
For plugins with dialog boxes, things are less obvious. I chose "grid" to look at first. I could change it in the fashion suggested in the bugzilla discussion, so that instead of calling gimp_drawable_mask_bounds() it calls gimp_drawable_mask_intersect() and checks the return value. However, this would still run the dialog, with a (totally spurious) preview. To me, it would make far more sense to bail out before the dialog is even called. However, when I had a look at blur_gauss.c, which is already fixed (i.e. it calls gimp_drawable_mask_intersect()), I saw that it does allow the plugin dialog to run.
In contrast, iwarp bails out immediately, with a warning that the affected region is empty (in a status bar below the image window).I was tempted to infer that blur_gauss was fixed in a really minimalist way (just to avoid nasty stuff happening, e.g. crashes) while leaving the code in a slightly icky state (i.e. the dialog box running even though it can't do anything at all to the image), while iwarp was fixed in a "better" way. However, when I look at the git commit attributed to Luidnel Maignan (9b6c9e1fe4f46d2d47c6d97d4147cf060abd07f8) I can see that *both* approaches were used (iwarp was fixed in this patch, and a bunch of others too). So I'm confused!
Some guidance on which approach is best (and why) would be welcome...
ta Neil
_____
Correction to my last comment, I just checked and sharpen does seem to work but blur has no dialog , it just does a blur with an uncontrolled radius.
The ensuing "Reshow Blur" had nothing to reshow and just adds another uncontrolled blur.
:?
Bug 155733 - discussion of approach (bug: need to check return values of gimp_drawable_mask_bounds())
On Tue, Jan 3, 2012 at 12:01 PM, wrote:
Correction to my last comment, I just checked and sharpen does seem to work but blur has no dialog , it just does a blur with an uncontrolled radius.
The ensuing "Reshow Blur" had nothing to reshow and just adds another uncontrolled blur.
:?
Yeah, this is entirely normal and as intended (I believe). The clue is in the lack of "..." after the name of the filter in the menu. E.g., the gaussian blur does have a dialog in which you can set options, and so its name is "Gaussian blur...", as against just plain "Blur". Note though that my email (and this bug) is specifically about the special case where the active layer is smaller than the image and the selection doesn't overlap with it at all.
Neil
Bug 155733 - discussion of approach (bug: need to check return values of gimp_drawable_mask_bounds())
On 01/03/12 14:43, Neil wrote:
On Tue, Jan 3, 2012 at 12:01 PM, > wrote:
Correction to my last comment, I just checked and sharpen does seem to work but blur has no dialog , it just does a blur with an uncontrolled radius.
The ensuing "Reshow Blur" had nothing to reshow and just adds another uncontrolled blur.
:?
Yeah, this is entirely normal and as intended (I believe). The clue is in the lack of "..." after the name of the filter in the menu. E.g., the gaussian blur does have a dialog in which you can set options, and so its name is "Gaussian blur...", as against just plain "Blur". Note though that my email (and this bug) is specifically about the special case where the active layer is smaller than the image and the selection doesn't overlap with it at all.
Neil
My original comment about sharpen not working seems to have been a local problem, that is now fixed. I thought this may be relevant to your issue.
If blur does not have a dialog it must be a one size fits all and may as well be removed! It clearly is not properly integrated into GIMP if it gets listed as "reshow" when it was never shown.
My earlier bug report here on sharpen showed it got integrated into menu as both "reshow" and "redo" even when I cancelled without doing a change.
These are more like UI defects but I thought it may have something to do with dialogue failures not exiting cleanly or at the right time. I could not imagine someone adding a blur that was not adjustable, that's just dumb.
Sorry for the noise.
gg
Bug 155733 - discussion of approach (bug: need to check return values of gimp_drawable_mask_bounds())
On Tue, Jan 3, 2012 at 2:09 PM, wrote:
My original comment about sharpen not working seems to have been a local problem, that is now fixed. I thought this may be relevant to your issue.
Yeah, it could have been. I tried to reproduce it before I read your second message :)
If blur does not have a dialog it must be a one size fits all and may as well be removed! It clearly is not properly integrated into GIMP if it gets listed as "reshow" when it was never shown.
My earlier bug report here on sharpen showed it got integrated into menu as both "reshow" and "redo" even when I cancelled without doing a change.
These are more like UI defects but I thought it may have something to do with dialogue failures not exiting cleanly or at the right time. I could not imagine someone adding a blur that was not adjustable, that's just dumb.
I tend to agree that the reshow/redo behaviour feels like a UI bug/misfeature but I'm a novice here - others may feel differently (?). And I was also caught by surprise recently by the blur behaviour - I had thought it had a dialog box but perhaps I have always used one of the other blurs (which does)... Perhaps it's there as a "simple" tool for beginners?
Neil
Bug 155733 - discussion of approach (bug: need to check return values of gimp_drawable_mask_bounds())
On 01/03/2012 06:49 AM, Neil wrote:
On Tue, Jan 3, 2012 at 2:09 PM, > wrote:
My original comment about sharpen not working seems to have been a local problem, that is now fixed. I thought this may be relevant to your issue.
Yeah, it could have been. I tried to reproduce it before I read your second message :)
If blur does not have a dialog it must be a one size fits all and may as well be removed! It clearly is not properly integrated into GIMP if it gets listed as "reshow" when it was never shown.
My earlier bug report here on sharpen showed it got integrated into menu as both "reshow" and "redo" even when I cancelled without doing a change.
These are more like UI defects but I thought it may have something to do with dialogue failures not exiting cleanly or at the right time. I could not imagine someone adding a blur that was not adjustable, that's just dumb.
I tend to agree that the reshow/redo behaviour feels like a UI bug/misfeature but I'm a novice here - others may feel differently (?). And I was also caught by surprise recently by the blur behaviour - I had thought it had a dialog box but perhaps I have always used one of the other blurs (which does)... Perhaps it's there as a "simple" tool for beginners?
With my version (2.6.11) "Blur" does an uncontrolled blur of about 4 pixels, then in the edit menu provides a "Fade blur" option ranging from 0-100%. This actually works pretty well for a couple of things I did to clean up some garbage pixels.
Neil
_______________________________________________ gimp-developer-list mailing list
gimp-developer-list@gnome.org
http://mail.gnome.org/mailman/listinfo/gimp-developer-list
Bug 155733 - discussion of approach (bug: need to check return values of gimp_drawable_mask_bounds())
Have looked some more at the various patches in the bugzilla list.
After extra scrutiny of Luidnel's patch (9b6c9e1), I now realise that this is the very patch where Michael Natterer comments at the top that he has altered it so as to not "spit messages", but the patch then makes iwarp.c do precisely that. I am now suspecting that iwarp.c got missed while the patch was being edited... :-) Correct?
In comment 26 (https://bugzilla.gnome.org/show_bug.cgi?id=155733#c26), Michael says that gimpressionist *should* warn, as it's interactive-only. But what does interactive-only mean? I initially took it to mean that if you tried to "repeat" it would bring back up the dialog box. Gimpressionist doesn't do that though - it simply reapplies the same effect to a new selection each time you "repeat". In contrast, iwarp would fit my concept of what "interactive-only" means, as if you try to "repeat", it *does* bring up the dialog box, so it seems that it really can't ever function without a dialog box.
I've also noticed that a whole bunch of plugins do issue a g_message() before they bail out (including I think all of the ones patched by Bill Skaggs).
So... I could use some guidance from core devs here (i.e. the people who'll be accepting/rejecting any patches). My own preference would certainly be for all plugins to issue a warning (in the form of a g_message() call or even a dialog) if they are about to do nothing because _mask_intersect() returns false. But if there is some argument against this we should patch the rest to stop doing it for consistency.
Bug 155733 - discussion of approach (bug: need to check return values of gimp_drawable_mask_bounds())
Bump......? Any core devs able to comment pls?
On Mon, Jan 9, 2012 at 11:30 PM, Neil wrote:
Have looked some more at the various patches in the bugzilla list.
After extra scrutiny of Luidnel's patch (9b6c9e1), I now realise that this is the very patch where Michael Natterer comments at the top that he has altered it so as to not "spit messages", but the patch then makes iwarp.c do precisely that. I am now suspecting that iwarp.c got missed while the patch was being edited... :-) Correct?
In comment 26 (https://bugzilla.gnome.org/show_bug.cgi?id=155733#c26), Michael says that gimpressionist *should* warn, as it's interactive-only. But what does interactive-only mean? I initially took it to mean that if you tried to "repeat" it would bring back up the dialog box. Gimpressionist doesn't do that though - it simply reapplies the same effect to a new selection each time you "repeat". In contrast, iwarp would fit my concept of what "interactive-only" means, as if you try to "repeat", it *does* bring up the dialog box, so it seems that it really can't ever function without a dialog box.
I've also noticed that a whole bunch of plugins do issue a g_message() before they bail out (including I think all of the ones patched by Bill Skaggs).
So... I could use some guidance from core devs here (i.e. the people who'll be accepting/rejecting any patches). My own preference would certainly be for all plugins to issue a warning (in the form of a g_message() call or even a dialog) if they are about to do nothing because _mask_intersect() returns false. But if there is some argument against this we should patch the rest to stop doing it for consistency.
