I'm working on it.

Fixed in http://git.collabora.co.uk/?p=user/cassidy/empathy.git;a=shortlog;h=refs/heads/chatroom-mgr

Created attachment 114257 [details] [review]
diff between master and my branch

Thanks for the patch, some comments:

1) Coding style of course, but you can fix that when the rest is OK.

2) Do we want to keep favorite rooms that are not auto-connect? xchat-gnome only keep auto-connect rooms and forget about others AFAIK. I think a room should be kept in the xml if and only if it is auto-connect. Do you agree?

3) chatroom_invalidated_cb() --> static functions of empathy_foo start with foo_. Also the chatroom should be kept if it is auto-connect? Or auto-connect implies favorite==true?

4) EmpathyChatroomManager is a singleton, if you call empathy_chatroom_new for the first time it gets created and if you unref later it get finalized if nobody else reffed it. I think it's safer if EmpathyDispatcher always keep a ref to the chatroom manager to avoid possible construction/destruction each time a new room channel appear.

5) Prevent chatroom_manager_file_save() to be called if a non-favorite chatroom is added/removed. Also it should be called when a chatroom become favorite or is not favorite anymore. In fact empathy_chatroom_manager_store should be removed and the manager should keep track itself of changes of one of its chatroom and rewrite the xml file in a timeout cb to avoid rewriting too often.

That's all I see for now.

I rebased my branch on top of master and added it to Monkey: http://monkey.collabora.co.uk/empathy.git_chatroom-mgr/

2) No, I think it's worth to be able to have not-autoconnect rooms in the manager. Furthermore, that's already implemented so why remove this feature?

3) I don't really see the point of this namespacing but I renamed it to dispatcher_chatroom_invalidated_cb.
Yes, autoconnect implies favorite.

4) done

I'll fix 5) later

New version of my patch fixing the review comments and adding a nice test framework.

Monkey: http://monkey.collabora.co.uk/empathy.git_chatroom-mgr-rebased/
Branch: http://git.collabora.co.uk/?p=user/cassidy/empathy.git;a=shortlog;h=refs/heads/chatroom-mgr-rebased

Created attachment 119043 [details] [review]
the new diff

Thanks for the patch, some comments:

1) in chatroom-manager.c, add_chatroom(): Instead of connecting each "notify::foo" signals separately you can just connect to "notify". In the callback you should reset timout only if it's a favorite, right?

2) In empathy_chatroom_set_auto_connect(): When set auto-connect to TRUE it changes the favorite property to TRUE. That's fine but g_object_notify() should be called for that property too.

3) In dispatcher_connection_new_channel_cb(): empathy_chatroom_manager_find() does not return a new ref, so you should not unref the chatroom.

4) tests does not compile, you can't ignore the return value of system() calls. I get that error from gcc: check-empathy-chatroom-manager.c:103: error: ignoring return value of ‘system’, declared with attribute warn_unused_result.

5) python binding does not build but I'll update them myself, it it just a matter of running some automated scripts :)

Otherwise it's OK. I didn't review tests but if they all succeed it's fine :)

1) done.
Humm no, because if a chatroom which was a favorite is not one anymore, we have to update the XML (to remove it).

2) Good point. fixed.

3) Actually, I unref it at the end to remove the ref I added when creating the object (or if I didn't create one, I manually ref it). I could avoid that and use a created boolean and unref only if I created the object.

4) humm, I don't have this problem on Gutsy. Should be fixed.

5) Shouldn't we automatically run these scripts when source is changed ?

(In reply to comment #9)
> 1) done.
> Humm no, because if a chatroom which was a favorite is not one anymore, we have
> to update the XML (to remove it).

Ah, right!

> 3) Actually, I unref it at the end to remove the ref I added when creating the
> object (or if I didn't create one, I manually ref it). I could avoid that and
> use a created boolean and unref only if I created the object.

You are right, you call g_object_ref() if chatroom is !NULL, I've read g_object_unref(). My bad :)

> 4) humm, I don't have this problem on Gutsy. Should be fixed.

You fixed only at one place. It have to be changed everywhere. Why don't you use g_spawn_command_line_async() instead of system()?

> 5) Shouldn't we automatically run these scripts when source is changed ?

No, there is still little manual stuff to do, but I should automate all that stuff so the script can be run from Makefile and we can remove all generated files from git/svn.

4) oups sorry fixed.

I pushed 2 more commits in http://git.collabora.co.uk/?p=user/cassidy/empathy.git;a=shortlog;h=refs/heads/chatroom-mgr-rebased
fixing a crasher.

Merged, thanks !