Openembedded Core Discussions

* Re: [PATCH 1/4] lib/oe/terminal: add support for XFCE's terminal emulator
From: Chris Larson @ 2011-10-29  0:56 UTC (permalink / raw)
  To: Patches and discussions about the oe-core layer
In-Reply-To: <988279ad6b90b50cf85426709540ce19b00745e6.1319844419.git.josh@linux.intel.com>

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 adding this, I'm sure folks using stock Xubuntu and the
like will find this helpful (I'm an rxvt-unicode guy myself).
-- 
Christopher Larson
clarson at kergoth dot com
Founder - BitBake, OpenEmbedded, OpenZaurus
Maintainer - Tslib
Senior Software Engineer, Mentor Graphics

^ permalink raw reply