Code review comment for lp:~ken-vandine/indicator-me/libgwibber-port

Revision history for this message
David Barth (dbarth) wrote :

Sounds good to me and works well on my test build. There are a few changes required though:

Line 343: the test for priv->send_enabled should probably be removed, ie toggling the boolean to true on each loop iteration; what matters is the end result

Line 350: the STATUS_RUNNING flag should be set in all cases, as a reply from gwibber implies that it is available for sending messages; the state name is a bit misleading though, it should be more about gwibber being "available" as a gateway service, but I chose running to make the distinction with the "availability" status of other online/chat services

Line 545: not sure about the void if (!exists), i don't think there is anything particular to do in this case either

Line 625: has_configured_accounts should return a positive value even if the service is temporarily down, ie a send would restart it anyway; i found it difficult to understand as a user that the entry is "sometimes" not there, for no apparent reason; it's better to have it always there, whatever the state of the service; the visibility criteria is only that gwibber is "configured".

review: Needs Fixing

« Back to merge proposal