Created attachment 90059 [details]
Psd plug-in source

What is the reason for writing a new plug-in instead of improving the existing one? Is this based on the old plug-in or is it really a rewrite from scratch? Does it incorporate the many bug-fixes and improvements that have gone into the PSD plug-in lately?

My original intention was to improve the existing one but after looking at for some time I considered that it had become unmanageable and would be extremely difficult to work on (for me at least).  The new plug-in really is written from scratch using the latest publicly available version of the file specs (V6). The style was in fact initially loosly based on the PSP plug-in. I have followed the changes that have been made to the current plug-in and the only one that has not been applied is the Unicode layer name patch that went in a couple of days ago (which does not work on windows by the way).  I believe that the new plug-in has all of the features of the existing one in addition to the new ones and the code is now much clearer and will be easier to extend and to port to GEGL when that is incorporated into GIMP.

It is a lot more work to review a complete rewrite. We can't accept this code without review, in particular not now as we are close to a stable release. So let's confirm this bug and hope that someone finds the time to review it for GIMP 2.6.

What about the unicode layer name patch doesn't work on Windows? Please post more details about the problem and a sample psd that exhibits the problem to bug #445316 so that I can fix it.

A few comments on the code:

- Please make sure that you incorporate the recent changes that validate the input to avoid buffer overflows and other vulnerabilities.

- Please do only use g_warning() for programming errors, not to warn about invalid input. In most cases you will want to use g_message() here to bring up a user-visible message dialog.

- Please rename your files to use hyphens instead of underscores.

Created attachment 94575 [details]
Psd plug-in source

An updated version of the PSD plug-in source including:

- Buffer overflow checks as requested.
- g_warning() has been replaced with g_message() throughout.
- source file names use hyphens instead of underscores.
- Better EXIF and XMP handling.
- Initial thumbnail loading code.
- Various memory leaks and bugs fixed.

IMO we should mark some (all?) of the other PSD-related bugs as depending on this one.

Hi John,

  Given that PSD is one of the most important formats (in the sense that
many users have PSD files that they would like to be able to open), can you
give us a sense of the extent to which you'll be able to maintain this
plug-in?  That is, how vigorously will you be able to respond to bug
reports and/or changes in the format?  It would be a huge gain over
the current situation to have a PSD plug-in that works for current 
PSD files and is actively maintained for some reasonable period of
time.  If you can give a satisfactory answer to this question, I'll
push strongly for getting your plug-in into svn -- and now would be
the time to do it.

Hi,

I would hope to be able to at least look at bugs within the week and it is my intention to maintain this plug-in for the forseable future. 
My intention is to add support for PSD features as they become available in GIMP and to add support for new PSD functionality as we figure out how it works!
I would like to hold off on getting the plug-in into svn for a week or two until I finish testing my current version after that I would be delighted to see it get a wider airing.

I have created the updated version of the plug-in source and a patch for the configure/make files.  In addition the psd-load.c source file would need to be removed from plug-ins/common.

Created attachment 100909 [details]
Psd plug-in source

Created attachment 100910 [details] [review]
Psd plug-in makefile patch

With a couple of minor tweaks (a couple of unused argument warnings, and a warning about an uninitialized variable that was caused by an obvious typo in the code), I built this without much difficulty.  I tested it on a couple of rather complex PSD files, and it loaded them successfully.  It tends to generate a lot of warnings about things that aren't fully supported, which will probably have to be tidied up a bit.  I also renamed two of the files.  The code style looks basically good except for a couple of minor violations, mainly a lot of trailing white space and some differently formatted function argument blocks.

I'm going to attach a bzipped tar file containing the psd directory with my minor modifications.

Created attachment 100923 [details]
bzipped tar of psd source, slightly modified

Hi Bill,

Thanks for looking at the code.  I have updated the source from your version to remove the remaining trailing spaces and reformating the rest of the function argument blocks.  I also converted a few rogue tabs to spaces.

The file conversion warnings are controlled by the value of the define CONVERSION_WARNINGS in psd.h.  I have set this to FALSE to suppress the "information will be lost" messages. Setting it to TRUE will re-enable.

In addition I made one small change to fix the unused variable warnings and will attach the resulting directory as a bzipped tar.

Created attachment 100965 [details]
bzipped tar of psd source, slightly modified some more

Sven and I both think that this should be committed.  As a final preliminary, I have sent an email to the gimp-developer list asking for comments.  Assuming there are no strong objections, one of us will commit this to the development branch in a few days.

I have committed this plug-in to trunk after some minor coding style cleanups.

2007-12-20  Sven Neumann  <sven@gimp.org>

        * configure.in
        * plug-ins/Makefile.am
        * plug-ins/psd: added new PSD load plug-in written by John Marshall.
        This plug-in adds a couple of features. See bug #448181 for details.

        * plug-ins/common/Makefile.am
        * plug-ins/common/plugin-defs.pl
        * plug-ins/common/psd-load.c: removed old psd-load plug-in.

We definitely need to review error handling. It is unacceptable to pass errors such as "Error writing pascal string" to the user. There are tons of messages that are clearly not meant to be seen by the user ever and these are even marked for translation. This needs to be changed before we add the new plug-in to POTFILES.in so that it is seen by the translators.

I'm sorry about the error handling, I took the style from the psp plug in.  Could you please suggest where I could look to see an example of how to handle such errors in an appropriate way and I will work on cleaning up the code.

Thanks
John

There are two things here. One is handling of general file I/O problems. This is quite unlikely to ever happen and it can probably be handled better by showing a more general message like "Error loading PSD file: %s" using the standard file error strings. This is best done using by using adding a GError parameter to the file utility functions and creating this error using g_file_error_from_errno().

The other thing is conversion warnings. I would suggest that these stay compiled in. But instead of using g_message(), the warnings could be collected and shown in a dialog raised by the PSD plug-in. Such a dialog could have a "Don't show me such warnings again." check-box.

I am including a patch below that uses GError to manage the file I/O errors and to report any file corruption errors (I hope that I have used GEror in an appropriate manner). I have significantly reduced the number of messages displayed and the number marked for translation.  I have also used messages that already exist in po-plug-ins wherever possible.

In this patch I have removed the translation macros from all of the conversion warnings but left them as not being compiled in at present.  I will work on the conversion warnings as a seperate patch.

The patch also includes fixes for a couple of memory leaks that were identified during testing.

Created attachment 102326 [details] [review]
psd plug-in error handling clean-up.

Thanks. I have committed this to trunk:

2008-01-08  Sven Neumann  <sven@gimp.org>

	* plug-ins/psd/psd-image-res-load.[ch]
	* plug-ins/psd/psd-layer-res-load.[ch]
	* plug-ins/psd/psd-load.c
	* plug-ins/psd/psd-thumb-load.c
	* plug-ins/psd/psd-util.[ch]
	* plug-ins/psd/psd.[ch]: applied a patch from John Marshall that
	improves error handling of the new PSD load plug-in (bug #448181).

I am closing this as FIXED now. If there are further improvements to the plug-in, please open a new bug report for it.