text: Learn how easy it is to sync an existing GitHub or Google Code repo to a SourceForge project! See Demo
  On Sun, Nov 18, 2012 at 6:40 PM, Mike Meyer <mwm@...> wrote:
  > Next time, please submit patches to
  > the sourceforge tracker. Or put them in an attachment (so many mailers
  > break formatted inline text that I just don't trust it).
  Got it. The patch is an attachment this time.
  > I've verified that it passes all the tests and checked it in. If this
  > breaks something for someone, they should feel free to back it out.
  Thanks!
  I bumped into two more problems while fixing the remaining issues with RENDER.
  Nesting more than two Structs didn't work correctly because Object's
  check_value() didn't flatten the argument. This resulted in Struct's
  to_binary() getting less values than it was expecting and raising an
  error. This is illustrated in the TestNestedStruct.testToBinary3 test
  in the attached test_manual.py.
  List didn't call check_value() of the contained object in its
  pack_value(), even though it does call parse_value() of the contained
  object in its parse_binary_value(). The RENDER extension has Lists of
  fixed-point values, so there's a need for the check_value() call.
  I've attached render-fixes-2.patch that takes care of these. I've also
  attached test_manual.py which has test cases for both this and the
  previous patch. Would it be a good idea to include it in the
  test-directory? The other tests seem to be automatically generated for
  the core protocol, but I think it wouldn't be bad idea to add some
  hand-crafted test for things as they come up.
  Thanks,
  Aleksi Torhamo
  Hi,
  While implementing the FIXED type (a fixed-point number) needed by the
  RENDER extension, I noticed that the conversion done by check_value()
  wasn't being done when the type was used in nested Structs, like this:
  Point = rq.Struct(
  Fixed('x'),
  Fixed('y'),
  )
  Line = rq.Struct(
  rq.Object('p1', Point),
  rq.Object('p2', Point),
  )
  I'm not that familiar with the internals of python-xlib, but based on
  reading Xlib/protocol/rq.py, I have the following picture about the
  packing and unpacking procedure:
  If a Field has a structcode, it's a fixed-length field and Struct will
  handle packing and unpacking the Field's data based on the structcode.
  The Field's parse_value() and check_value() will be called (if they
  exist) to refine the unpacked value or simplify the value before
  packing, respectively.
  If a Field's structcode is None, it's a variable-length field and
  Struct will call the Field's parse_binary_value() and pack_value()
  which will handle the unpacking and refining or simplifying and
  packing, respectively.
  Structs can't be directly used as Fields, but Object seems to exists
  solely for the purpose of nesting Structs. As far as I can see,
  Structs can only be nested by using either Lists or Objects, and
  Objects are only used in the source code with a Struct as the
  argument.
  The "refined" value of a Struct is a dictionary, so Struct's
  parse_value() just creates a dictionary based on the values it gets,
  filtering every value through the corresponding Field's parse_value(),
  if it exists. Structs don't have a check_value() method, so Object's
  check_value() handles that by itself by checking if the argument is a
  dictionary and converting it to a list if so.
  The problem I encountered seems to be caused by Object's check_value()
  not filtering the values through the corresponding Fields'
  check_value() methods.
  Assuming the picture above is correct, there is also much unused code
  in the Object methods. The parse_binary_value() and pack_value()
  methods of Object check whether the structcode is None or not, and act
  differently based on that, but as far as I can see, they will only
  ever get called if structcode is None, since those methods should only
  be called for variable-length fields. The parse_value() method checks
  the existence of parse_value() on the contained object, but that seems
  to be redundant if the contained object is always a Struct.
  I've included a patch that fixes the problem for me and seems to pass
  all the tests. Only the modifications to check_value() are strictly
  necessary.
  The patch can also be found at:
  --- a/Xlib/protocol/rq.py
  +++ b/Xlib/protocol/rq.py
  @@ -546,8 +546,6 @@ class FixedList(List):
  class Object(ValueField):
  - structcode = None
  -
  def __init__(self, name, type, default = None):
  ValueField.__init__(self, name, default)
  self.type = type
  @@ -555,41 +553,26 @@ class Object(ValueField):
  self.structvalues = self.type.structvalues
  def parse_binary_value(self, data, display, length, format):
  - if self.type.structcode is None:
  - return self.type.parse_binary(data, display)
  -
  - else:
  - scode = '=' + self.type.structcode
  - slen = struct.calcsize(scode)
  -
  - v = struct.unpack(scode, data[:slen])
  - if self.type.structvalues == 1:
  - v = v[0]
  -
  - if self.type.parse_value is not None:
  - v = self.type.parse_value(v, display)
  -
  - return v, buffer(data, slen)
  + return self.type.parse_binary(data, display)
  def parse_value(self, val, display):
  - if self.type.parse_value is None:
  - return val
  - else:
  - return self.type.parse_value(val, display)
  + return self.type.parse_value(val, display)
  def pack_value(self, val):
  - # Single-char values, we'll assume that mean an integer
  - if self.type.structcode and len(self.type.structcode) == 1:
  - return struct.pack('=' + self.type.structcode, val), None, None
  - else:
  - return self.type.pack_value(val)
  + return self.type.pack_value(val)
  def check_value(self, val):
  - if self.type.structcode is None:
  - return val
  -
  if type(val) is types.TupleType:
  - return val
  + vals = []
  + i = 0
  + for f in self.type.fields:
  + if f.name:
  + if f.check_value is None:
  + vals.append(val[i])
  + else:
  + vals.append(f.check_value(val[i]))
  + i = i + 1
  + return vals
  if type(val) is types.DictType:
  data = val
  @@ -601,7 +584,10 @@ class Object(ValueField):
  vals = []
  for f in self.type.fields:
  if f.name:
  - vals.append(data[f.name])
  + if f.check_value is None:
  + vals.append(data[f.name])
  + else:
  + vals.append(f.check_value(data[f.name]))
  return vals
  Does this sound okay?
  Thanks,
  Aleksi Torhamo
  On Sat, Nov 10, 2012 at 10:19 AM, Aleksi Torhamo <aleksi@...> wrote:
  > Hi,
  >
  > While implementing the FIXED type (a fixed-point number) needed by the
  > RENDER extension, I noticed that the conversion done by check_value()
  > wasn't being done when the type was used in nested Structs, like this:
  ...
  > I've included a patch that fixes the problem for me and seems to pass
  > all the tests. Only the modifications to check_value() are strictly
  > necessary.
  > The patch can also be found at:
  >
  I don't have an opinion on this one (I was hoping someone more
  knowledgeable than I would speak up). I don't see a problem, except
  that github patch files are ugly. Next time, please submit patches to
  the sourceforge tracker. Or put them in an attachment (so many mailers
  break formatted inline text that I just don't trust it).
  I've verified that it passes all the tests and checked it in. If this
  breaks something for someone, they should feel free to back it out.
  Thanks,
  <mike