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

Jump to bottom

Update endpoints for blurhash implementation. #650

Merged
merged 4 commits into from Jul 4, 2021
Merged

Update endpoints for blurhash implementation. #650

merged 4 commits into from Jul 4, 2021

Conversation

Frinksy
Copy link
Contributor

@Frinksy Frinksy commented Jun 26, 2021

Add blurhash to profile and avatar endpoints.

Resolves: #450

@Frinksy Frinksy requested a review from jplatte as a code owner June 26, 2021 16:17
jplatte
jplatte reviewed Jun 29, 2021
View reviewed changes
Copy link
Member

@jplatte jplatte left a comment

Choose a reason for hiding this comment

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

Comments on get_avatar_url apply to the other two modified modules as well.

crates/ruma-client-api/src/r0/profile/get_avatar_url.rs Show resolved Hide resolved
crates/ruma-client-api/src/r0/profile/get_avatar_url.rs Outdated Show resolved Hide resolved
crates/ruma-client-api/src/r0/profile/set_avatar_url.rs Outdated Show resolved Hide resolved
@jplatte jplatte changed the base branch from next to main June 29, 2021 21:47
@jplatte
Copy link
Member

jplatte commented Jun 29, 2021 •
edited

Changed the base branch to main since this can (and should) be done in a backwards-compatible manner. You can git rebase -i main your branch and remove the unrelated commits as part of that to synchronize this PR to the main branch.

iinuwa
iinuwa requested changes Jul 1, 2021
View reviewed changes
Copy link
Member

@iinuwa iinuwa left a comment

Choose a reason for hiding this comment

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

(Sorry for the late response.)

It looks like the changes to GET /_matrix/federation/v1/query/profile are missing. The previous implementation also only implemented the blurhash response field for PUT /_matrix/media/r0/upload, but not the generate_blurhash request query parameter.

The types are also missing for the m.sticker and m.room.member events.

@Frinksy
Copy link
Contributor Author

Frinksy commented Jul 2, 2021

The types are also missing for the m.sticker and m.room.member events.

I've added the missing field for m.room.member, but I think m.sticker has already been dealt with. m.sticker uses ruma_events::room::ImageInfo, which already has a blurhash field.

@jplatte jplatte self-assigned this Jul 2, 2021
DevinR528
DevinR528 approved these changes Jul 4, 2021
View reviewed changes
Copy link
Member

@DevinR528 DevinR528 left a comment

Choose a reason for hiding this comment

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

Everything looks good to me (as long as the other review is done, which it looks like you got it all)! Good catch with the gated fields that was a tricky one.

crates/ruma-client-api/src/r0/profile/get_avatar_url.rs Show resolved Hide resolved
iinuwa
iinuwa approved these changes Jul 4, 2021
View reviewed changes
Copy link
Member

@iinuwa iinuwa left a comment

Choose a reason for hiding this comment

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

I think m.sticker has already been dealt with. m.sticker uses ruma_events::room::ImageInfo, which already has a blurhash field.

Ah, yes. My bad.

This looks good to me! I haven't looked at the #[cfg] attribute issue in depth, but I assume we'll need to wait for #659 to be merged before merging this one.

@jplatte
Copy link
Member

jplatte commented Jul 4, 2021

#659 has been merged and CI adjusted to also check most things without unstable-pre-spec. This just needs a rebase now.

@Frinksy
Update endpoints for blurhash implementation.
b2f145e 
Add blurhash to profile and avatar endpoints.
@Frinksy
Feature-gate blurhash and remove from constructor.
6760d78
@Frinksy
Add more blurhash fields.
57044be 
Add `blurhash` fields to `GET /_matrix/federation/v1/query/profile`
and `m.room.member`.
Add `generate_blurhash` field to `PUT /_matrix/media/r0/upload`.
@Frinksy
No longer borrow a bool and fix formatting.
a0e455c
@jplatte jplatte merged commit 118ea0f into ruma:main Jul 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jplatte jplatte jplatte left review comments

@iinuwa iinuwa iinuwa approved these changes

@DevinR528 DevinR528 DevinR528 approved these changes

Assignees

@jplatte jplatte

Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Update / finish blurhash implementation
4 participants
@Frinksy @jplatte @iinuwa @DevinR528