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

Support audio file previews #1806

Merged
merged 1 commit into from Dec 13, 2017

Conversation

Projects
None yet
4 participants
@MaxLeiter
Member

MaxLeiter commented Dec 6, 2017

example

To test you can use https://demo.thelounge.chat/audio/pop.ogg

case "audio/x-mid":
case "audio/x-midi":
case "audio/ogg":
if (res.size > (Helper.config.prefetchMaxImageSize * 4028)) {

This comment has been minimized.

@MaxLeiter

MaxLeiter Dec 6, 2017

Member

need to change this. thoughts as to what it should be?

This comment has been minimized.

@dgw

dgw Dec 6, 2017

Contributor

Why not a new prefetchMaxAudioSize defaulted to 4-8 MB?

Or deprecate the existing setting in favor of a prefetchMaxMediaSize.

@@ -65,6 +66,7 @@ function parse(msg, preview, res, client) {
|| $('meta[name="twitter:image:src"]').attr("content")
|| $('link[rel="image_src"]').attr("href")
|| "";
preview.type = res.type;

This comment has been minimized.

@xPaw

xPaw Dec 6, 2017

Member

unnecessary?

This comment has been minimized.

@MaxLeiter

MaxLeiter Dec 6, 2017

Member

yeah, but may as well populate if it's there?

This comment has been minimized.

@xPaw

xPaw Dec 6, 2017

Member

It's already set to link above.

This comment has been minimized.

@MaxLeiter
case "audio/x-midi":
case "audio/x-mpeg":
case "audio/x-mpeg-3":
if (res.size > (Helper.config.prefetchMaxImageSize * 4028)) {

This comment has been minimized.

@xPaw

xPaw Dec 7, 2017

Member

How do browsers deal with preloading audio data? Does it download the entire audio file as soon as it sees the audio tag or downloads just enough to display the duration?

This comment has been minimized.

@MaxLeiter

MaxLeiter Dec 7, 2017

Member

Browsers should default to fetching metadata, but I can add a ‘preload’ attribute to specify

This comment has been minimized.

@xPaw

xPaw Dec 7, 2017

Member

My point is, if browsers just fetch the metadata, don't really need to limit the size.

You do need to abort the request after content-type was fetched by the way.

This comment has been minimized.

@MaxLeiter

MaxLeiter Dec 7, 2017

Member

MDN says browsers should should default to metadata, and says that the preload attribute acts as a hint to the browser — should we trust the browser?

This comment has been minimized.

@astorije

astorije Dec 7, 2017

Member

MDN mentions:

If not set, its default value is browser-defined (i.e. each browser may have its own default value). The spec advises it to be set to metadata.

I think it's fair to have preload="metadata".

This comment has been minimized.

@xPaw

xPaw Dec 7, 2017

Member

What if we went with preload=none to avoid unnecessary requests? When I tried this PR Chrome didn't display duration half the time anyway.

This comment has been minimized.

@MaxLeiter

MaxLeiter Dec 7, 2017

Member

Time won’t be loaded then, and I think that’s pretty important

This comment has been minimized.

@MaxLeiter

MaxLeiter Dec 7, 2017

Member

added preload="metadata"

@xPaw xPaw added the Type: Feature label Dec 7, 2017

@astorije

This comment has been minimized.

Member

astorije commented Dec 8, 2017

@MaxLeiter, without going all-in with styling as of yet, could you make this use the whole container? Something like this:

screen shot 2017-12-07 at 13 27 25

@xPaw

This comment has been minimized.

Member

xPaw commented Dec 8, 2017

@astorije Don't do that, link previews only take width of their content not parent. It will look rather dumb on any big screen.

@MaxLeiter

This comment has been minimized.

Member

MaxLeiter commented Dec 8, 2017

removed prefetchMaxImageSize test, as it's at the users discretion if the audio file should be downloaded or not. Ready to go, unless I should do something with storage, I think?

@astorije

This comment has been minimized.

Member

astorije commented Dec 9, 2017

Makes sense @xPaw, but at least 1.5 or 2x Chrome's default width (max out at parent width for mobile), it's very small as-is and makes seeking through progress rather hard.

case "audio/x-midi":
case "audio/x-mpeg":
case "audio/x-mpeg-3":
if (!`/^https:\/\//`.test(url)) {

This comment has been minimized.

@xPaw

xPaw Dec 9, 2017

Member

Why do you have string interpolation here?

This could be done without regex: url.startsWith("https://")

@MaxLeiter

This comment has been minimized.

Member

MaxLeiter commented Dec 9, 2017

should be set

@xPaw

xPaw approved these changes Dec 10, 2017

@astorije astorije self-requested a review Dec 10, 2017

@astorije

This comment has been minimized.

Member

astorije commented Dec 10, 2017

@xPaw, @MaxLeiter, what about this?

at least 1.5 or 2x Chrome's default width (max out at parent width for mobile), it's very small as-is and makes seeking through progress rather hard.

@MaxLeiter

This comment has been minimized.

Member

MaxLeiter commented Dec 11, 2017

@astorije bumped the width. default for me was 300px, so just doubled and it seems fine on all screen sizes I tested.

@xPaw xPaw added this to the 2.7.0 milestone Dec 11, 2017

@astorije

This comment has been minimized.

Member

astorije commented Dec 12, 2017

Looks great @MaxLeiter!

I've noticed a couple issues, trying to make sense out of the first one:

@MaxLeiter

This comment has been minimized.

Member

MaxLeiter commented Dec 12, 2017

First link is https, second is not

No clue on second point, will investigate

@astorije

This comment has been minimized.

Member

astorije commented Dec 12, 2017

First link is https, second is not

Ah right, I forgot that we added this since your original commit, thanks!

will investigate

Cool, thanks!

It's very cool you took this btw, nice job 👏

@xPaw

This comment has been minimized.

Member

xPaw commented Dec 12, 2017

It doesn't open by default because of this: https://github.com/thelounge/lounge/blob/d0f5d5025e4c5184ace4d98d250643d37e1d0c92/client/js/options.js#L46-L48

We most likely need to switch image to a more generic media.

@MaxLeiter

This comment has been minimized.

Member

MaxLeiter commented Dec 12, 2017

I think thats part of a larger issue with embedding images/links (maybe it should just be (expand media or something)).

For now I'll just have it embed if options.links?

@MaxLeiter

This comment has been minimized.

Member

MaxLeiter commented Dec 12, 2017

Relies on #1832

@@ -44,7 +44,7 @@ userOptions = null;
module.exports = options;
module.exports.shouldOpenMessagePreview = function(type) {
return (options.links && type === "link") || (options.thumbnails && type === "image");
return (options.links && type === "link") || (options.media && (type === "image" || type === "audio"));

This comment has been minimized.

@astorije

astorije Dec 13, 2017

Member

Off-topic/Not for this PR: @xPaw, @MaxLeiter, what about merging both under options.preview or something?

This comment has been minimized.

@MaxLeiter

MaxLeiter Dec 13, 2017

Member

I think there's a slight difference, in that links will be low-bandwidth, but media can vary. At least, I think that's how @xPaw and I see it at the moment.

This comment has been minimized.

@astorije

astorije Dec 13, 2017

Member

That's fair, thanks!

@astorije

Great stuff as usual :)

@xPaw xPaw merged commit 7dcab09 into thelounge:master Dec 13, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@MaxLeiter MaxLeiter deleted the MaxLeiter:sound branch Dec 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment