Allowing comments after the line continuation backslash

[Mark Wooding]

Lawrence D'Oliveiro <ldo at geek-central.gen.new_zealand> writes:

> In message <87fwvdb69k.fsf.mdw at 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]