Re: [Gnome-print] First draft of new encoding routines

[Scott Gifford <sgifford tir com>]

A new release of the filter library is available at:

    http://www.tir.com/~sgifford/filter/

.  It addresses most of the issues Chema and Lauris brough up.  It
doesn't address issues of how deeply it will be integrated into
gnome_print, or potential prefix conflicts.  There's some things I
need to figure out before I can make a decision...see below.

  Please let me know if you have any comments on the new version!  I
really appreciate all of the feedback on the previous one.

Chema Celorio <chema@ximian.com> ("CC") and 
Lauris Kaplinski <lauris@ximian.com> ("LK") wrote:

    LK> Scott!  If you want your filters to be included in
    LK> gnome-print, you have to make these available under LGPL. I
    LK> started porting it to gnome-print stable tree, but discovered,
    LK> that legally I have no permission to distribute it! So I do
    LK> not commit, which is BAD THING, because then we all cannot
    LK> test single build tree :( I understand, that often such issues
    LK> are splitting of hair, but after all we want to show everyone,
    LK> that WE follow copyright/licensing terms of free software, to
    LK> be able to ask that from others ;-)

That was a mistake, the license is changed in the new tarball.  I got
confused by the antics of "autoconf -a".  :-)  Sorry!

Originally it was licensed under the standard GPL, though, so
certainly you had permission to distribute it...

    CC> Looks good, some comments :
    CC> 
    CC> - Please add the GPL header to all the .c files.

OK, I have done this (with the LGPL header).

    CC> - Patch agains -ps2 not -ps

Right, I posted that patch a day or two after my original.  I'll
include the updated patch in my new tarball.

    CC> - If we move this code to the gnomeprint
    CC> directory we should change the filenames to gnome-print-filter

    LK> I will place your code into separate subdirectory filter under
    LK> libgnomeprint at first, so we create temporary .la library
    LK> during compilation. I you plan (we/somebody plans) to
    LK> distribute these in separate library, we can easily switch
    LK> over using it.
    LK> 

I haven't quite decided what to do about this.  It looks like maybe
Mozilla could use this filter code, too, which would mean I would need
to dual-license it under the LGPL and Mozilla's MPL.  For it to become
a part of gnome-print, I would probably have to assign copyright to
the FSF, which would make it impossible for me to do this
dual-licensing.  If somebody has a solution for this, I could just
make everything gnome-print-filter*, make the official home of this
work gnome-print, and just use the relevant parts for other projects
as needed.  Otherwise, I will probably need to maintain it as a
seperate library to maintain copyright.  Of course, it's LGPL, so it
isn't really a big problem either way, but it would be more convenient
for everybody if we could just make all of this a part of gnome-print.

    CC> - Use a better Prefix. "Filter" and "filter_.." is too
    CC> vauge. Since this symbols will end up in gnome-print, an app
    CC> that uses gnome-print might have a symbol crash, think about
    CC> filter_free for example.

    LK> As of namespace, well, little google search would do good. I
    LK> checked 'filter_init' and it gave me quite a long list of
    LK> matches (although I had no time to check, whether any of these
    LK> could cause real problems).  I would suggest prefixing
    LK> everything with something like sff_ (Scott Free Filter). There
    LK> were some matches to sff too, but these seemed to come from
    LK> some higher-level language.  The real problem is, that most
    LK> 2-letter namespaces are already occupied, as are big number of
    LK> 3-letter ones. And you cannot actually use too long prefix, as
    LK> AFAIK there are some silly limits of distinguishable name
    LK> length on some systems. Plus long prefixes are extremely
    LK> difficult to read/type/distinguish.

If the library becomes part of gnome_print, I'll probably use
gnome_print_filter; do you think that's too long?  I'm not really
familiar with the variety of compilers out there.

Otherwise, I'll have to think of something else.  :)

    CC> - Add comments to the source, we might need to fix bugs here
    CC> later and the comments will save us time. I looked for example
    CC> at the rle filter and at the old one, adding this notes really
    CC> help if we need to look at this code after we forget about it.

OK, I added some basic comments.  Obviously it's tricky with code like
this, since I know what it does.  If anybody finds places where it's
confusing what's going on, let me know and I'll clarify with comments.

    CC> - You don't seem to want to use glib functions.  assert,
    CC> strdup, malloc, realloc can be replaced with g_assert,
    CC> g_strdup, g_malloc, g_realloc.  Maybe you don't want this code
    CC> to depend on glibc. Is this your intention ?

I wrote the code in the manner I'm most familiar with, and that
involves using the standard C functions.  I tried to do all of the
error checking that the glib versions provide, so there shouldn't
really be any disadvantage to this.

I've been looking around to see some of the places this code could be
useful, and have found a few (particularly Mozilla) that compile on
platforms where glib is not easily available, so I'd like to continue
using the standard C functions.

Let me know if you can think of any problems this causes (apart from
my own inconvenience).

    CC> - Dont' hardcode sizes of buffers, use a #define instead grep
    CC> for 128 for example.  

OK, I have fixed this everywhere where it made sense.  One place I did
not fix it is this:

    filter_hexenc.c:83:  const char tohex[16] = "0123456789abcdef";

#defining a constant for this didn't seem to help in any way for that
code.

I also #defined the line-length constant and some other constants that
I should have #defined from the beginning.

There may be a few other places constants make sense; when there are
no architectural issues, I'll go over the code with a fine-tooth come
looking for things like this.

    CC> [The rest of the comments fall on the personal coding style,
    CC> inside gnome-print we have several coding sytles mixed. This
    CC> is not what we want and should standarize, I just don't want
    CC> to make this even worse if possible. Hoever, this is a
    CC> separate library so if you have strong preferences you can
    CC> leave it like that]

    LK> I do not care about coding style, as long as code is clearly
    LK> separate unit (as your filter code will be), and does not need
    LK> extensive maintenance/bugfixing on my side. But if I have to
    LK> do that, I tend to convert everything to my personal style ;-)
    LK> That is quite logical too, as every programmers eye is trained
    LK> with certain visual patterns in code, so finding bugs from
    LK> differently formatted code is much bigger pain.

At least while I'm doing most of the work on this library, I'd prefer
to keep it in my own style.  As Lauris mentions, I am accustomed to
this style, and can both code and debug much more quickly using it.
If we decide to standardize on a style throughout gnome-print, I would
be willing to conform to it, though.

Sorry, old habits die hard.  :)

    CC> I am sure more comments will come up, but most of the
    CC> important stuff is here. I just looked at the code, i didn't
    CC> had time to actually play with it. How much testing have you
    CC> done ? can you write a test program so that we can look at it
    CC> ?  We need to move this code to the cvs as soon as we can.

Yes, there is a test suite included in the "t" subdirectory of the
tarball.  Run the program "test" in there for a 15-minute, quite
thorough test of your build of the filter stuff.  "testwith" is a
script that makes it easier to test individual cases, and is also
included.  They do the encoding with "filtertest", included in
filter.tar, decode the results with GhostScript, and make sure it's
identical to the original file.  These tests proved incredibly useful;
they helped me track down at least a dozen bugs.

Is there any infrastructure for including these tests in gnome-print?
If the filters get completely folded into gnome-print, I'd hate to
lose them.

Thanks again,

----ScottG.