Merge lp:~sunweaver/unity-greeter/x2go into lp:unity-greeter

[Mike Gabriel]

Add X2Go session support to Unity Greeter's remote login feature.

[Robert Ancell (robert-ancell)]

Code looks good to me, David - if you approve just set to approved and it should autoland.

review: Approve

[David Barth (dbarth)]

Hi,

I wrote back to Mike a few months to explain why this change is not necessary. I think it is easier to branch off what the RDP session does. See http://bazaar.launchpad.net/~freerdp-remote-team/lightdm-remote-session-freerdp/trunk/view/head:/freerdp.desktop.in

Sorry to 'disapprove' (that sounds too harsh), but that change is not needed in unity-greeter. Let's keep it simple in the main component, and make special cases in the lightdm-remote-session-* packages.

[David Barth (dbarth)]

Mike, can you suppress this merge proposal also please? It just doesn't feel right for me to do so.

[Mike Gabriel (sunweaver)]

> Hi,
>
> I wrote back to Mike a few months to explain why this change is not necessary.
> I think it is easier to branch off what the RDP session does. See
> http://bazaar.launchpad.net/~freerdp-remote-team/lightdm-remote-session-
> freerdp/trunk/view/head:/freerdp.desktop.in

That's what we actually do:
http://code.x2go.org/gitweb?p=lightdm-remote-session-x2go.git;a=blob;f=x2go.desktop.in

The point is that for X2Go Sessions we need to set an option that FreeRDP sessions do not have: the desktop shell to run with X2Go (GNOME, KDE, MATE, LXDE, ...). So the Unity Greeter looks much nicer if it knows how to query/show this piece of information (i.e. the session type).

> Sorry to 'disapprove' (that sounds too harsh), but that change is not needed
> in unity-greeter. Let's keep it simple in the main component, and make special
> cases in the lightdm-remote-session-* packages.

That I already have done. And in order to get it working nicely in Unity Greeter, it currently requires this patch. It would be much nicer if Unity Greeter would retrieve its UI information (what fields to show on login) through the remote-login-service itself. Are there any plans for that?

Mike

[Mike Gabriel (sunweaver)]

> Mike, can you suppress this merge proposal also please? It just doesn't feel
> right for me to do so.

I'd prefer you disapproving this merge proposal as it is more authentic and mirrors better the decisioning workflow.

Greets+Thanks!
Mike

[David Barth (dbarth)]

Le 30/07/2013 17:19, Mike Gabriel a écrit :
> The point is that for X2Go Sessions we need to set an option that FreeRDP sessions do not have: the desktop shell to run with X2Go (GNOME, KDE, MATE, LXDE, ...). So the Unity Greeter looks much nicer if it knows how to query/show this piece of information (i.e. the session type).
To clarify, the need to present the session option is for users who do
not want to use the /default/ remote session set by the remote desktop,
right?

As a rare use case, this feels similar to the local session option which
is set via the small Ubuntu icon at the top right of an entry.

So I would propose 2 changes:

1. To rename the session option to 'remote_session': it may not be x2go
specific.
2. Move the option to a session menu specific to the remote login entry,
ie not a prompt

Also, a question: how do you query the remote server for available
session types? Is there a mechanism to enforce a particular session, and
thus not offer the session choice to the user if it is locked down?

David

[Mike Gabriel (sunweaver)]

> Le 30/07/2013 17:19, Mike Gabriel a écrit :
> > The point is that for X2Go Sessions we need to set an option that FreeRDP
> sessions do not have: the desktop shell to run with X2Go (GNOME, KDE, MATE,
> LXDE, ...). So the Unity Greeter looks much nicer if it knows how to
> query/show this piece of information (i.e. the session type).
> To clarify, the need to present the session option is for users who do
> not want to use the /default/ remote session set by the remote desktop,
> right?
>
> As a rare use case, this feels similar to the local session option which
> is set via the small Ubuntu icon at the top right of an entry.

Ok. I will test that. The icon has stayed hidden to my eye so far (I did not realize that it is clickable.

> So I would propose 2 changes:
>
> 1. To rename the session option to 'remote_session': it may not be x2go
> specific.

Ack. Agreed.

> 2. Move the option to a session menu specific to the remote login entry,
> ie not a prompt

Sounds sensible. Will take a look at the latest during DebConf13. Loads of other tasks before that.

> Also, a question: how do you query the remote server for available
> session types? Is there a mechanism to enforce a particular session, and
> thus not offer the session choice to the user if it is locked down?

This one is tricky. As you might have noticed, the X2Go Session Broker has become able to behave like an UCCS server. The X2Go Session Broker is able to query associated X2Go Terminal Servers for several kinds of information. At the moment, there is no way of querying a list of available session types, but this is surely something to be put on our wishlist. Which I will do in a minute.

David, thanks for taking your time on this!!!
Mike

[Mike Gabriel (sunweaver)]

these are the X2Go issues that ask for detection of available desktop sessions on an X2Go Server and making those queryable from an X2Go Session Broker.

http://bugs.x2go.org/cgi-bin/bugreport.cgi?bug=279
http://bugs.x2go.org/cgi-bin/bugreport.cgi?bug=280

I understand that on uccs.landscape.canonical.com you do not have the means to query the remote servers directly. So for that use case of the ,,UCCS protocol'' it needs some other mechanism.

So the generic case would be some after-login-mechanism that let's the login fail and an error message that gets reported back to the Unity Greeter (or so...).

Greets,
Mike

[Robert Ancell (robert-ancell)]

I talked to David the other day and he said that he'd discussed this with you and you'd decided that this was no longer required - is that correct?

[Mike Gabriel (sunweaver)]

Hi Robert,

thanks for getting back to this one.

This is not quite exact. I still think that this merge should get into unity-greeter (or a similar patchset that provides X2Go remote login in Unity Greeter). I am also willing to provide X2Go patches against the Unity Greeter NG.

However, I did not get a proper signal from Canonical / Ubuntu devs, that this is something really wanted. Of course, I hesitate working for the paper bin. My impression has been that X2Go remote login support is not on the list for Unity Greeter, so I stopped spending time on this.

Also: isn't Unity Greeter deprecated for 14.04? Is there a future for this code base? This also stopped me from working on this.

So, I'd like to see this code being added to Unity Greeter (or at least the X2Go remote login functionality). What do you need from me to make this happen?

light+love
Mike

review: Needs Information

[David Barth (dbarth)]

Hey Mike,

I still feel that this patch should be updated like mentioned above, ie provide a generic remote linux session mechanism on top of which the x2go goodness is built.

We're heading for an LTS, so it's better if we can stay lean and simple, particularly wrt to maintenance.

I would say the same for the related remote-login patch, ie provide a generic mechanism as well. Or, alternatively, make the feature a build time option. This way we can put aside the associated maintenance overhead.

[Mike Gabriel (sunweaver)]

Hi David,

ok. I will take a look at that during next week then. So, this merge request can be closed, I will open a new one...

Greets,
Mike

Unmerged revisions

827. By Mike Gabriel on 2013-04-25

Add support for X2Go

=== modified file 'src/user-list.vala'
--- src/user-list.vala	2013-02-13 04:25:47 +0000
+++ src/user-list.vala	2013-04-25 23:37:26 +0000
@@ -20,7 +20,7 @@
 
 int remote_server_field_sort_function (RemoteServerField? item1, RemoteServerField? item2)
 {
-    string[] sorted_fields = { "domain", "username", "email", "password" };
+    string[] sorted_fields = { "domain", "x2gosession" , "username", "email", "password"};
     foreach (var field in sorted_fields)
     {
         if (item1.type == field)
@@ -651,6 +651,13 @@
                         entry.text = default_value;
                         widget = entry;
                     }
+                    else if (field.type == "x2gosession")
+                    {
+                        var prompt = add_prompt (_("X2Go Session:"));
+                        prompt.text = default_value;
+                        prompt.sensitive = true;
+                        widget = prompt;
+                    }
                     else if (field.type == "domain")
                     {
                         string[] domainsArray = {};
@@ -1025,6 +1032,12 @@
                 var answer = field != null ? field.text : "";
                 UnityGreeter.singleton.respond (answer);
             }
+            else if (text == "x2gosession:")
+            {
+                Gtk.Entry field = current_remote_fields.get ("x2gosession") as Gtk.Entry;
+                var answer = field != null ? field.text : "";
+                UnityGreeter.singleton.respond (answer);
+            }
         }
         else
             base.show_prompt_cb (text, type);