Skip to content
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

Subscriptions overhaul #1872

Merged
merged 13 commits into from
Aug 20, 2018
Merged

Subscriptions overhaul #1872

merged 13 commits into from
Aug 20, 2018

Conversation

daovist
Copy link
Contributor

@daovist daovist commented Aug 13, 2018

Epic #1861

*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.

@daovist daovist requested a review from neb-b August 13, 2018 16:34
@lbry-bot lbry-bot assigned neb-b and unassigned neb-b Aug 13, 2018
@daovist
Copy link
Contributor Author

daovist commented Aug 13, 2018

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 neb-b requested a review from skhameneh August 13, 2018 18:58
@lbry-bot lbry-bot assigned skhameneh and neb-b and unassigned neb-b Aug 13, 2018
Copy link

@neb-b neb-b left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

name="auto_download"
onChange={this.onAutoDownloadChange}
checked={autoDownload}
postfix={__('Automatically download new content')}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

dispatch(
doNotify({
message: `'${fileInfo.metadata.title}' has been downloaded`,
displayType: ['snackbar'],
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@@ -26,6 +26,7 @@ const defaultState = {
automaticDarkModeEnabled: getLocalStorageSetting(SETTINGS.AUTOMATIC_DARK_MODE_ENABLED, false),
autoplay: getLocalStorageSetting(SETTINGS.AUTOPLAY, false),
resultCount: Number(getLocalStorageSetting(SETTINGS.RESULT_COUNT, 50)),
autoDownload: getLocalStorageSetting(SETTINGS.AUTO_DOWNLOAD, false),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@neb-b
Copy link

neb-b commented Aug 15, 2018

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
Copy link

neb-b commented Aug 15, 2018

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
Copy link
Contributor Author

daovist commented Aug 15, 2018

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
Copy link

neb-b commented Aug 15, 2018

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
Copy link
Contributor

@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
Copy link
Member

kauffj commented Aug 16, 2018

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)

@daovist daovist force-pushed the subscriptions-overhaul branch 2 times, most recently from 2673dc6 to d254065 Compare August 17, 2018 12:50
@neb-b neb-b merged commit a5bc6ad into master Aug 20, 2018
@neb-b neb-b deleted the subscriptions-overhaul branch September 10, 2018 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants