Tk Source Code

View Ticket
Login
Ticket UUID: d4fb4e80d220e46e588f310291fd7a4205e8cd67
Title: Image photo commands read/write/put do not accept "-" as first letter in file names resp. image data
Type: Patch Version: 8.6.6
Submitter: anonymous Created on: 2016-12-06 21:35:23
Subsystem: 41. Photo Images Assigned To: jan.nijtmans
Priority: 6 Severity: Important
Status: Closed Last Modified: 2017-01-11 13:30:53
Resolution: Fixed Closed By: jan.nijtmans
    Closed on: 2017-01-11 13:30:53
Description:
If specifying file names starting with "-" (example "-folder.gif") to photo commands read and write gives error:
unrecognized option "-folder.gif": must be -format, -from, -shrink, or -to
    while executing
"$img2 read "-folder.gif""

Same problem when having image data for command "put" starting with "-" (i.e. integer 45):
unrecognized option "--..." must be -format, or -to
    while executing
"$img2 put [binary format "cu*" $rowList]

Attached is a patch which works for me. It may break existing code, which does not conform to the documentation. Documentation says:
read fileName options
while read options filename does work with current implementation.
Bug seems to be present in 8.5, too.
User Comments: jan.nijtmans added on 2017-01-11 13:30:53:
Thanks, François, for pursuing this!

fvogel added on 2017-01-11 13:05:33:
Better fix merged by Jan Nijtmans in core-8-5-branch, core-8-6-branch and trunk.
Thanks!

fvogel added on 2017-01-10 20:31:52:
Makes sense. Please proceed :-)

jan.nijtmans added on 2017-01-10 16:14:38:
Having a look ... I'm not very fond of this fix, although I didn't find a mistake in it yet. I would expect the function ParseSubcommandOptions() to handle everything consistently, without needing code changes outside of it to correct for misbehavior. If ParseSubcommandOptions() misbehaves, I would expect this function to be corrected. I'll try to come up with a better solution.

fvogel added on 2017-01-10 09:29:31:
Trying to find what more this fix could break, I have used teacup to install everything I could (on Win32_x64) from the Activestate Teapot repository.

This did not work till its end (even with --force), ending up with some internal Tcl error (background) in teacup because of permission denied. Nevertheless, a lot of packaqes could be installed before that error.

Searching in the Tcl source code of all of these packages I could not find any further occurrence of wrong (swapped params) use of the image/photo put|read commands.

I wanted to add rkeene's teapot repo as well but could not manage to connect to it using teacup.

All in all, this fix should not break anything else I know than Tklib, for which I have opened an adequate bug report (can't fix it myself, no write access to the repo).

fvogel added on 2017-01-09 17:34:10:

I have opened a bug report for tklib.


fvogel added on 2017-01-09 17:23:56:
In tklib, in the ico module, there are four occurrences of "$img put -to $x $y $data", two of which are enclosed in an if {0}. There is no occurrence of "$img read -option $data" however. That's a limited breakage.

fvogel added on 2016-12-07 21:11:36:

Thanks for the report, patch and test scripts.

I have committed your patch in branch bug-d4fb4e80d2 for easy testing.

<TODO>: remaining work to do is to transform your test scripts into proper Tcl non-regression tests.

Also, with this patch the following test now fails, which is certainly expected since it uses the syntax "options data" (as opposed to data before options, which is the only documented syntax AFAIK):

==== imgPhoto-4.29 ImgPhotoCmd procedure: put option FAILED
==== Contents of test case:

photo1 put -to 10 10 20 20 {{white}} photo1 get 19 19

---- Test generated error; Return code was: 1 ---- Return code should have been one of: 0 2 ---- errorInfo: can't parse color "-to" while executing "photo1 put -to 10 10 20 20 {{white}}" ("uplevel" body line 2) invoked from within "uplevel 1 $script" ---- errorCode: TK VALUE COLOR ==== imgPhoto-4.29 FAILED

Changing this test by swapping the order of the -to option and the data {{white}} makes it pass.

The question here is, as you mentioned, how many scripts using the wrong syntax will be broken by fixing the bug. I suggest to fix this only in trunk (i.e. not in the 8.6 maintenance releases).


Attachments: