-
Notifications
You must be signed in to change notification settings - Fork 414
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
kill render-media (range-requests) and add floating player #2707
Conversation
77f95d3
to
0eb409c
Compare
cb7fb90
to
6aa8768
Compare
This is ready. I'm going to wait to add analytics in a different PR. Just so we can get a little testing on this to see if anything crazy needs to change before I update this new |
src/ui/page/settings/view.jsx
Outdated
'Multi-language support is brand new and incomplete. Switching your language may have unintended consequences.' | ||
)} | ||
label={__('Max Connections')} | ||
helper={__('More connections, like, do stuff dude')} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tzarebczan Can you add some help text for this setting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will add it today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Slightly hastier review than I would have liked, but wanted to get something back to you today. I also did not run the code. I'd still like to do this, but do not let me block.
src/ui/component/fileRender/view.jsx
Outdated
<VideoViewer claim={claim} source={{ downloadPath, fileName }} contentType={contentType} poster={poster} /> | ||
), | ||
audio: <VideoViewer claim={claim} source={{ downloadPath, fileName }} contentType={contentType} />, | ||
video: <VideoViewer source={streamingUrl} contentType={contentType} />, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does the creation of mediaTypes
trigger the instantiation/invocation of the individual components? or is that deferred until the element is used in some way? (I think it's the latter, but making sure)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If download path exists and the file is completed, we should use that for the video and audio player.
@@ -64,7 +63,7 @@ class SelectThumbnail extends React.PureComponent<Props, State> { | |||
|
|||
const { thumbnailError } = this.state; | |||
|
|||
const isSupportedVideo = getMediaType(null, filePath) === 'video'; | |||
const isSupportedVideo = Lbry.getMediaType(null, filePath) === 'video'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
out of scope for this PR, but I wonder if putting all of these on the Lbry
object is the right design
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also have the file name with extension in the claim data now, but we should fall back to file list when it's not available
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since that only has some of the media types, I don't think it's worth it to use for now, especially since it's working pretty well.
And yeah, this is probably better as a regular utility function?
const SANDBOX_TYPES = ['application/x-lbry', 'application/x-ext-lbry']; | ||
|
||
// This server exists in src/platforms/electron/startSandBox.js | ||
const SANDBOX_SET_BASE_URL = 'http://localhost:5278/set/'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are these important enough configuration values that they should be hoisted to top-level config of some kind? does anything else use them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope, they exist entirely for this app viewer
src={appUrl} | ||
autosize="on" | ||
style={{ border: 0, width: '100%', height: '100%' }} | ||
useragent="Mozilla/5.0 AppleWebKit/537 Chrome/60 Safari/537" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can this be read from Electron?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly. I haven't spent any time looking into that, just copy pasting it from the old file
const fileInfo = makeSelectFileInfoForUri(uri)(state); | ||
const uriIsStreamable = makeSelectUriIsStreamable(uri)(state); | ||
const downloadingByOutpoint = selectDownloadingByOutpoint(state); | ||
const alreadyDownloaded = fileInfo && (fileInfo.completed || (fileInfo.blobs_remaining === 0 && uriIsStreamable)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've seen a few fileInfo checks similar to this, consider if it makes sense to combine into a selector (it might not).
const daemonSettings = selectDaemonSettings(state); | ||
const costInfo = makeSelectCostInfoForUri(uri)(state); | ||
const cost = Number(costInfo.cost); | ||
const saveFile = !uriIsStreamable ? true : daemonSettings.save_files || saveFileOverride || cost > 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why always save if cost > 0? I would not expect this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@seanyesmunt I heard the answer to this but I still question it. I say remove.
|
||
if (cost === 0 || skipCostCheck) { | ||
beginGetFile(); | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return is avoidable if made into 3 nested if
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO nested if statements are harder to skim and understand. Especially when one of the if statements does something else.
Especially during this new video player refactor, the hardest parts to follow were blocks with nested if statements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So one if/else is okay, but a third means use an early return?
I don't hate this but I do think this will result in idiosyncratic code because I doubt others are following this paradigm
However, debating this is classic bikeshedding, so ship whatever you want
@@ -100,7 +100,7 @@ export const selectVolume = createSelector( | |||
state => state.volume | |||
); | |||
|
|||
export const selecetMute = createSelector( | |||
export const selectMute = createSelector( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌
d74bb3f
to
cafd03d
Compare
0aff909
to
4544e8c
Compare
4544e8c
to
8f91bf5
Compare
8f91bf5
to
2b09d56
Compare
@kauffj this is ready for another review I would just look at the last commit I responded to your change requests and did all the floating player stuff in that one commit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code can be shipped, but some UX feedback left in Slack
By no means is this file viewer/purhcase logic code good, but this is sure as hell better than it was before.
The plan
Merge this shit before anyone can take a really close look at thisI'm happy with this now. It should be a lot easier to work moving forward.Things to actually do before this is ready
.lbry
file support