kill render-media (range-requests) and add floating player #2707

neb-b · 2019-08-02T06:25:25Z

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 this~~ I'm happy with this now. It should be a lot easier to work moving forward.

Things to actually do before this is ready

Fix purchase confirmation modal
Fix .lbry file support
Fix volume/mute memory
Add super basic image viewer
Add back analytics (possibly do this in another PR)
- I stripped out all the analytics stuff for file views since it was so tangled up (and only worked for videos)

neb-b · 2019-08-06T03:30:03Z

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 play api. It should be fairly easy to add for all file types now.

neb-b · 2019-08-06T03:40:04Z

src/ui/page/settings/view.jsx

-                      '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')}


@tzarebczan Can you add some help text for this setting?

Will add it today.

kauffj

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/fileActions/view.jsx

kauffj · 2019-08-06T22:13:05Z

src/ui/component/fileRender/view.jsx

-        <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} />,


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)

If download path exists and the file is completed, we should use that for the video and audio player.

kauffj · 2019-08-06T22:14:15Z

src/ui/component/selectThumbnail/view.jsx

@@ -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';


out of scope for this PR, but I wonder if putting all of these on the Lbry object is the right design

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

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?

kauffj · 2019-08-06T22:15:27Z

src/ui/component/viewers/appViewer/view.jsx

+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/';


are these important enough configuration values that they should be hoisted to top-level config of some kind? does anything else use them?

Nope, they exist entirely for this app viewer

kauffj · 2019-08-06T22:15:53Z

src/ui/component/viewers/appViewer/view.jsx

+          src={appUrl}
+          autosize="on"
+          style={{ border: 0, width: '100%', height: '100%' }}
+          useragent="Mozilla/5.0 AppleWebKit/537 Chrome/60 Safari/537"


can this be read from Electron?

Possibly. I haven't spent any time looking into that, just copy pasting it from the old file

src/ui/redux/actions/content.js

kauffj · 2019-08-06T22:25:05Z

src/ui/redux/actions/content.js

+    const fileInfo = makeSelectFileInfoForUri(uri)(state);
+    const uriIsStreamable = makeSelectUriIsStreamable(uri)(state);
+    const downloadingByOutpoint = selectDownloadingByOutpoint(state);
+    const alreadyDownloaded = fileInfo && (fileInfo.completed || (fileInfo.blobs_remaining === 0 && uriIsStreamable));


I've seen a few fileInfo checks similar to this, consider if it makes sense to combine into a selector (it might not).

kauffj · 2019-08-06T22:25:29Z

src/ui/redux/actions/content.js

+    const daemonSettings = selectDaemonSettings(state);
+    const costInfo = makeSelectCostInfoForUri(uri)(state);
+    const cost = Number(costInfo.cost);
+    const saveFile = !uriIsStreamable ? true : daemonSettings.save_files || saveFileOverride || cost > 0;


why always save if cost > 0? I would not expect this

@seanyesmunt I heard the answer to this but I still question it. I say remove.

kauffj · 2019-08-06T22:25:53Z

src/ui/redux/actions/content.js

+
+    if (cost === 0 || skipCostCheck) {
+      beginGetFile();
+      return;


return is avoidable if made into 3 nested if

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.

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

kauffj · 2019-08-06T22:26:16Z

src/ui/redux/selectors/app.js

@@ -100,7 +100,7 @@ export const selectVolume = createSelector(
  state => state.volume
 );

-export const selecetMute = createSelector(
+export const selectMute = createSelector(


neb-b · 2019-08-13T05:36:42Z

@kauffj this is ready for another review
I gave up on mute/volume/position memory, will circle back after this is merged but I'd rather get this in so I can start on the other tickets before the release.

I would just look at the last commit
2b09d56

I responded to your change requests and did all the floating player stuff in that one commit.

kauffj

Code can be shipped, but some UX feedback left in Slack

lbry-bot assigned neb-b Aug 2, 2019

neb-b force-pushed the not-streaming-but-streaming branch from 77f95d3 to 0eb409c Compare August 2, 2019 06:28

neb-b mentioned this pull request Aug 2, 2019

[WIP] Range requests #2619

Closed

9 tasks

neb-b force-pushed the not-streaming-but-streaming branch 2 times, most recently from cb7fb90 to 6aa8768 Compare August 6, 2019 03:28

neb-b commented Aug 6, 2019

View reviewed changes

lbry-bot assigned neb-b and unassigned neb-b Aug 6, 2019

kauffj self-requested a review August 6, 2019 15:07

lbry-bot assigned kauffj and unassigned neb-b Aug 6, 2019

kauffj reviewed Aug 6, 2019

View reviewed changes

lbry-bot assigned neb-b and unassigned kauffj and neb-b Aug 6, 2019

neb-b force-pushed the not-streaming-but-streaming branch from d74bb3f to cafd03d Compare August 12, 2019 16:03

Sean Yesmunt added 4 commits August 12, 2019 12:37

oh boy

f25559a

fix paid content - thanks tom

cd3edca

create AppViewer for .lbry files

ba2ccd4

cleanup

90bcde4

neb-b force-pushed the not-streaming-but-streaming branch 2 times, most recently from 0aff909 to 4544e8c Compare August 13, 2019 04:59

lbry-bot assigned neb-b and unassigned neb-b Aug 13, 2019

neb-b force-pushed the not-streaming-but-streaming branch from 4544e8c to 8f91bf5 Compare August 13, 2019 05:27

floating player actually working

2b09d56

neb-b force-pushed the not-streaming-but-streaming branch from 8f91bf5 to 2b09d56 Compare August 13, 2019 05:35

neb-b requested a review from kauffj August 13, 2019 05:36

lbry-bot assigned kauffj and unassigned neb-b and kauffj Aug 13, 2019

kauffj approved these changes Aug 13, 2019

View reviewed changes

lbry-bot assigned neb-b and unassigned kauffj Aug 13, 2019

more consistent floating close buttons

b60fcb0

neb-b merged commit 1ce826f into master Aug 13, 2019

neb-b changed the title ~~kill render-media (range-requests)~~ kill render-media (range-requests) and add floating player Aug 13, 2019

neb-b deleted the not-streaming-but-streaming branch August 14, 2019 00:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kill render-media (range-requests) and add floating player #2707

kill render-media (range-requests) and add floating player #2707

neb-b commented Aug 2, 2019 •

edited

Loading

neb-b commented Aug 6, 2019

neb-b Aug 6, 2019

tzarebczan Aug 8, 2019

kauffj left a comment

kauffj Aug 6, 2019

tzarebczan Aug 8, 2019

kauffj Aug 6, 2019

tzarebczan Aug 8, 2019

neb-b Aug 13, 2019

kauffj Aug 6, 2019

neb-b Aug 7, 2019

kauffj Aug 6, 2019

neb-b Aug 8, 2019

kauffj Aug 6, 2019

kauffj Aug 6, 2019

kauffj Aug 13, 2019

kauffj Aug 6, 2019

neb-b Aug 7, 2019

kauffj Aug 7, 2019

kauffj Aug 6, 2019

neb-b commented Aug 13, 2019

kauffj left a comment

kill render-media (range-requests) and add floating player #2707

kill render-media (range-requests) and add floating player #2707

Conversation

neb-b commented Aug 2, 2019 • edited Loading

The plan

Things to actually do before this is ready

neb-b commented Aug 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kauffj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neb-b commented Aug 13, 2019

kauffj left a comment

Choose a reason for hiding this comment

neb-b commented Aug 2, 2019 •

edited

Loading