[Skencil-devel] Things to fix for Skencil 1.0

Fri Nov 26 15:39:51 CET 2010

Hi Berhard!

Sorry for delay in responding. The message required some discussions and
code reviewing so I have spent some time preparing reply.

On Sun, Nov 14, 2010 at 6:49 PM, Bernhard Herzog <bh at intevation.de> wrote:

>
> 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.
>

Fixed.

> 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
>

There is no reason to spend time for developing full featured distutil
command
because in current revision we have specialized solution for Skencil which
is
an extension for build command but not a generic solution.

Around bdist_deb command there was a lot of holywars because Debian
maintainers have own point of view for the issue. The build_and_copy command
is just for internal purposes so generic solution has no sense.

> 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.
>

Fixed. The build&copy command is renamed as build_and_copy.

> 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.
>

You are right. The INSTALL file should be updated and included into
distribution
because it could be useful for those who install Skencil from source code.

> 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.
>

pygtk could be utilized directly from Skencil code, but unfortunately
pygtk mainloop will be terminated only after python process exit. And if
you will try launching another application copy you will get following
error:

The program '-c' received an X Window System error.
This probably reflects a bug in the program.
The error was 'BadWindow (invalid Window parameter)'.
  (Details: serial 1663 error_code 3 request_code 15 minor_code 0)

  (Note to programmers: normally, X errors are reported asynchronously;
   that is, you will receive the error a while after causing it.
   To debug your program, run it with the --sync command line
   option to change this behavior. You can then get a meaningful
   backtrace from your debugger if you break on the gdk_x_error() function.)

I didn't test subprocess or popen2 because we have an os.system working
solution.
Actually this is "workaround for workaround" because Tk has a workaround for
those
cases when color/font info cannot be imported from DE. This and a lot of
similar issues
will be resolved by porting on mainstream widgetset (gtk/qt)

Any way you could propose your own implementation if you think that this
improves Skencil.

>  - 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.
>

Yes I agree that in some cases this code can produce an exception. I have
committed
small improvements to use built-in color/font values when exception is
occurred.

>  - 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.
>

Actually gtkutils are extended and interactive variant of add_options() from
configutil.py
Therefore this module is placed into Sketch.Base. The code is not used in UI
directly.
Only as a source of configuration data.

> 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.
>

We have discussed this issue inside sK1 Team and decided that there is no
reason
to support skconvert because a lot of filters are out of date. We would
propose
using UniConvertor as a translation tool because this application has a good
*.sk
format support and allows processing more formats than Skencil/skconvert
supports.
Also UniConvertor doesn't depend on Xlib and has been ported on win32 and
macosx.

> 6. The debian package depends on python-imaging-tk
>
> Skencil doesn't use ImageTk and AFAICT that hasn't changed.
>

Yes, you are right! BTW package for 0.6.17 contains such dependency also:

http://saveimg.ru/pictures/23-11-10/b9d7c7b2b0e21355da4dc24156db782e.png

I have fixed this issue.

> 7. Formatting:
>
> Some files have very long lines and much trailing whitespace:
>
>   src/Sketch/UI/ruler.py
>   src/Sketch/Base/gtkutils.py
>   setup.py
>

 The lines are not wrapped because in such form the code is just easy for
reviewing.
Wrapping was important 5-10 years ago when 15-17" monitors were
conventional. But
now 22-24" displays are widely used as developer's and user's hardware:

http://saveimg.ru/pictures/23-11-10/1692954d8b3e745b9e55e45fff85c1f0.png

-- 
Regards,

Igor Novikov
sK1 Project
http://sk1project.org
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.wald.intevation.org/pipermail/skencil-devel/attachments/20101126/3ddc0bed/attachment.html