Re: [PATCH 1/4] lib/oe/terminal: add support for XFCE's terminal emulator

[Joshua Lock <josh@linux.intel.com>]

On 28/10/11 17:56, Chris Larson wrote:
> On Fri, Oct 28, 2011 at 4:42 PM, Joshua Lock <josh@linux.intel.com> wrote:
>> That's Terminal on Fedora and xfce4-terminal on Ubuntu/Debian... This
>> could get interesting!
>>
>> Signed-off-by: Joshua Lock <josh@linux.intel.com>
>> ---
>>  meta/lib/oe/terminal.py |   21 +++++++++++++++++++++
>>  1 files changed, 21 insertions(+), 0 deletions(-)
>>
>> diff --git a/meta/lib/oe/terminal.py b/meta/lib/oe/terminal.py
>> index 1455e8e..b90a1f2 100644
>> --- a/meta/lib/oe/terminal.py
>> +++ b/meta/lib/oe/terminal.py
>> @@ -57,6 +57,27 @@ class Gnome(XTerminal):
>>     command = 'gnome-terminal --disable-factory -t "{title}" -x {command}'
>>     priority = 2
>>
>> +class Xfce(XTerminal):
>> +    def distro_name():
>> +        import subprocess as sub
>> +        try:
>> +            p = sub.Popen(['lsb_release', '-i'], stdout=sub.PIPE,
>> +                          stderr=sub.PIPE)
>> +            out, err = p.communicate()
>> +            distro = out.split(':').strip()
>> +        except:
>> +            distro = "unknown"
>> +        return distro
>> +
>> +    # Upstream binary name is Terminal but Debian/Ubuntu use
>> +    # xfce4-terminal to avoid possible(?) conflicts
>> +    distro = distro_name()
>> +    if distro == 'Ubuntu' or distro == 'Debian':
>> +        command = 'xfce4-terminal -T "{title}" -e "{command}"'
>> +    else:
>> +        command = 'Terminal -T "{title}" -e "{command}"'
>> +    priority = 2
> 
> The first problem I see with this is you're calling lsb_release at
> module import time. Doing things like that is generally a no-no. I'd
> suggest shifting it into the constructor. Set up the command there as
> self.command, and be sure to call the superclass constructor as well,
> after the command bits. The other thing is, and I'm sure you just
> copied & pasted it from konsole, but we already import a fully
> functional Popen -- it's the superclass of all the terminal classes.
> So there's no need to pull in subprocess's version of it. And the
> defaults of the Popen we use ensure the output is captured, so there's
> no need for the sub.PIPE bits at all if you use that one. Simply do p
> = Popen(['lsb_release', '-i']).

Thanks for the feedback Chris. I'll roll your suggestions into a v2 today.

> 
> Thanks for adding this, I'm sure folks using stock Xubuntu and the
> like will find this helpful (I'm an rxvt-unicode guy myself).

I figured that DE would be getting more popular, hence throwing a patch
together.

Cheers,
Joshua
-- 
Joshua Lock
        Yocto Project "Johannes factotum"
        Intel Open Source Technology Centre