GNOME Bugzilla – Bug 472316
Alpha as logo: frosty error message
Last modified: 2008-10-30 19:57:58 UTC
Please describe the problem: The filter frosty (translated from German Frostig) starts and a progress bar is visible showing "glitzern" (glitter?); Then at the end of the procedure a GIMP message appears, consisting of two messages: 1. GIMP message: Keine Auswahl zum Nachzeichen vorhanden (No selection for drawing found ?) 2. Frosty message: procedural database execution of gimp-edit-stroke failed. (this seems similar to the bug 327681 of vers. 2.2.10 cool metal problem) Steps to reproduce: 1. open GIMP 2. new file 3. write Text (e.g. Hello, 100 px) 4. apply filter alpha as logo "frosty"; leave the default 5. I changed to color "red" and effect size 50 px and changed font 6. message is reproducable Actual results: The filter frosty (translated from German Frostig) starts and a progress bar is visible showing "glitzern" (glitter?); Then at the end of the procedure a GIMP message appears, consisting of two messages: 1. GIMP message: Keine Auswahl zum Nachzeichen vorhanden (No selection for drawing found ?) 2. Frosty message: procedural database execution of gimp-edit-stroke failed Expected results: I would expect to have the text changes with the alph as logo effect Does this happen every time? yes, the problem is stable and seems not to depend on values and data or font Other information: this seems similar to the bug 327681 of vers. 2.2.10 cool metal problem
The problem here is that there are two "frosty" scripts, one which runs from the Toolbox menu and creates a new text layer with specified text, the other in the image menu, operating on an existing text layer. The one in the Toolbox menu seems to work okay; the one in the image menu is badly broken. I think it was probably broken by the fix to bug #132145, which fixed problems with the Toolbox version but apparently wasn't tested with the other version. The difference is that the Toolbox version places the layer at a specific spot in the image, and gives it a specific border size -- if you artificially do that to a text layer before applying the image version, it works (sort of). In my opinion this can probably be fixed hackishly, but a proper fix needs a text layer API, so that the script can actually decode the text that is in the layer and manipulate it accordingly.
I succeeded wrtitng a bug fix version (no more bugs!) I'll attach the file in a moment - It works fine for me. Instead of rewriting it I added several commands to fix the original version (workaround).
Created attachment 105766 [details] [review] BugFix version for Frosty script-fu The attached file is a bug fix version of Script-Fu "Frosty" - the bug fix applies on the "alpha to logo" section and it is now running OK with GIMP 2.4
Created attachment 105778 [details] [review] diff created from the attached file Usually you would not attach the changed file but a diff against the original version. I have now created such a diff and attached it here for review.
LightningIsMyName, thank you for the modified script. There is one noticeable problem with it as provided. You declared four variables in a let* block but did not provide a value for those variables. Script-Fu will let you get away with that for now but, at some point in the future, scripts that declare variables in a let block without providing a value will break.
(In reply to comment #5) > LightningIsMyName, thank you for the modified script. There is one noticeable > problem with it as provided. You declared four variables in a let* block but > did not provide a value for those variables. Script-Fu will let you get away > with that for now but, at some point in the future, scripts that declare > variables in a let block without providing a value will break. If this is the case then all I need to do is declare all variables as set to 0 in the let area - I'll do it tomorrow. also, how do I create a diff file? Thanks
To avoid having to write out an answer that tries to cover all bases, let me ask first what operating system you are using, and whether you have any sort of development environment set up.
Created attachment 105857 [details] [review] Diff with variables valued to 0 (reply to 5) In reply to Kevin Cozens' comment (#5) I now gave values for all the variables that were left without a value in the let* block as 0. (I only modified the diff file that was made by Sven Neumann (#4))
Kevin, if the patch looks good otherwise, can you please apply it and fix the minor issue with the variables initialization before you commit?
Oh and please fix up the coding style, removed the commented out line and the additional header line. Thanks.
OS: Windows XP sp 2
Created attachment 105895 [details] [review] patch after coding style cleanups To get this rolling (so that we can get 2.4.5 released), I have created a patch with the changes that I asked for. Now we just need to verify that the changes are the right thing to do.
2008-02-26 Kevin Cozens <kcozens@cvs.gnome.org> * plug-ins/script-fu/scripts/frosty-logo.scm: Commited slightly modified patch from LightningIsMyName. Fixes bug #472316. The appearance of the sparkle layer could be better. There is still a minor problem with the sparkle layer. It appears to be offset in regards to the image resulting in the sparkle effect being cut off along the left and top sides leaving a white border tending to spoil the overall effect.
I see what you mean yet I doubt i can fix it since there is a selection and it's bordered by the image. a process of uncroping and then recropping might be possible - I'll check out to see if I can do anything yet I think this as an issue that can not be resolved.
Created attachment 106021 [details] [review] Important effect size fix Important! Appernatly extremely large effect sizes could have made this script give unwanted error messages I added it in a new version to include an option for the script to check this before it's actual begining and give a normal message in case the effect size is to big for the layer.
Comment on attachment 106021 [details] [review] Important effect size fix Important! Appernatly extremely large effect sizes could have made this script give unwanted error messages I added it in a new version to include an option for the script to check this before it's actual begining and give a normal message in case the effect size is to big for the layer. A diff file is now needed (I don't know how to create one...)
This check can and should be done nicer. The error message also needs to be marked for translation. Since this introduces a new string, it can not be done in the stable branch. Moving this to the 2.6 milestone.
LightningIsMyName, are you still there? Let's introduce the new string ASAP. I recomend you to create a diff using GNU diff: diff -u -p file.original file.new > output-file.patch
Created attachment 112057 [details] [review] Diff file for patch including the string Hello, I'm still here I made a diff using python's ndiff function (since I don't have Gnu Diff) It's possible to apply the patch using python restore() function from the difflib library. (The original version is also attached above in Comment #15 so no need to do any applying of the patch)
Please make a diff in the unified format (e.g. with GNU diff). You can download GNU diff for windows here: http://gnuwin32.sourceforge.net/packages/diffutils.htm (take "Complete package, except sources)
Created attachment 112368 [details] [review] Script Diff - Gnu Diff Hello, I added the patch (diff file) that I made using the Gnu Diff from the link above as requested above
Created attachment 112400 [details] [review] Diff for latest revision 25472 p.s. The diff I applied in the comment above was on the original script as it apperared in 2.4 compared to my complete fix. I'm attaching now a diff based on the latest revision of the script - revision 25472.
It doesn't help to provide a diff between your version which is based on the old script in 2.4 and the version in trunk. What we need is the diff between the original version you based your work on and the result of your changes.
Hello Sven, as I have said above: In Comment #21 I have a diff between the final copy with the string and the original bugged copy without any of the fixes. In Comment #22 I have a diff between the final copy with the string and the partial fix that was applied in gimp 2.4.5 (one of the two fixes - the one without the string). If I understand you right you want the diff between the original bugged copy (without any fix - as it appeared in GIMP 2.4.0) and the new one with both fixes. This diff is located in Comment #21 . If I'm wrong, please explain yourself in a more detailed way so I'll understand what do you need.
I don't understand why fixing this problem requires the script to grow by more than 50 lines of code. This looks like terrible code duplication to me.
Hello, Sorry for the late reply, I was a bit busy. I'll try looking over the code and see what can be shortened up in various ways. By the way, I have discovered an hour ago, that if the layer is out of the canvas (not inside the bounds of the canvas), The script won't work for the simple reason which is - there is no selection around it. it would be practically impossible to fix this bug, since it would require to rewrite this script from scratch. Should the user be prompted with an appropriate message? (Sorry for the late discovery - I know it would annoy the translators, yet no one ever thought about it) If you will give me some migration guide (ot some link where I can find all the script-fu PDB changes) for the script-fu for gimp 2.6, I'll try to update the syntax to.
Created attachment 114940 [details] [review] New Patch - More efficient and less bugs Hello, So I sat 5 hours on fixing this code to be bugless (as far as I can see) - I located some bugs that were not even described before. including: 1. Entering a white space as the string could have caused the script to crash - now it won't. 2. Layers outside of the image were ignored - now the error message can also indicate this problem 3. Entering text which was to small and caused the script to crash (a '.' sign for example) in the logo script have a unique error message. 4. Apperantly the layer adding was buggy which could result wrong order of the new layers that were added - I fixed that. 5. Improved efficency - less calls for PDB procedures, less lines of code, and less variables used compared to other fixes in this discussion. the Diff is between the original 2.4 buggy version and the final new version. The final script itself can be reviewed here http://lightningismyname.googlepages.com/FrostyLogo2.4_2.6_Final.scm New Strings were introduced as you can see, hope they will be translated in time for 2.6.
Thanks, that looks a lot better already. The messages need string review though. It would be nice if we could get away with one or two error messages instead of three slightly different ones that point out basically the same problem.
Created attachment 115009 [details] [review] String Fix Patch Hello sven, as requested, I reduced one string and gave 1 general error message for the logo part instead of 2 errors. The other one can not be reduced since I believe it's important for it to be kept like this for better understanding by the user. The final script (not the diff) can be found here http://lightningismyname.googlepages.com/FrostyLogo2.4_2.6_Final2.scm
This change conflicts with other changes that have been done to the script in trunk. The patch does not apply cleanly.
Created attachment 116033 [details] [review] Fixed Patch I have no idea why it didn't work, so I went and downloaded the latest revision in the SVN (rev 26411) and recreated the diff. I also fixed a typo in the string (spelled "to small" instead of "too small").
Thanks, I've done some cleanups and committed this to trunk: 2008-08-07 Sven Neumann <sven@gimp.org> * plug-ins/script-fu/scripts/frosty-logo.scm: applied slightly modified version of a patch provided by LightningIsMyName. Catches some cases where the script would fail (bug #472316). Actually, the message texts as well as the layer names should be localized. But I just realized that there's no support for this in Script-Fu. That's something that should really be added if we want to continue to ship scripts with GIMP...