Created attachment 67222 [details] [review]
Patch to updated Galago stuff in Util, and use it in beagle-search

Heres a patch that checks for galago-sharp 0.5 or above, and uses those bindings to display status.

Step in the right direction.

The actual string stuff should probably happen solely inside beagle-search so we can localize the messages.  That would require some tweaking of Util/Galago.cs.

I would change "Presence" in the UI to "Status".

You should be able to send an IM to the user from the right-click menu.

Comments should have a space between the comment marker and the comment itself.  Ie, "// Blah blah", not "//Blah blah".  I just added that bit to the HACKING file.

Created attachment 67558 [details] [review]
Galago update

Ok, should be translatable now, I wanted to keep the galago dependency in Util, so I took a kinda-hacky approach to internationalizing the term 'Idle' everything else should be handled in Galago.

This also adds some configure magic to keep the build sane for users who have galago-sharp pre-0.5 (which this will not build against).

I had some trouble getting this patch made in general (just the actual diffing part) so if theres a problem applying it, let me know.

+			string str = Beagle.Util.GalagoTools.GetPresence (Hit.GetFirstProperty ("fixme:protocol"), Hit.GetFirstProperty ("fixme:speakingto"));
+			if ( str. StartsWith ("Idle"))
+				str.Replace ("Idle",Catalog.GetString ("Idle"));
+			return str;

This really needs to be fixed in the API for GalagoUtils.  Doing this here is just a hack.

Unfortunately, I'm not really sure what to do, I don't want to make the translations a dependency of any of the Util stuff. And while this is quite hacky, I'm not quite sure what I would do for an elegant way, any thoughts?

You could use an enum for storing the presence status (enum Status { Online = 0, Away = 1, ...}) and then doing all the string related stuff on the UI front. I think that should work just fine.

Yeah, I agree with Lukas here.

Ok, I was talking to someone on the IRC a few days ago, and he said he wanted to finish this patch up, if I can't get in touch with him again, then I'll polish this off on my own later this week.

Created attachment 70807 [details] [review]
Update/Revive Galago Support in Beagle-search

Ok, I used an enum to pass the status info back and forth, all of it is translatable, and happy. Let me know what you all think.

Some comments and questions since I'm not familiar with Galago:

Initialize is misspelled.

Is there a reason why we are initializing and uninitializing Galago all over the place instead of just doing it once if it hasn't been done before?

What is the point of the Initialize() function?  It doesn't do any action based on the return value of Galago.Global.Init() and is just one line, so why not call it directly?

Apparently Galago.Global.Init() can fail.  What happens if it fails and you try to run a galago function afterward anyway?

Instead of using presence.ActiveStatus.Name to get a string and then parsing a string, you should probably use presense.ActiveStatus.Primitive and then swtich on the enum value.  In fact, can we just use the GalagoStatusType for everything instead of defining our own Status enum?

For the idle time, we're still returning the string "Idle".  We shouldn't return any hardcoded string at all from there.  Just returning the time itself should be fine, and we can add the "Idle" string in the UI.

As a result of the last point, I would refactor GetBuddyStatus() in the tile to get the string for the enum in any case, and then tack on an idle time if applicable.

(In reply to comment #10)
> Some comments and questions since I'm not familiar with Galago:
> 
> Initialize is misspelled.

Oops *blush* fixed.
> 
> Is there a reason why we are initializing and uninitializing Galago all over
> the place instead of just doing it once if it hasn't been done before?
> 
Its difficult to maintain a galago connection, there were too many times where the interface would get closed with an open connection causing the galago daemon to crash occasionally, I hope that in the future it won't and we just initialize one connection, but when I tried to just do one static one, it didn't work.

> What is the point of the Initialize() function?  It doesn't do any action based
> on the return value of Galago.Global.Init() and is just one line, so why not
> call it directly?
>
Done. 
> Apparently Galago.Global.Init() can fail.  What happens if it fails and you try
> to run a galago function afterward anyway?

We don't run them anymore, if initialization fails, then nothing is displayed.
> 
> Instead of using presence.ActiveStatus.Name to get a string and then parsing a
> string, you should probably use presence.ActiveStatus.Primitive and then switch
> on the enum value.  In fact, can we just use the GalagoStatusType for
> everything instead of defining our own Status enum?
No, Galago has an odd way of handling idle times. Because a user can have any status as well as being idle, it is handled separately, for our purposes, this is an unnecessary level of complexity to the code, and a separate dbus query which can fail. To me, it seems easier to keep just one enum which contains all of the status' which we handle. 

However, I did switch to using the Primitive enum to switch inside Galago.cs
> 
> For the idle time, we're still returning the string "Idle".  We shouldn't
> return any hardcoded string at all from there.  Just returning the time itself
> should be fine, and we can add the "Idle" string in the UI.
> 

Fixed
> As a result of the last point, I would refactor GetBuddyStatus() in the tile to
> get the string for the enum in any case, and then tack on an idle time if
> applicable.

Passing the galago connection information around can be difficult, I tried this but ran into a lot of crashes and bugs, if you are really set on it, I can keep working, but it seems like the current API is good for what we need.
> 

Created attachment 72106 [details] [review]
Update mentioned in above comment

Let me know if the patch doesn't apply cleanly. This should do what I mentioned in the comment above, making it more or less ready for widespread testing.

Looks good to go in, thanks.

Done and done.