GNOME Bugzilla – Bug 122049
gimpfu.py should validate input
Last modified: 2004-03-02 18:28:57 UTC
- Start Xtns -> Python-Fu -> Misc -> Sphere - Enter into radius something like 100,2 -> Nothing happend
What's the state of this report? Can it be reproduced with 1.3.23?
I can reproduce this with current CVS HEAD. If I enter '100,2' as the reporter suggests, I get this in my console: Traceback (most recent call last):
+ Trace 42067
extra_params = _interact(func_name)
ret = map(lambda wid: wid.get_value(), edit_wids)
return string.atoi(self.get_text())
return _int(s, base)
I thought this might be locale dependent, so I tried entering '100.2' instead: Traceback (most recent call last): File "/usr/local/lib/gimp/1.3/python/gimpfu.py", line 427, in _run extra_params = _interact(func_name) File "/usr/local/lib/gimp/1.3/python/gimpfu.py", line 403, in _interact ret = map(lambda wid: wid.get_value(), edit_wids) File "/usr/local/lib/gimp/1.3/python/gimpfu.py", line 403, in <lambda> ret = map(lambda wid: wid.get_value(), edit_wids) File "/usr/local/lib/gimp/1.3/python/gimpfu.py", line 254, in get_value return string.atoi(self.get_text()) File "/usr/lib/python2.2/string.py", line 225, in atoi return _int(s, base) ValueError: invalid literal for int(): 100.2 Looks like the script expects an integer.
This patch fix the bug: ? gimp-1.3.18 Index: plug-ins/pygimp/plug-ins/sphere.py =================================================================== RCS file: /cvs/gnome/gimp/plug-ins/pygimp/plug-ins/sphere.py,v retrieving revision 1.10 diff -u -p -r1.10 sphere.py --- plug-ins/pygimp/plug-ins/sphere.py 6 Jan 2004 16:16:30 -0000 1.10 +++ plug-ins/pygimp/plug-ins/sphere.py 17 Jan 2004 20:48:45 -0000 @@ -21,6 +21,8 @@ import math from gimpfu import * def python_sphere(radius, light, shadow, bg_colour, sphere_colour): + if radius<=1 : + radius=1 width = int(radius * 3.75) height = int(radius * 2.5) img = gimp.Image(width, height, RGB) @@ -76,7 +78,7 @@ register( "<Toolbox>/Xtns/Python-Fu/Misc/_Sphere", "RGB*, GRAY*, INDEXED*", [ - (PF_INT, "radius", "Radius for sphere", 100), + (PF_FLOAT, "radius", "Radius for sphere", 100), (PF_SLIDER, "light", "light angle", 45, (0,360,1)), (PF_TOGGLE, "shadow", "shadow?", 1), (PF_COLOR, "bg_colour", "background", (255,255,255)),
A patch the fixes the root problem would be much better than something that papers it over for a specific script.
What is the root problem? To me this looks like a buggy script and the patch seems to fix it. But then I don't know much about pygimp. Yosh, could you please appreciate the contribution instead of wiping it away without much explanation?
The problem is the gimpfu wrapper doesn't gracefully handle bad input from the user. The script isn't buggy, it's asking for an integer and it expects one (the stack trace doesn't involve the script code at all). The gimpfu code should validate the input, perhaps even putting up an error dialog saying what's wrong.
It would be nice if the bug summary could describe the actual problem then.
What needs to be done (technically) to validate input in Python? Does it have strtol and strtod equivalents? Will this get done soon? If not, it should be on the 2.2 milestone. Cheers, Dave.
>What needs to be done (technically) to validate input in Python? Does >it have strtol and strtod equivalents? Yes. int (string) and float (string) will do the trick Also, enclosing these conversions in a try: except: clause will catch invalid values. IMO, it is simple enough to go in for 2.0
I had a look at this and have started it - just waiting for a compile to finish :} Dave.
Created attachment 24589 [details] [review] Partial patch which catches a ValueException and uses the default value.
Created attachment 24592 [details] [review] Improved patch, still needs a bit of work.
Adding PATCH keyword. Also, there's no reason not to apply Florent's original patch - while it doesn't add error handling, there's no real reason for the sphere script to limit itself to an int. Dave.
Created attachment 24593 [details] [review] Next patch - fixes an obvious error where we can set the defaults to invalid values
Committed a reworked patch: 2004-02-20 Manish Singh <yosh@gimp.org> * plug-ins/common/pygimp/gimpfu.py: Do some simple validation on the GUI input. Arrays are still kinda hokey though. Based on Dave Neary's patch, addresses bug #122049. * plug-ins/common/pygimp/plug-ins/sphere.py: Make sure radius is at least 1. Thanks to Florian Traverse for noticing. (Also in #122049)
So as I said above, arrays still don't have any real validation. There should be a some sort of consistent format, but I'm not sure what it should be. Perhaps removing support and revisiting post-2.0 is the right thing. I left sphere.py taking an int since it really is the python equivalent of test-sphere.scm. If someone wants to expand it to cover all the PF values, be my guest.
Out of interest, what input checking is needed on arrays? While we're at it, we may as well try and close out this bug. To be clear though - the original bug report is fixed, twice, so I'd have no objections to this bug being resolved as FIXED. Dave.
Array input is handled like this: class ArrayEntry(StringEntry): def get_value(self): return eval(self.get_text(), {}, {}) And in edit_mapping: PF_INT8ARRAY : ArrayEntry, PF_INT16ARRAY : ArrayEntry, PF_INT32ARRAY : ArrayEntry, PF_FLOATARRAY : ArrayEntry, PF_STRINGARRAY : ArrayEntry, There are several problems, since it's just a bare eval: a) No checking is done to make sure a valid sequence is produced by the arbitrary code eval. Hell, even SyntaxError could be thrown. b) Even if a valid sequence is given, no checking is done to verify all members of the sequence are of the correct type. Really, it shouldn't be a bare eval at all. There should be some policy that says what format an array should be entered in that's consistent throughout the app and all bindings. I haven't really thought about what's really good for that, hence suggesting punting to post-2.0. Neither script-fu nor perl allow arrays explictly, so there is no precedent. I doubt any python script uses it, so removing support for it now means we don't have to worry about backwards compat issues when trying to fix it the right way.
Hi, > Neither script-fu nor perl allow arrays explictly, so there is no > precedent. I doubt any python script uses it, so removing support for > it now means we don't have to worry about backwards compat issues when > trying to fix it the right way. Given that this is the case, I agree we should drop arrays for 2.0, and perhaps create an enhancement to add them back in later? Will you do it, or shall I? The patch is pretty trivial, I think. Cheers, Dave.
I had a go at this, and I think I deleted too much. Basically I removed PF_*_ARRAY from gimpfu.py, and also from the PDB wrapper. That gave me lots of complaints about unused enum values (which is normal), but no obvious errors. Is this the right thing to do, or should I just stop in gimpfu.py? Dave.
2004-03-02 Manish Singh <yosh@gimp.org> * plug-ins/pygimp/gimpfu.py: Disable PF_*ARRAY for now. Addresses #122049.
This is purely interface related. There's no issue with array inside code, so the pdb stuff shouldn't be touched, since it's fine.