Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Receive images #21

Merged
merged 4 commits into from
Nov 20, 2016
Merged

Receive images #21

merged 4 commits into from
Nov 20, 2016

Conversation

penguin42
Copy link
Contributor

This is at the 'seems to work' level; I get the feeling it could probably do with a bit more testing and perhaps a bit more thought on things like what gets free'd where; but have a look.

I added an extra /join in the upload url - synapse
seems to ignore it but we'd better remove it!

Signed-off-by: Dr. David Alan Gilbert <[email protected]>
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a few minor things, otherwise looks good!

@@ -105,6 +105,8 @@ typedef struct {
gchar *content_type;
gboolean got_headers;
JsonParser *json_parser;
const char *body;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to initialise these in _response_parser_data_new

@@ -444,6 +450,17 @@ static const char *type_guess(PurpleStoredImage *image)
}
}

/**
* Check if the declared image type is an image type we recongise.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/image type/content-type/

/**
* Check if the declared image type is an image type we recongise.
*/
static gboolean check_image_type(const char *content_type)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: i'd prefer a less ambiguous name here: is_image_type or something

PURPLE_MESSAGE_RECV | PURPLE_MESSAGE_IMAGES,
g_strdup_printf("<IMG ID=\"%d\">", img_id), rid->timestamp / 1000);
} else {
serv_got_chat_in(rid->conv->account->gc, g_str_hash(rid->room_id),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it would be nice to show the fallback (event.body) here?

ditto in _image_download_bad_response and _image_download_error

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I think I've fixed that and the other comments; and pushed to this same branch - do you see that?

When a response can't be parsed as JSON pass the body and length
to the callback.
Also add the content_type.

Signed-off-by: Dr. David Alan Gilbert <[email protected]>
Download a file given a URI.

Signed-off-by: Dr. David Alan Gilbert <[email protected]>
Download images received in events, adding them to Purple's image
cache.

There are probably improvements that can be made in failure paths
and also in trying to use thumbnails.

Signed-off-by: Dr. David Alan Gilbert <[email protected]>
@richvdh
Copy link
Member

richvdh commented Nov 20, 2016

Sorry for sitting on this. LGTM!

@richvdh
Copy link
Member

richvdh commented Nov 20, 2016

(and thanks!)

@richvdh richvdh merged commit 61ffd59 into matrix-org:master Nov 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants