[Parti-discuss] rebase questions

[Nathaniel Smith]

On Tue, Nov 10, 2009 at 8:42 AM, Antoine Martin <antoine at nagafix.co.uk> wrote:
> Hi Nathaniel,
>
> I was looking at the changes whilst rebasing my patches on mainline and
> I've got a few questions:
>
> 1)
> def draw(self, x, y, width, height, rgb_data):
>        assert len(rgb_data) == width * height * 3
>        (my_width, my_height) = self.window.get_size()
>        gc = self._backing.new_gc()
>        self._backing.draw_rgb_image(gc, x, y, width, height,
>                                     gtk.gdk.RGB_DITHER_NONE, rgb_data)
>        self.window.invalidate_rect(gtk.gdk.Rectangle(x, y, width, height),
>                                    False)
>
> my_width and my_height aren't used, lint told me ;)

Good catch, fixed.

> 2) As mentionned in the other email, can you please merge this patch:
> http://xpra.devloop.org.uk/patches/v0.0.8pre/mswindows-move-eventcode.patch
> This really seems like a reasonable thing to do and would massively
> reduce the amount of rebasing I have to do to keep win32 support in sync.
> Please, please, please.

See my reply to the other email (when I finish writing it :-)).

> 3) xvfb switch:
>        xvfb = subprocess.Popen(["Xvfb-for-Xpra-%s" % display_name,
>                                     display_name,
>                                     "-auth", xauthority,
>                                     "+extension", "Composite",
>                                     # Biggest easily available monitors are
>                                     # 1920x1200. This is 1920*2 x 1920:
>                                     "-screen", "0", "3840x1920x24+32",
>                                     "-once"],
>                                    executable="Xvfb")
>
> Did you test this?
> executable="Xvfb" should be opts.xvfb surely?
> Also AFAIK, this does not allow you to specify arguments to Xvfb (or
> Xvnc), like: --xvfb="Xvnc --PasswordFile=blah"

Err, doh. Yeah, apparently half the patch got lost somewhere... maybe
I somehow reverted xpra/scripts/server.py before committing? Anyway,
now committed and tested.

And I'm a bit confused about the complaint about passing arguments,
because one of the things I disliked about your patch is that it
didn't allow passing arguments :-). But yes, the version in mainline
supports passing arguments.

> 4) Still some unused imports, ie: xpra/scripts/server.py

You mean the 'import time'? Fixed. (And I swear I did this before
too... oh well.)

> And "from wimpiggy.error import *" is making lint shout at me. :(

Okay, like I said I don't think it matters much at this point, so send
a patch :-). (If I did it I'd probably be too lazy to look up exactly
which files reference 'trap', which reference 'XError', and which
reference both, and then you'd just end up sending me a patch
complaining about unused variables anyway, so this way seems simpler.)

-- Nathaniel