Hi Berhard!<br><br>Sorry for delay in responding. The message required some discussions and<br>code reviewing so I have spent some time preparing reply. <br><br><div class="gmail_quote">On Sun, Nov 14, 2010 at 6:49 PM, Bernhard Herzog <span dir="ltr"><<a href="mailto:bh@intevation.de" target="_blank">bh@intevation.de</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
<br>
Hi all,<br>
<br>
I've looked more closely at the current state of Skencil's 0.6 branch.<br>
There are quite a few things that I think should be fixed before the<br>
final 1.0 release. Here's an incomplete list of what I've found so far.<br>
<br>
1. setup.py is broken<br>
<br>
Normally, I would expect to be able to run "python setup.py --help" to<br>
get information about how to use setup.py and that command shouldn't<br>
have any side-effects besides printing help information. Skencil's<br>
setup.py --help compiles the po-files. It also tries to find the<br>
installed tcl/tk version and complains if it doesn't find it and does<br>
not print the help text.<br></blockquote><div><br>Fixed.<br> </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
The reason is the way the new and extended distutils commands are<br>
implemented. They should be implemented the right way, as described in<br>
e.g. <a href="http://docs.python.org/release/2.5.4/dist/node32.html" target="_blank">http://docs.python.org/release/2.5.4/dist/node32.html</a><br></blockquote><div><br>There is no reason to spend time for developing full featured distutil command<br>
because in current revision we have specialized solution for Skencil which is<br>an extension for build command but not a generic solution.<br><br>Around bdist_deb command there was a lot of holywars because Debian<br>maintainers have own point of view for the issue. The build_and_copy command<br>
is just for internal purposes so generic solution has no sense.<br> <br></div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
The build&copy command doesn't work because the shutil module is not<br>
imported. Also, it shouldn't have an & in the name, because that is<br>
awkward in the shell.<br></blockquote><div><br>Fixed. The build&copy command is renamed as build_and_copy.<br> </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
The way Skencil is currently being installed is also problematic, but<br>
we're already discussing that in another thread.<br>
<br>
<br>
2. The dependencies should be documented somewhere<br>
<br>
In the older versions they were documented in the INSTALL file. Now<br>
there doesn't seem to be any documentation of the dependencies. In<br>
particular, the new dependency on gtk should be documented.<br></blockquote><div><br>You are right. The INSTALL file should be updated and included into distribution<br>because it could be useful for those who install Skencil from source code.<br>
</div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
3. Problems with src/Sketch/Base/gtkutils<br>
<br>
Several things that should be fixed in gtkutils:<br>
<br>
- it should use subprocess or popen2 with a list of strings instead of<br>
os.system with a command string. The command executed does not<br>
require a shell and you don't use any escaping in creating the<br>
command string. It could happen e.g. that the name of the temporary<br>
file contains e.g. spaces. Also, with subprocess or popen2 the<br>
temporary file is not even needed.<br></blockquote><div><br>pygtk could be utilized directly from Skencil code, but unfortunately <br>pygtk mainloop will be terminated only after python process exit. And if<br>you will try launching another application copy you will get following<br>
error:<br><br><pre>The program '-c' received an X Window System error.<br>This probably reflects a bug in the program.<br>The error was 'BadWindow (invalid Window parameter)'.<br> (Details: serial 1663 error_code 3 request_code 15 minor_code 0)<br>
(Note to programmers: normally, X errors are reported asynchronously;<br> that is, you will receive the error a while after causing it.<br> To debug your program, run it with the --sync command line<br> option to change this behavior. You can then get a meaningful
backtrace from your debugger if you break on the gdk_x_error() function.)<br></pre><br>I didn't test subprocess or popen2 because we have an os.system working solution.<br>Actually this is "workaround for workaround" because Tk has a workaround for those<br>
cases when color/font info cannot be imported from DE. This and a lot of similar issues<br>will be resolved by porting on mainstream widgetset (gtk/qt)<br><br>Any way you could propose your own implementation if you think that this improves Skencil.<br>
<br> </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
- The exit-code of the os.system call is ignored. If the subprocess<br>
fails it leads to strange errors later, because e.g. the temporary<br>
file remains empty.<br></blockquote><div><br>Yes I agree that in some cases this code can produce an exception. I have committed<br>small improvements to use built-in color/font values when exception is occurred. <br>
</div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
- The subprocess should use sys.executable as the name of the python<br>
interpreter to make sure its the same as the one used for Skencil<br>
itself. Otherwise it might be one where e.g. the gtk module is not<br>
available.<br>
<br>
- gtkutils.get_gtk_fonts() and gtkutils.ColorScheme() are executed when<br>
the main Sketch package is imported and results are bound to ui_fonts<br>
and ui_colors. This is bad for two reasons:<br>
<br>
* Skencil can now only be imported if an X11 connection can be<br>
established. This breaks skconvert which until now didn't require<br>
that. Of course, skconvert is broken anyway, see below.<br>
<br>
* ui_fonts and ui_colors are only used in the Sketch.UI package.<br>
Why are bound in the main Sketch package, then? Furthermore,<br>
ui_fonts is only used in one place, TkApplication.init_tk. Why<br>
not simply create it there? ui_colors is used in ruler.py, too,<br>
but it still doesn't need to be in Sketch. In fact, the whole<br>
gtkutils module should be in Sketch.UI, because it's UI specific<br>
and only used there.<br></blockquote><div><br>Actually gtkutils are extended and interactive variant of add_options() from configutil.py <br>Therefore this module is placed into Sketch.Base. The code is not used in UI directly.<br>
Only as a source of configuration data.<br><br> </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
4. skencil_run suppresses warnings<br>
<br>
Why does it do that? Which warnings does it intend to suppress? There<br>
should be a comment explaining that.<br>
<br>
<br>
5. skconvert and skshow are missing<br>
<br>
They should be reactivated again. Both require that the main Sketch can<br>
be imported without running the full Skencil application, which is<br>
impossible right now without changes. Also, skconvert must work without<br>
an X-connection as mentioned above.<br></blockquote><div><br>We have discussed this issue inside sK1 Team and decided that there is no reason <br>to support skconvert because a lot of filters are out of date. We would propose <br>
using UniConvertor as a translation tool because this application has a good *.sk<br>format support and allows processing more formats than Skencil/skconvert supports.<br>Also UniConvertor doesn't depend on Xlib and has been ported on win32 and macosx.<br>
<br><br> </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
6. The debian package depends on python-imaging-tk<br>
<br>
Skencil doesn't use ImageTk and AFAICT that hasn't changed.<br></blockquote><div><br>Yes, you are right! BTW package for 0.6.17 contains such dependency also:<br><br><a href="http://saveimg.ru/pictures/23-11-10/b9d7c7b2b0e21355da4dc24156db782e.png" target="_blank">http://saveimg.ru/pictures/23-11-10/b9d7c7b2b0e21355da4dc24156db782e.png</a><br>
<br>I have fixed this issue.<br><br> </div><blockquote class="gmail_quote" style="margin: 0pt 0pt 0pt 0.8ex; border-left: 1px solid rgb(204, 204, 204); padding-left: 1ex;">
7. Formatting:<br>
<br>
Some files have very long lines and much trailing whitespace:<br>
<br>
src/Sketch/UI/ruler.py<br>
src/Sketch/Base/gtkutils.py<br>
setup.py<br></blockquote><div><br> The lines are not wrapped because in such form the code is just easy for reviewing.<br>Wrapping was important 5-10 years ago when 15-17" monitors were conventional. But<br>now 22-24" displays are widely used as developer's and user's hardware:<br>
<br></div></div><a href="http://saveimg.ru/pictures/23-11-10/1692954d8b3e745b9e55e45fff85c1f0.png" target="_blank">http://saveimg.ru/pictures/23-11-10/1692954d8b3e745b9e55e45fff85c1f0.png</a><br clear="all"><br>-- <br>Regards,<br>
<br>Igor Novikov<br>
sK1 Project<br><a href="http://sk1project.org" target="_blank">http://sk1project.org</a><br><br><br>