Merge lp:~sdague/do-plugins/sean into lp:do-plugins/community

[Sean Dague]

[Sean Dague (sdague)]

Please consider merging this support for launching NX sessions with Gnome Do

[Alex Launi (alexlauni)]

First, thanks for using the new API with IEnumerable<T>, that saves me some work later :).
A couple off issues with the plugin, first off I don't like the namespace. Can we do Nx or NX instead? I know some others use Do. or GnomeDo, but I don't like that and I'll be fixing them as I do a major plugin code cleanup. Next, the hardcoded path to nxclient is not good, first off you appear to have a nonstandard path, most people would have /usr/bin/nxclient, but the actual path is irrelevant, it should not be hardcoded. Is there any reason you can't just do string exec = "nxbuild"; The environment should be able to find the path. For finding the config dir, you should use the Paths class provided by Do. Use Path.Combine (from System.IO) to merge paths, and there is a Paths (from Do.Addins I think) method for getting the home directory.

review: Needs Fixing (none)

[Sean Dague (sdague)]

Alex Launi wrote:
> Vote: Needs Fixing none
> First, thanks for using the new API with IEnumerable<T>, that saves me some work later :).
> A couple off issues with the plugin, first off I don't like the namespace. Can we do Nx or NX instead? I know some others use Do. or GnomeDo, but I don't like that and I'll be fixing them as I do a major plugin code cleanup.

Not a problem, I can change the namespace easily

> Next, the hardcoded path to nxclient is not good, first off you appear to have a nonstandard path, most people would have /usr/bin/nxclient, but the actual path is irrelevant, it should not be hardcoded.

The nomachine NX packages for debian/ubuntu put it in that location, and
do not add it to the path. You have to manually launch it from that
path, or use the desktop launching files in the menu.

> Is there any reason you can't just do string exec = "nxbuild"; The environment should be able to find the path.

If you just install the .deb files from nomachine.com, you won't have
that in your path. I really think leaving the hardcode in there is
better as it will work out of the box if you install the debian/ubuntu
packages from nomachine (I assume that is true with the rpms as well)

> For finding the config dir, you should use the Paths class provided by Do. Use Path.Combine (from System.IO) to merge paths, and there is a Paths (from Do.Addins I think) method for getting the home directory.

Ok, that's an easy enough change. I'll do that this weekend.

 -Sean

--
__________________________________________________________________

Sean Dague Mid-Hudson Valley
sean at dague dot net Linux Users Group
http://dague.net http://mhvlug.org

There is no silver bullet. Plus, werewolves make better neighbors
than zombies, and they tend to keep the vampire population down.
__________________________________________________________________

[Alex Launi (alexlauni)]

> The nomachine NX packages for debian/ubuntu put it in that location, and
> do not add it to the path. You have to manually launch it from that
> path, or use the desktop launching files in the menu.

hardcoding is just bad all around, you should file a bug with that package because that's not where it's supposed to go. The plugin should look on $PATH, if a user has a nonstandard path (either because they did it themself, or weird packages), then they can add that path to $PATH.

[Sean Dague (sdague)]

Alex Launi wrote:
>> The nomachine NX packages for debian/ubuntu put it in that location, and
>> do not add it to the path. You have to manually launch it from that
>> path, or use the desktop launching files in the menu.
>
> hardcoding is just bad all around, you should file a bug with that package because that's not where it's supposed to go. The plugin should look on $PATH, if a user has a nonstandard path (either because they did it themself, or weird packages), then they can add that path to $PATH.

It's a commercial (free beer) package (http://www.nomachine.com/), not
something that is changable in the distro. While it is probably bad, it
is consistant, and has been in the same path location for the last 2
years that I've been using it.

The question is basically whether the plugin works out of the box or
requires the user to change his/her PATH to include the nx location
prior to it working.

 -Sean

--
__________________________________________________________________

Sean Dague Mid-Hudson Valley
sean at dague dot net Linux Users Group
http://dague.net http://mhvlug.org

There is no silver bullet. Plus, werewolves make better neighbors
than zombies, and they tend to keep the vampire population down.
__________________________________________________________________

[lp:~sdague/do-plugins/sean]

57. By Sean Dague on 2008-12-14

changed namespace to NX per merge feedback
find home directory with Paths.UserHome per merge feedback
use Path.Combine to safely build paths
append /usr/NX/bin to Path and exec nxclient out of path instead of hard coding location

[Sean Dague (sdague)]

Alex Launi wrote:
>> The nomachine NX packages for debian/ubuntu put it in that location, and
>> do not add it to the path. You have to manually launch it from that
>> path, or use the desktop launching files in the menu.
>
> hardcoding is just bad all around, you should file a bug with that package because that's not where it's supposed to go. The plugin should look on $PATH, if a user has a nonstandard path (either because they did it themself, or weird packages), then they can add that path to $PATH.

I've made updates to the branch, please let me know how it looks.

I changed the namespace to NX

I'm now using Paths.UserHome instead of getting the environment for Home

I'm now using Path.Combine to build up paths safely.

To get around the hardcoded path issue I'm appending the odd path for NX
(/usr/NX/bin) to the path environment. This lets you call nxclient
without hardcoding it's location. It's still less than ideal, but
hopefully better.

 -Sean

--
__________________________________________________________________

Sean Dague Mid-Hudson Valley
sean at dague dot net Linux Users Group
http://dague.net http://mhvlug.org

There is no silver bullet. Plus, werewolves make better neighbors
than zombies, and they tend to keep the vampire population down.
__________________________________________________________________

[Alex Launi (alexlauni)]

It looks much better, here are my comments this time
NXHosts.cs:55 Do not call UpdateItems yourself, we call that for you.

NXHosts.cs:77; you make two calls to Path.Combine, this is ok, but we can use some C# 3.0 sexiness to make this cleaner.
string nxDir = new [] {Paths.UserHome, ".nx", "config"}.Aggregate (Path.Combine);
Hotness, right?

You've also got an empty catch at line 85, Print a Log.Error, something is obviously going wrong if it's throwing an exception, errors should never pass silently. What here was throwing an exception? Is there even a need for that try/catch block?

NXAction.cs:66; You should check if /usr/NX/bin is on the path already, it looks like you'll be appending it every time Perform runs.

NXAction.cs:86 I think with the new API returning an empty IEnumerable is preferred to returning null.
return Enumerable.Empty<IItem> ();

Looks like you do that Path.Combine to get the NX config dir more than once, can you make a static string in NXHostItem (or somewhere you think makes sense) so you're not duplicating this? One of the most important things Dave has taught me is that any time you write the same piece of code twice you should be very concerned.

review: Needs Fixing (none)

[Sean Dague (sdague)]

> NXHosts.cs:55 Do not call UpdateItems yourself, we call that for you.

Fixed

> NXHosts.cs:77; you make two calls to Path.Combine, this is ok, but we can use some C# 3.0 sexiness to > make this cleaner.
> string nxDir = new [] {Paths.UserHome, ".nx", "config"}.Aggregate (Path.Combine);
> Hotness, right?

I suppose, except it doesn't compile under monodevelop under ubuntu 8.10

> You've also got an empty catch at line 85, Print a Log.Error, something is obviously going wrong if
> it's throwing an exception, errors should never pass silently. What here was throwing an exception?
> Is there even a need for that try/catch block?

That comes from the SSH plugin in the core directory. I don't know if it is needed or not, but if it should be changed, it should be changed there as well. I'll remove it, hoping nothing goes wrong in the process.

> NXAction.cs:66; You should check if /usr/NX/bin is on the path already, it looks like you'll be
> appending it every time Perform runs.

Fixed.

> NXAction.cs:86 I think with the new API returning an empty IEnumerable is preferred to returning
> null.
> return Enumerable.Empty<IItem> ();

Fixed.

> Looks like you do that Path.Combine to get the NX config dir more than once, can you make a static > string in NXHostItem (or somewhere you think makes sense) so you're not duplicating this? One of > the most important things Dave has taught me is that any time you write the same piece of code twice you should be very concerned.

I've now moved the path into the NXHost object, that removes the duplication.

[lp:~sdague/do-plugins/sean]

58. By Sean Dague on 2008-12-28

fixed a number of issues found with most recent review. Hopefully it's good for merge now.

[Sean Dague (sdague)]

Just a quick ping to see if anyone has had a chance to look at this again. I'd love to get this into the next release. Please let me know once you get a chance.