Lawrence D'Oliveiro <l...@geek-central.gen.new_zealand> writes:
> In message <87fwvdb69k.fsf....@metalzone.distorted.org.uk>, Mark Wooding
> wrote:
> > for descr, attr, colours in [
> > ('normal', 'image', 'Normal'),
> > ('highlighted', 'highlight', 'Highlighted'),
> > ('selected', 'select', 'Selected')]:
> > colourlist = getattr(MainWindow, 'Colors%sList' % colours)
> > ## ...
>
> But then you lose the ability to match up the bracketing symbols.
You're right. I do.
> That’s why I put them on lines by themselves.
But the bracketing symbols are thoroughly uninteresting! All that
putting them on their own lines achieves is to draw attention to the
scaffolding at the expense of the building. It's great if you like the
Pompidou Centre, I suppose...
Besides, any editor worth its salt will highlight bracket mismatches for
the reader, or at least get its automatic indentation in a twist if you
get the brackets wrong.
> Maybe you should look at the code in context
> <https://github.com/ldo/dvd_menu_animator>, then you can express some
> more opinions on how to improve it.
1. class keyval: `are there official symbolic names for these
anywhere?' The gtk.keysyms module contains `Return', `KP_Enter',
`Left', `Up', `Right', and `Down'. Question: why are `returnkey'
and `enterkey' defined in hex and the others in decimal?
2. The MainWindow class only has the `Window' attribute described in
its definition. Apparently there are other attributes as well (the
ColorMumbleList family for one, also `LayerDisplay'). Similarly,
the LoadedImage class has attributes `Contents' and `FileName', but
I see attributes `RowStride', `ImageDrawSize' and `SrcDimensions'
used immediately below.
3. In SelectSaveMenu, there's a curious `for' loop:
for State in (gtk.STATE_NORMAL,) : # no need for gtk.STATE_INSENSITIVE
## blah
Firstly, this is obviously equivalent to `state = gtk.STATE_NORMAL'
followed by the loop body (YAGNI). Secondly, the loop is followed
by an `#end if' comment. If you're going to write the things, at
least get them right!
4. Ahh! I've tracked down where the MainWindow is actually
populated. Ugh. The code appears to revel in boilerplate. (For
those at home, we're now looking at `SetupMainWindow', specifically
at the calls to `DefineScrollingList'.) I count six calls, each
nearly (but not quite) identical, of about eleven lines each. Of
course, being boilerplate, the similarities visually outweigh the
differences, when in fact it's the differences that are
interesting.
It doesn't help that all of the names of the things involved are so
wretchedly verbose. How about the following?
def make_listview(label, render, colattr, head, bounds, osc, box):
setattr(MainWindow, label + 'Display',
DefineScrollingList(getattr(MainWindow, label + 'List'),
0, render, colattr, head,
bounds, osc, box)
def make_listview_l1(label, head, osc):
make_listview(label, None, 'text, head, (160, 160), osc, List1Box)
make_listview_l1('Button', 'Buttons', None)
make_listview_l1('Layer', 'Button Layer', LayerSelectionChanged)
MainWindow.colourlist = {}
MainWindow.colourview = {}
for head in ['Normal', 'Highlight', 'Select']:
MainWindow.colourlist[head] = gtk.ListStore(gobject.TYPE_PYOBJECT)
MainWindow.colourview[head] = \
DefineScrollingList(MainWindow.colourlist[head],
0, ColorCellRenderer(), None,
head, (120, 120), None, List2Box)
make_listview('LoadedColors', ColorCellRenderer(), None, '',
(120, 240), None, MiddleBox)
Looking at some of the rest of the code, it might (or might not) be
worthwhile actually making a class to gather together a list's model
and view; subclasses can then vary their individual parameters, and
the various colour lists can also keep the various captions with
them. (Sometimes a strong separation between model and view is a
good thing; this doesn't seem to be one of those times.)
5. Much of the indentation and layout is rather unconventional, though
not intolerable. But I found (deep in `SelectSaveMenu'):
NewRenderPixels = array.array \
(
"B",
FillColor
*
(ImageRowStride // 4 * ImageBounds[1])
)
to be most disconcerting. Also, I couldn't work out why some
parens are indented only two spaces and others are indented the
full eight. Oh! It's inconsistent tab/space selection.
I'm afraid that about this point I had to get on with some other stuff.
I've done enough throwing of stones, so I should point at a glass house of
my own so that others can return the favour:
http://git.distorted.org.uk/~mdw/tripe/tree
The Python code is in `py', `svc' and `mon'. Those who believe PEP 8 is
carven in stone will be disappointed. It's a bit... tricky in places.
-- [mdw]
--
http://mail.python.org/mailman/listinfo/python-list
