Re: GNUstep Printing Backend Patch and such

[Chad Hardin]

First, thank you very much for the review.  I appreciate your time in  
doing this very much.  Thanks!  to make my replies more clear, I  
divided it my answers up below.

On Jul 5, 2004, at 12:33 AM, Fred Kiefer wrote:
> I just started to look at the code of this great change and there are  
> a few minor things that should be corrected before the final version:
>
> - My opinion is that the new printing bundles should not be situated  
> inside of GUI, but in some directory alongside to it, taking that huge  
> amount of PPD files with them.

Ok, I have no opinion either way, so I'll defer to the *step  
leadership... :-)
BTW, this is by means the final version, this is just "Round One".   
Much more is to follow....

> - You removed all the ivars and some of the enumerators in the header  
> files. The ivars are fine, but even when GNUstep stops using the  
> enumerator we need to supply them, as some OpenStep application may  
> still rely on them and we don't want to frustrate a user. No big deal,  
> as enums don't directly result in any code. You did copy these enums  
> to the headers of the implementation files so there really was no  
> gain.

At first, I was going to make all the NS classes very abstract.  The  
thinking was that the differences between the UNIX type bundles and a  
Win32 bundle would be very great.  However, on second thought, I moved  
quite a few ivars and some of the private methods from GSLPR back to  
the NS classes.  I did this after I emailed this patch.  This allows  
more code sharing between the GSLPR bundle and the future GSCUPS  
bundle.  My logic is that a Win32 bundle will be so completely  
different that it will have to re-implement almost everything anyhow.   
So, rather than having some wacky three layered class architecture, I  
just put some more functionality back into the NS classes.

I'm afraid I'm not sure what your talking about in your reference to  
enumerators.  Would you please explain this further?

> - You added a method declaration -allocWithZone: to most of the  
> classes. This should never be needed as this method is inherited from  
> NSObject. Doing so to place an implementation detail comment doesn't  
> seem to be a justifiaction for this extra declaration. Just remove it  
> again.

My bad, I really know better.  Done.

> - Was it needed to reformat all the method declarations and headers?  
> They looked fine to me. Over all you seem to use a diffrent code  
> formating than the rest of GNUstep, which is a bit annoying, when you  
> just change the format of some methods. (Imagine me scrolling through  
> your pathc file, finding all real changes as opposed to reformatting)

Yeah, that was one of those things that seemed like a good idea at the  
time.  :-).  I had read the FSF and GNUstep coding standards  
(http://www.gnustep.org/resources/documentation/Developer/ 
CodingStandards/coding-standards_3.html#SEC3) and thought that while I  
was "in the code" I would set everything right.  Of course, I wasn't  
thinking about what it would look like when someone would look at the  
patch.

Now, I'm also not so sure about my interpretation of the coding  
guidelines.  I know that when using a method, it should be aligned as  
so:

[dict setObject: object
         forKey: key];

I took this to mean that methods declarations and definitions should  
also be the same way.  Personally, I find it much easier to read this  
way and have never run past the 80 character width limit doing it this  
way.  I sincerely apologize for doing this, it made the patch much  
bigger and more confusing than it had to be.

> - Why did you add a -init method to NSPrintInfo, -initWithDictionary  
> is the designated initializer, so this should do and be able to deal  
> with a nil parameter. And why don't you use ASSIGN in the code for  
> -setSharedPrintInfo: and replaced the RELEASE() in the -dealloc  
> method?

For -init,  it just seemed like a good place to put the default state  
of the object.  The dictionary may (or may not) write over those  
defaults.  I didn't see any harm in -init being there.  Is there some  
unforeseen consequence to it being there that I don't understand?  Not  
using RELEASE and especially ASSIGN() was just a very bad oversight :-)  
 Fixed.

> - What is the use of spliting up the file NSPrintOperation.m into  
> separate ones? It may be nice to have the one-class-per-file pattern,  
> but the classes here are to simple to request separate files.

Two reasons here.  One is that the printing bundles subclass  
GSPrintOperation, not NSPrintOperation, I wanted to make that easily  
apparent that there is a distinction between the two.  Also, I think  
there is a possibility that more classes like these will be added;  
there may be more outputs we would like to support in the future.   
Also, GSPDFPrintOperation may get really big when it is actually  
implemented.

> - When loading the GORM file for the panel you did stick with the old  
> code, which even stated that it is wrong. Have a look at  
> NSDataLinkPanel for a better way to do this.

That is because I did not want to mess with any implementation code for  
this round.  All I wanted to do was separate the NS classes from the GS  
bundles.  I know I'm contradicting myself here a bit because I did a  
lot of reformatting.  :-)  There is a a bit of justification though.  I  
had to understand all of these NS Classes line by line, and quite a bit  
of it had really bad formatting, seriously, so in order to understand  
it I went on a quest to fix up the formatting.  I guess I got a bit  
carried away.  :-)

> - I also would expect that there is some more common code between the  
> printing backends. You made some methods on the NS classes abstract  
> that I can only think of one possible implementation. Either you have  
> a lot more imagination than I, or you should bring back the  
> implementation here to share it. Perhaps you need to implement a  
> second backend first, to be sure which methods get shared.

After sending this patch, I put some more stuff back into the NS  
classes.  Like I said, a Win32 bundle would be so completely different  
that I see little point in making the NS classes so abstract.
Since the only backends like to ever exist are GSLPR, GSCUPS, and  
Win32, I think this will be fine.  To be frank, I'm not sure we'll ever  
see a Win32 printing backend bundle, such a beast would not be trivial  
in the slightest.

> After so many negative sounding statements I have to say that I fully  
> support this split into printing backends and that you should go ahead  
> with this.


Thanks!

Chad

> Fred
>
>
> _______________________________________________
> Bug-gnustep mailing list
> Bug-gnustep@gnu.org
> http://lists.gnu.org/mailman/listinfo/bug-gnustep