Created attachment 77335 [details] [review]
proposed patch

add a get_avatar_requirements() function in GossipProtocol and use it in gossip-image-chooser.

Hmm.. didn't look at the patch but what does this mean in reality?

If it is that we ask for a size closest to 96x96 and then scale ourselves I'm fine but we don't want to show avatars of different sizes in the UI.

Micke, the point here is that some protocols, like Jabber say that you must provide an image (png or jpg for example) within particular requirements, this is from the XEP-0084 which we abide by:

Certain restrictions are placed upon the image. First, the image height and width SHOULD be between thirty-two (32) and ninety-six (96) pixels. The suggested size is sixty-four (64) pixels high and sixty-four (64) pixels wide. Images SHOULD be square, but this is not required. Finally, images SHOULD use less than eight (8) kilobytes of data.

This API should allows the UI to query the backend what size the image restrictions we need to worry about.

About scaling ourselves, the backend and libgossip doesn't know anything about GdkPixbuf, it simply is passed guchar* data. So any scaling is done in the UI.

Ah, I think I misunderstood this as when receiving avatar images, not sending them.

Sorry for the misunderstanding and please ignore my comment, thanks for clearifying.

I didn't know about XEP-0084 so I updated jabber_get_avatar_requirements() with the exact numbers.

Ok to commit ?

Xavier, some comments before you commit:


1. Please don't do this:
+	g_return_if_fail (min_width > 0 && min_height > 0 &&
+			  max_width > 0 && max_height > 0 &&
+			  max_size  > 0 && format != NULL);

The problem is, if just ONE of those variables is wrong, then when WE have to debug it, it could be ANY number of them that are the problem. Instead it would be better if you did a check for each variable.

Also I would add additional checks like:

        g_return_if_fail (max_width < min_width)
        g_return_if_fail (max_height < min_height)

2. The same with:
+	g_return_if_fail (min_width != NULL || min_height != NULL ||
+			  max_width != NULL || max_height != NULL ||
+			  max_size  != NULL || format != NULL);

3. I would have done gossip_image_chooser_get_requirements() differently. I would have made it possible to request any parameters you want, and it is generally accepted in GNOME libraries that you can pass NULL in for some variable and not for others. That way if you are only interested in the minimum width and maximum width, you can pass valid pointers for those, and NULL pointers for the others. I would prefer we did that here than make sure they are ALL set (as in #2.)

4. We definitely should NULL check in jabber_get_avatar_requirements() since you should be able to request some values (as mentioned in #3) from the backend without having to supply all of them.

5. In gossip_image_chooser_set_requirements() we can't set the minimum_* or maximum_* to 0 or -1 here, this might be beneficial for when we don't have a maximum or minimum.

6. No need to check for NULL pointers before using g_free(), since g_free() already does that:
+	if (priv->image_format) {


Other than that, great patch Xavier, you can commit when these issues are resolved ;)

If I have come across a little strong, I apologise, I am just being pedantic :)

(In reply to comment #6)
> If I have come across a little strong, I apologise, I am just being pedantic :)

No problem, you were right. I fixed issues you pointed and committed.