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.

Sign up for GitHub

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

Support rendering some media downloads as inline #1360

Closed
wants to merge 7 commits into from
Closed

Support rendering some media downloads as inline #1360

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
21989c8
Try to support inline and attachment behaviours
Half-Shot Jul 24, 2023
231d43d
Add a test for inline behaviour
Half-Shot Jul 24, 2023
b4ef274
Remove unused sub
Half-Shot Jul 24, 2023
4866459
Fix docstring
Half-Shot Jul 24, 2023
79d93f3
Merge branch 'develop' into hs/add-media-inline-support
Half-Shot Aug 8, 2023
c0e7b46
Fix tests
Half-Shot Aug 8, 2023
a7823d5
Remove eq
Half-Shot Aug 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 19 additions & 8 deletions tests/51media/01unicode.pl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@
=head2 upload_test_content

my ( $content_id, $content_uri ) = upload_test_content(
$user, filename => "filename",
$user, filename => "filename", $content_type
Copy link

Choose a reason for hiding this comment

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

Not a fan of mixing positionals and named arguments like that.

We could make it another param, and skip it when using %params in its entirety, like this:

my ($user, %params) = @_;
my $content_type = delete $params{content_type} //= "application/octet-stream";

This is using the defined-or construct introduced in Perl 5.10 – which is over 15 years old, though it is tradition that if the version is not declared anywhere, we should assume 5.8 featureset. I see a bunch of use 5.010; elsewhere in the codebase though, so we should be in the clear here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can't modify delete in defined or assignment (//=) at tests/51media/01unicode.pl line 61, at EOF :(

Copy link

Choose a reason for hiding this comment

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

Oh yes, I got ahead of myself there. That should be just //, not //=. We already have the assignment in that line :)

) -> get;

Uploads some test content with the given filename.
Expand All @@ -55,13 +55,15 @@ =head2 upload_test_content
=cut
sub upload_test_content
{
my ( $user, %params ) = @_;
my ( $user, %params, $content_type) = @_;

$content_type ||= "application/octet-stream"

$user->http->do_request(
method => "POST",
full_uri => "/_matrix/media/v3/upload",
content => "Test media file",
content_type => "text/plain",
content_type => $content_type,

params => {
access_token => $user->access_token,
Expand All @@ -88,14 +90,16 @@ sub upload_test_content

=head2 get_media

my ( $content_disposition_params, $content ) = get_media( $http, $content_id ) -> get;
my ( $content_disposition_params, $content ) = get_media( $http, $content_id, $allow_inline_disposition ) -> get;

Fetches a piece of media from the server.

=cut
sub get_media
{
my ( $http, $content_id ) = @_;
my ( $http, $content_id, $allow_inline_disposition ) = @_;

$allow_inline_disposition ||= 0;

$http->do_request(
method => "GET",
Expand All @@ -107,15 +111,15 @@ sub get_media

my $cd_params;
if ( defined $disposition ) {
$cd_params = parse_content_disposition_params( $disposition );
$cd_params = parse_content_disposition_params( $disposition, $allow_inline_disposition );
}
Future->done( $cd_params, $body );
});
}
push @EXPORT, qw( get_media );

sub parse_content_disposition_params {
my ( $disposition ) = @_;
my ( $disposition, $allow_inline_disposition ) = @_;
Copy link

Choose a reason for hiding this comment

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

I'd consider making this a named argument to make it more readable on the caller side (more than a plain 1 in the argument list).

my @parts = split_header_words( $disposition );

# should be only one list of words
Expand All @@ -125,7 +129,14 @@ sub parse_content_disposition_params {
# the first part must be 'attachment'
my $k = shift @parts;
my $v = shift @parts;
assert_eq( $k, "attachment", "content-disposition" );

if ($allow_inline_disposition eq 1) {
Copy link

Choose a reason for hiding this comment

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

eq is for string comparison, which is works here but it needlessly explicit and out of place.

Suggested change
if ($allow_inline_disposition eq 1) {
if ($allow_inline_disposition) {

assert_ok( $k eq "attachment" or $k eq "inline", "content-disposition");
}
else {
assert_eq( $k, "attachment", "content-disposition" );
}

die "invalid CD" if defined $v;

my %params;
Expand Down
4 changes: 2 additions & 2 deletions tests/51media/02nofilename.pl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
do => sub {
my ( $user ) = @_;

upload_test_content( $user, )->then( sub {
upload_test_content( $user )->then( sub {
( $content_id ) = @_;
Future->done(1)
});
Expand All @@ -19,7 +19,7 @@ sub test_using_client
{
my ( $client ) = @_;

get_media( $client, $content_id )->then( sub {
get_media( $client, $content_id, 0 )->then( sub {
my ( $disposition ) = @_;

# Require `attachment` `Content-Disposition` without a filename portion.
Expand Down
17 changes: 17 additions & 0 deletions tests/51media/04inline.pl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
my $content_id;

# This test ensures an uploaded file may optionally be rendered inline
# in the browser. This is checked in get_media

test "Can download a file that optionally inlines",
requires => [ $main::API_CLIENTS[0] ],

check => sub {
my ( $http ) = @_;
upload_test_content( $user, {} , "text/plain")->then( sub {
( $content_id ) = @_;
get_media( $http, $content_id, 1 )->then( sub {
Future->done(1);
})
});
};