[Skencil-devel] Things to fix for Skencil 1.0

[Bernhard Herzog]

Hi all,

I've looked more closely at the current state of Skencil's 0.6 branch.
There are quite a few things that I think should be fixed before the
final 1.0 release.  Here's an incomplete list of what I've found so far.

1. setup.py is broken

Normally, I would expect to be able to run "python setup.py --help" to
get information about how to use setup.py and that command shouldn't
have any side-effects besides printing help information.  Skencil's
setup.py --help compiles the po-files.  It also tries to find the
installed tcl/tk version and complains if it doesn't find it and does
not print the help text.

The reason is the way the new and extended distutils commands are
implemented.  They should be implemented the right way, as described in
e.g.  http://docs.python.org/release/2.5.4/dist/node32.html

The build&copy command doesn't work because the shutil module is not
imported.  Also, it shouldn't have an & in the name, because that is
awkward in the shell.

The way Skencil is currently being installed is also problematic, but
we're already discussing that in another thread.


2. The dependencies should be documented somewhere

In the older versions they were documented in the INSTALL file.  Now
there doesn't seem to be any documentation of the dependencies.  In
particular, the new dependency on gtk should be documented.


3. Problems with src/Sketch/Base/gtkutils

Several things that should be fixed in gtkutils:

 - it should use subprocess or popen2 with a list of strings instead of
   os.system with a command string.  The command executed does not
   require a shell and you don't use any escaping in creating the
   command string.  It could happen e.g. that the name of the temporary
   file contains e.g. spaces.  Also, with subprocess or popen2 the
   temporary file is not even needed.

 - The exit-code of the os.system call is ignored.  If the subprocess
   fails it leads to strange errors later, because e.g. the temporary
   file remains empty.

 - The subprocess should use sys.executable as the name of the python
   interpreter to make sure its the same as the one used for Skencil
   itself.  Otherwise it might be one where e.g. the gtk module is not
   available.

 - gtkutils.get_gtk_fonts() and gtkutils.ColorScheme() are executed when
   the main Sketch package is imported and results are bound to ui_fonts
   and ui_colors.  This is bad for two reasons:

    * Skencil can now only be imported if an X11 connection can be
      established.  This breaks skconvert which until now didn't require
      that.  Of course, skconvert is broken anyway, see below.

    * ui_fonts and ui_colors are only used in the Sketch.UI package.
      Why are bound in the main Sketch package, then?  Furthermore,
      ui_fonts is only used in one place, TkApplication.init_tk.  Why
      not simply create it there?  ui_colors is used in ruler.py, too,
      but it still doesn't need to be in Sketch.  In fact, the whole
      gtkutils module should be in Sketch.UI, because it's UI specific
      and only used there.


4. skencil_run suppresses warnings

Why does it do that?  Which warnings does it intend to suppress?  There
should be a comment explaining that.


5. skconvert and skshow are missing

They should be reactivated again.  Both require that the main Sketch can
be imported without running the full Skencil application, which is
impossible right now without changes.  Also, skconvert must work without
an X-connection as mentioned above.


6. The debian package depends on python-imaging-tk

Skencil doesn't use ImageTk and AFAICT that hasn't changed.


7. Formatting:

Some files have very long lines and much trailing whitespace:

   src/Sketch/UI/ruler.py
   src/Sketch/Base/gtkutils.py
   setup.py