Range request support for desktop #176

2019-08-02T07:12:22+02:00

neb-b commented

2019-08-02 07:12:22 +02:00

(Migrated from github.com)

Changes

Update fileInfo after successful get to populate with streaming_url
Removed purchasedStreamingUrls from redux, this can now just be accessed from fileInfo

Notes

@akinwale I still think it makes sense to combine file and file_info reducers. I think it could be valuable to keep purchase history separate, so we can persist that without persisting a user's entire file_list (maybe?). Maybe file should be renamed to purchase? Or something similar. Not a big deal now though.

#### Changes - Update `fileInfo` after successful `get` to populate with `streaming_url` - Removed `purchasedStreamingUrls` from redux, this can now just be accessed from `fileInfo` #### Notes @akinwale I still think it makes sense to combine `file` and `file_info` reducers. I think it could be valuable to keep purchase history separate, so we can persist that without persisting a user's entire `file_list` (maybe?). Maybe `file` should be renamed to `purchase`? Or something similar. Not a big deal now though.

neb-b (Migrated from github.com) reviewed

2019-08-02 16:56:57 +02:00

src/redux/selectors/file.js

@akinwale Do we even need to keep track of streamingUrls if we can just access it in fileInfo?

@akinwale Do we even need to keep track of `streamingUrls` if we can just access it in `fileInfo`?

neb-b (Migrated from github.com) reviewed

2019-08-06 04:28:14 +02:00

src/lbry.js

					
				@ -43,0 +48,4 @@

				    const fileExt = extName ? `.${extName}` : null;

				    const testString = fileName || fileExt;

				    // Get mediaType from file extension

I guess there have been some changes to this in the desktop codebase and we never moved over. Just copy pasting this in so we can switch to it.

Android doesn't use the second argument for this function so changing it should be fine.

I guess there have been some changes to this in the desktop codebase and we never moved over. Just copy pasting this in so we can switch to it. Android doesn't use the second argument for this function so changing it should be fine.

tzarebczan (Migrated from github.com) reviewed

2019-08-06 04:53:41 +02:00

src/lbry.js

					
				@ -43,0 +48,4 @@

				    const fileExt = extName ? `.${extName}` : null;

				    const testString = fileName || fileExt;

				    // Get mediaType from file extension

I also mentioned to you that the SDK now returns a stream type:

      },
      "stream_type": "video",
      ],

The mapping is here - I think it should be able to work with some small adjustments (glaringly missing are fbx/gcode, but not sure if anyone uses those): lbryio/lbry-sdk@41b866c1ee/lbry/lbry/schema/mime_types.py

I also mentioned to you that the SDK now returns a stream type: ``` ... }, "stream_type": "video", ], ``` The mapping is here - I think it should be able to work with some small adjustments (glaringly missing are fbx/gcode, but not sure if anyone uses those): https://github.com/lbryio/lbry-sdk/blob/41b866c1eeb002aaa0e42bc71734f6879d3b4457/lbry/lbry/schema/mime_types.py

kauffj commented

2019-08-12 21:04:02 +02:00

(Migrated from github.com)

@shockr can you review/merge before 11am meeting tomorrow?

akinwale (Migrated from github.com) requested changes

2019-08-13 10:48:27 +02:00

akinwale (Migrated from github.com) left a comment

Just a couple of issues to clarify. Everything else looks good.

src/redux/actions/file.js

					
				@ -5,57 +5,70 @@ import { doToast } from 'redux/actions/notifications';

				import { selectBalance } from 'redux/selectors/wallet';

				import { makeSelectFileInfoForUri, selectDownloadingByOutpoint } from 'redux/selectors/file_info';

				import { makeSelectStreamingUrlForUri } from 'redux/selectors/file';

				import { makeSelectClaimForUri } from 'redux/selectors/claims';

Since doPurchaseUri also dispatches the same action, how do we differentiate between the two? I recall using a different action here because it's possible for a purchase to fail due to insufficient credits, the wallet being locked or some other condition, so get doesn't actually get called in those scenarios.

Since `doPurchaseUri` also dispatches the same action, how do we differentiate between the two? I recall using a different action here because it's possible for a purchase to fail due to insufficient credits, the wallet being locked or some other condition, so `get` doesn't actually get called in those scenarios.

src/redux/selectors/file_info.js

Can a file be considered downloaded where just blobs_completed > 0? I believe it's also possible for streamed files to meet the condition blobs_completed > 0, but I'll have to test this to be sure.

Can a file be considered downloaded where just `blobs_completed > 0`? I believe it's also possible for streamed files to meet the condition `blobs_completed > 0`, but I'll have to test this to be sure.

neb-b (Migrated from github.com) reviewed

2019-08-13 16:54:23 +02:00

src/redux/selectors/file_info.js

The way we have been using this selector in the past is is any part of it downloaded. Possibly we should add another called selectFileInfosCompletelyDownloaded? Or something similar.

The way we have been using this selector in the past is `is any part of it downloaded`. Possibly we should add another called `selectFileInfosCompletelyDownloaded`? Or something similar.

neb-b (Migrated from github.com) reviewed

2019-08-13 16:57:54 +02:00

src/redux/actions/file.js

					
				@ -5,57 +5,70 @@ import { doToast } from 'redux/actions/notifications';

				import { selectBalance } from 'redux/selectors/wallet';

				import { makeSelectFileInfoForUri, selectDownloadingByOutpoint } from 'redux/selectors/file_info';

				import { makeSelectStreamingUrlForUri } from 'redux/selectors/file';

				import { makeSelectClaimForUri } from 'redux/selectors/claims';

Ah that's a good point. I just searched for LOADING_FILE_ and didn't find anything so I figured it wasn't needed. That does make sense to have two though. I'll add it back. Maybe GET_FILE_XXX to signal that it's specifically for get calls?

Ah that's a good point. I just searched for `LOADING_FILE_` and didn't find anything so I figured it wasn't needed. That does make sense to have two though. I'll add it back. Maybe `GET_FILE_XXX` to signal that it's specifically for `get` calls?

akinwale (Migrated from github.com) reviewed

2019-08-13 17:13:47 +02:00

src/redux/actions/file.js

					
				@ -5,57 +5,70 @@ import { doToast } from 'redux/actions/notifications';

				import { selectBalance } from 'redux/selectors/wallet';

				import { makeSelectFileInfoForUri, selectDownloadingByOutpoint } from 'redux/selectors/file_info';

				import { makeSelectStreamingUrlForUri } from 'redux/selectors/file';

				import { makeSelectClaimForUri } from 'redux/selectors/claims';

GET_FILE_XXX sounds perfect.

`GET_FILE_XXX` sounds perfect.

akinwale (Migrated from github.com) reviewed

2019-08-13 17:14:55 +02:00

src/redux/selectors/file_info.js

That makes sense so it's fine as is. For completely downloaded, the condition would have to be something like fileInfo.completed || fileInfo.written_bytes >= fileInfo.total_bytes.

That makes sense so it's fine as is. For completely downloaded, the condition would have to be something like `fileInfo.completed || fileInfo.written_bytes >= fileInfo.total_bytes`.

Sign in to join this conversation.

No reviewers

akinwale

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

LBRYCommunity/lbry-redux!176

Rows
Columns