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

login
register
mail settings
Submitter Joshua Lock
Date Oct. 28, 2011, 11:42 p.m.
Message ID <988279ad6b90b50cf85426709540ce19b00745e6.1319844419.git.josh@linux.intel.com>
Download mbox | patch
Permalink /patch/13993/
State New, archived
Headers show

Comments

Joshua Lock - Oct. 28, 2011, 11:42 p.m.
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(-)
Chris Larson - Oct. 29, 2011, 12:56 a.m.
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).
Joshua Lock - Oct. 31, 2011, 5:09 p.m.
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

Patch

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
+
 class Konsole(XTerminal):
     command = 'konsole -T "{title}" -e {command}'
     priority = 2