Subscriptions overhaul #1872

2018-08-13T18:34:40+02:00

daovist commented

2018-08-13 18:34:40 +02:00

(Migrated from github.com)

show new/incoming content immediately on subscriptions page #1734
prevent window notification when viewing subscriptions page; show snackbar instead
add auto-download client setting; use as necessary #1114
increase SUBSCRIPTION_DOWNLOAD_LIMIT from 1 to 3 and apply limit to total downloads rather than per-channel*
automatically (attempt to) claim subscription reward after subbing to channel (fail silently) #1835
load subscription data on app startup #1852
auto-download on startup #1775
Do #1852 and #1775 correctly. My fix uses setTimeout which is a hack. We should be using redux-persist's <PersistGate> component/feature so actions are dispatched once redux has been rehydrated. This requires upgrading redux-persist from version 4.x to 5.x. There is a migration guide.
The last commit prevents downloading anything immediately following a user subscribing to a channel. This wasn't in any of the issues but it was very weird from a UX perspective to auto-download multiple videos upon subscribing. It still sets latest so it will work like other subscription from then on.

*As it was, the app would download the limit (1) for each channel which could be too much if a user is subscribed to many channels. One issue with this approach is that it will dispatch downloads in the order it iterates through subscriptions. To fix that would require calling all the endpoints, flattening the data, and choosing which ones to download. I found this to be a reasonable compromise for when a user opens the app and may have lots of new content available. Any new content that is prevented from downloading this way still creates a NOTIFY_ONLY notification, adding to the badge count next to Subscriptions. We may want to allow a user to determine the value of SUBSCRIPTION_DOWNLOAD_LIMIT.

Epic #1861 - [x] show new/incoming content immediately on subscriptions page #1734 - [x] prevent window notification when viewing subscriptions page; show snackbar instead - [x] add auto-download client setting; use as necessary #1114 - [x] increase `SUBSCRIPTION_DOWNLOAD_LIMIT` from 1 to 3 and apply limit to total downloads rather than per-channel* - [x] automatically (attempt to) claim subscription reward after subbing to channel (fail silently) #1835 - [x] load subscription data on app startup #1852 - [x] auto-download on startup #1775 - [ ] Do #1852 and #1775 correctly. My fix uses setTimeout which is a hack. We should be using `redux-persist`'s [`<PersistGate>`](https://github.com/rt2zz/redux-persist/blob/master/docs/PersistGate.md) component/feature so actions are dispatched once redux has been rehydrated. This requires upgrading `redux-persist` from version `4.x` to `5.x`. There is a [migration guide](https://github.com/rt2zz/redux-persist/blob/master/docs/MigrationGuide-v5.md). - [ ] The last commit prevents downloading anything immediately following a user subscribing to a channel. This wasn't in any of the issues but it was very weird from a UX perspective to auto-download multiple videos upon subscribing. It still sets latest so it will work like other subscription from then on. *As it was, the app would download the limit (1) for each channel which could be too much if a user is subscribed to many channels. One issue with this approach is that it will dispatch downloads in the order it iterates through subscriptions. To fix that would require calling all the endpoints, flattening the data, and choosing which ones to download. I found this to be a reasonable compromise for when a user opens the app and may have lots of new content available. Any new content that is prevented from downloading this way still creates a `NOTIFY_ONLY` notification, adding to the badge count next to Subscriptions. We may want to allow a user to determine the value of `SUBSCRIPTION_DOWNLOAD_LIMIT`.

skhameneh (Migrated from github.com) reviewed 2018-08-13 18:34:40 +02:00

daovist commented

2018-08-13 18:45:15 +02:00

(Migrated from github.com)

One weird thing about this @seanyesmunt

I'm using the existing reducer FETCH_CHANNEL_CLAIMS_COMPLETED to update the claim info so it will show on the subscriptions page; the lack of doing so was causing the main quirk in all this #1734 . I am not using FETCH_CHANNEL_CLAIMS_STARTED because that prevented the subscriptions page from even loading. The code I started with dispatched CHECK_SUBSCRIPTION_STARTED and CHECK_SUBSCRIPTION_COMPLETED which had no reducers and thus did nothing.

One weird thing about this @seanyesmunt I'm using the existing reducer `FETCH_CHANNEL_CLAIMS_COMPLETED` to update the claim info so it will show on the subscriptions page; the lack of doing so was causing the main quirk in all this #1734 . I am not using `FETCH_CHANNEL_CLAIMS_STARTED` because that prevented the subscriptions page from even loading. The code I started with dispatched `CHECK_SUBSCRIPTION_STARTED` and `CHECK_SUBSCRIPTION_COMPLETED` which had no reducers and thus did nothing.

neb-b (Migrated from github.com) requested changes 2018-08-14 02:39:50 +02:00

neb-b (Migrated from github.com) left a comment

A few minor comments but looks good. Will need to do some testing before merging.

src/renderer/page/settings/view.jsx

neb-b (Migrated from github.com) commented

2018-08-14 02:33:26 +02:00

This should reference that it is for subscriptions.
Automatically download new content from your subscriptions? That might be a little long

This should reference that it is for subscriptions. `Automatically download new content from your subscriptions`? That might be a little long

src/renderer/redux/actions/content.js

					
				@ -138,7 +139,7 @@ export function doUpdateLoadStatus(uri, outpoint) {

				            0

neb-b (Migrated from github.com) commented

2018-08-14 02:35:36 +02:00

I think the idea behind snackbars is that they are reserved for user actions, which wouldn't make sense for this use case. Maybe we should create our own notification component that mimics desktop notifications but has some additional style.

Not needed for this PR but I think we should stick with the OS notifications for now.

I think the idea behind snackbars is that they are reserved for user actions, which wouldn't make sense for this use case. Maybe we should create our own notification component that mimics desktop notifications but has some additional style. Not needed for this PR but I think we should stick with the OS notifications for now.

src/renderer/redux/reducers/settings.js

neb-b (Migrated from github.com) commented

2018-08-14 02:38:46 +02:00

I think this should default to true. Users can turn it off if they want.

I think this should default to `true`. Users can turn it off if they want.

neb-b commented

2018-08-15 23:50:20 +02:00

(Migrated from github.com)

Found one issue (still testing).

I subscribed to a channel
Started downloading the first video
Saw a badge of (9) in the side bar
Finished downloading
Badge chagned to (10)

Found one issue (still testing). > I subscribed to a channel > Started downloading the first video > Saw a badge of `(9)` in the side bar > Finished downloading > Badge chagned to `(10)`

neb-b commented

2018-08-15 23:53:01 +02:00

(Migrated from github.com)

Possibly we should look at the date you subscribed to a channel, and if it is newer than that, then show the notification/download? Not sure.

Possibly we should look at the date you subscribed to a channel, and if it is newer than that, _then_ show the notification/download? Not sure.

daovist commented

2018-08-15 23:58:20 +02:00

(Migrated from github.com)

The badge 9/10 thing is because of how that number is calculated. It's the count of all notifications that are not DOWNLOADING, so just NOTIFY_ONLY and DOWNLOADED. I think counting downloading files as notifications seems like a good fix.

The badge 9/10 thing is because of how that number is calculated. It's the count of all notifications that are *not* `DOWNLOADING`, so just `NOTIFY_ONLY` and `DOWNLOADED`. I think counting downloading files as notifications seems like a good fix.

neb-b commented

2018-08-16 00:07:51 +02:00

(Migrated from github.com)

I think that number is ok, but it should only show the badge if it's actually new content, not just the first page of content on an old channel that I subscribed to.

In this case:

I subscribe to a channel
Close the app
Channel uploads 10 files
Open the app
See notification with (10)

I think that number is ok, but it should only show the badge if it's actually new content, not just the first page of content on an old channel that I subscribed to. In this case: > I subscribe to a channel > Close the app > Channel uploads 10 files > Open the app > See notification with `(10)`

tzarebczan commented

2018-08-16 03:45:20 +02:00

(Migrated from github.com)

@seanyesmunt and I also discussed exposing the autodownload setting on the sub page too.

Is autodownload on startup supposed to work with the hack? When I go into a claim I'm subbed to (and not downloaded), I see it starting to download automatically - is this intentional?

We should incorporate the download limit as a setting if we are going to impose it. I think it would also work better as a per channel instead of across all subs setting.

@seanyesmunt and I also discussed exposing the autodownload setting on the sub page too. Is autodownload on startup supposed to work with the hack? When I go into a claim I'm subbed to (and not downloaded), I see it starting to download automatically - is this intentional? We should incorporate the download limit as a setting if we are going to impose it. I think it would also work better as a per channel instead of across all subs setting.

kauffj commented

2018-08-16 20:48:03 +02:00

(Migrated from github.com)

Encountered this error running this branch:

subscriptions.js?421d:133 Uncaught ReferenceError: savedSubscription is not defined
    at eval (subscriptions.js?421d:133)
    at /home/kauffj/code/lbry-app/node_modules/redux-thunk/lib/index.js:11
    at dispatch (/home/kauffj/code/lbry-app/node_modules/redux/lib/applyMiddleware.js:45)
    at eval (subscriptions.js?421d:273)
    at Array.forEach (<anonymous>)
    at eval (subscriptions.js?421d:272)
    at /home/kauffj/code/lbry-app/node_modules/redux-thunk/lib/index.js:11
    at dispatch (/home/kauffj/code/lbry-app/node_modules/redux/lib/applyMiddleware.js:45)
    at eval (subscriptions.js?421d:282)

Encountered this error running this branch: ``` subscriptions.js?421d:133 Uncaught ReferenceError: savedSubscription is not defined at eval (subscriptions.js?421d:133) at /home/kauffj/code/lbry-app/node_modules/redux-thunk/lib/index.js:11 at dispatch (/home/kauffj/code/lbry-app/node_modules/redux/lib/applyMiddleware.js:45) at eval (subscriptions.js?421d:273) at Array.forEach (<anonymous>) at eval (subscriptions.js?421d:272) at /home/kauffj/code/lbry-app/node_modules/redux-thunk/lib/index.js:11 at dispatch (/home/kauffj/code/lbry-app/node_modules/redux/lib/applyMiddleware.js:45) at eval (subscriptions.js?421d:282) ```

neb-b (Migrated from github.com) approved these changes 2018-08-20 18:36:15 +02:00

Sign in to join this conversation.

No reviewers

skhameneh

neb-b