LBRYCommunity/lbry-desktop
1
0
Fork 0
Code Issues 596 Pull requests 6 Projects Releases 238 Packages Wiki Activity

Subscribe notify #1066

Merged
liamcardenas merged 9 commits from subscribe-notify into master 2018-03-08 06:13:26 +01:00
Conversation 0 Commits 9 Files changed 10 +38 -1
4 changed files with 38 additions and 1 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files
Showing only changes of commit 7296800ce0 - Show all commits

1
src/renderer/constants/action_types.js
View file

@ -164,6 +164,7 @@ export const CLEAR_SHAPE_SHIFT = 'CLEAR_SHAPE_SHIFT';
export const CHANNEL_SUBSCRIBE = 'CHANNEL_SUBSCRIBE'; export const CHANNEL_SUBSCRIBE = 'CHANNEL_SUBSCRIBE';
export const CHANNEL_UNSUBSCRIBE = 'CHANNEL_UNSUBSCRIBE'; export const CHANNEL_UNSUBSCRIBE = 'CHANNEL_UNSUBSCRIBE';
export const HAS_FETCHED_SUBSCRIPTIONS = 'HAS_FETCHED_SUBSCRIPTIONS'; export const HAS_FETCHED_SUBSCRIPTIONS = 'HAS_FETCHED_SUBSCRIPTIONS';
export const SET_SUBSCRIPTION_LATEST = 'SET_SUBSCRIPTION_LATEST';
// Video controls // Video controls
export const SET_VIDEO_PAUSE = 'SET_VIDEO_PAUSE'; export const SET_VIDEO_PAUSE = 'SET_VIDEO_PAUSE';

9
src/renderer/redux/actions/content.js
View file

@ -7,6 +7,7 @@ import Lbryio from 'lbryio';
import { normalizeURI, buildURI } from 'lbryURI'; import { normalizeURI, buildURI } from 'lbryURI';
import { doAlertError, doOpenModal } from 'redux/actions/app'; import { doAlertError, doOpenModal } from 'redux/actions/app';
import { doClaimEligiblePurchaseRewards } from 'redux/actions/rewards'; import { doClaimEligiblePurchaseRewards } from 'redux/actions/rewards';
import { setSubscriptionLatest } from 'redux/actions/subscriptions';
import { selectBadgeNumber } from 'redux/selectors/app'; import { selectBadgeNumber } from 'redux/selectors/app';
import { selectMyClaimsRaw } from 'redux/selectors/claims'; import { selectMyClaimsRaw } from 'redux/selectors/claims';
import { selectResolvingUris } from 'redux/selectors/content'; import { selectResolvingUris } from 'redux/selectors/content';
@ -358,6 +359,14 @@ export function doFetchClaimsByChannel(uri, page) {
const claimResult = result[uri] || {}; const claimResult = result[uri] || {};
const { claims_in_channel: claimsInChannel, returned_page: returnedPage } = claimResult; const { claims_in_channel: claimsInChannel, returned_page: returnedPage } = claimResult;
if(claimResult && claimResult.claims_in_channel && claimResult.claims_in_channel.length) {
let latest = claimResult.claims_in_channel[0];
dispatch(setSubscriptionLatest({
channelName: latest.channel_name,
uri: `${latest.channel_name}#${latest.value.publisherSignature.certificateId}`
}, `${latest.name}#${latest.claim_id}`));
}
dispatch({ dispatch({
type: ACTIONS.FETCH_CHANNEL_CLAIMS_COMPLETED, type: ACTIONS.FETCH_CHANNEL_CLAIMS_COMPLETED,
data: { data: {

11
src/renderer/redux/actions/subscriptions.js
View file

@ -14,5 +14,16 @@ export const doChannelUnsubscribe = (subscription: Subscription) => (dispatch: D
data: subscription, data: subscription,
}); });
export const setSubscriptionLatest = (subscription: Subscription, uri: string) => (dispatch: Dispatch) =>
{
return dispatch({
type: ACTIONS.SET_SUBSCRIPTION_LATEST,
neb-b commented 2018-03-06 19:13:27 +01:00 (Migrated from github.com)
Review
Copy link

Why do you need to pass in the new cost object. Shouldn't doPurchaseUri be able to select that data with the uri?

Why do you need to pass in the new cost object. Shouldn't `doPurchaseUri` be able to select that data with the uri?
kauffj commented 2018-03-06 21:55:55 +01:00 (Migrated from github.com)
Review
Copy link

Can we wait to show this notification until after content is ready to watch? I.e. after get has successfully started? That way when the user clicks the notification they get something immediately.

Can we wait to show this notification until after content is ready to watch? I.e. after `get` has successfully started? That way when the user clicks the notification they get something immediately.
liamcardenas commented 2018-03-07 20:27:56 +01:00 (Migrated from github.com)
Review
Copy link

its is more efficient this way because I already have the cost information so we don't need to fetch it again and

its is more efficient this way because I already have the cost information so we don't need to fetch it again and
liamcardenas commented 2018-03-07 20:37:11 +01:00 (Migrated from github.com)
Review
Copy link

Yes, @seanyesmunt brought this up as well. I have reasons for why I didn't make it this way, which I think we should discuss -- although I think you are right that we probably want to download it first and I probably should have implemented it this way. I will schedule this in the next sprint.

Yes, @seanyesmunt brought this up as well. I have reasons for why I didn't make it this way, which I think we should discuss -- although I think you are right that we probably want to download it first and I probably should have implemented it this way. I will schedule this in the next sprint.
kauffj commented 2018-03-07 22:51:26 +01:00 (Migrated from github.com)
Review
Copy link

I checked out the code here:

  1. doLoadVideo and doPurchaseUri are confusingly -- almost backwardly -- named. It may make sense to tweak these.
  2. Since you are already checking that the price is free, it seems like you could potentially skip doPurchaseUri and go right to doLoadVideo.
  3. Both doLoadVideo and doPurchaseUri can trigger modal popups. It is weird that asynchronous background loading of content can trigger a modal. Fairly guaranteed that this will cause confusion at some point.
  4. It doesn't seem particularly tricky to attach a notification to the success of the call to get. I think a light modification of doLoadVideo to take a notification or callback would get you there.
I checked out the code here: 1) `doLoadVideo` and `doPurchaseUri` are confusingly -- almost backwardly -- named. It may make sense to tweak these. 2) Since you are already checking that the price is free, it seems like you could potentially skip `doPurchaseUri` and go right to `doLoadVideo`. 3) Both `doLoadVideo` and `doPurchaseUri` can trigger modal popups. It is weird that asynchronous background loading of content can trigger a modal. Fairly guaranteed that this will cause confusion at some point. 4) It doesn't seem particularly tricky to attach a notification to the success of the call to `get`. I think a light modification of `doLoadVideo` to take a notification or callback would get you there.
data: {
subscription,
uri
}
})
};
export const setHasFetchedSubscriptions = () => (dispatch: Dispatch) => export const setHasFetchedSubscriptions = () => (dispatch: Dispatch) =>
dispatch({ type: ACTIONS.HAS_FETCHED_SUBSCRIPTIONS }); dispatch({ type: ACTIONS.HAS_FETCHED_SUBSCRIPTIONS });

18
src/renderer/redux/reducers/subscriptions.js
View file

@ -5,6 +5,7 @@ import { handleActions } from 'util/redux-utils';
export type Subscription = { export type Subscription = {
channelName: string, channelName: string,
uri: string, uri: string,
latest: ?string
}; };
// Subscription redux types // Subscription redux types
@ -28,7 +29,15 @@ type HasFetchedSubscriptions = {
type: ACTIONS.HAS_FETCHED_SUBSCRIPTIONS, type: ACTIONS.HAS_FETCHED_SUBSCRIPTIONS,
}; };
export type Action = doChannelSubscribe | doChannelUnsubscribe | HasFetchedSubscriptions; type setSubscriptionLatest = {
type: ACTIONS.SET_SUBSCRIPTION_LATEST,
data: {
subscription: Subscription,
uri: string
}
}
export type Action = doChannelSubscribe | doChannelUnsubscribe | HasFetchedSubscriptions | setSubscriptionLatest;
export type Dispatch = (action: Action) => any; export type Dispatch = (action: Action) => any;
const defaultState = { const defaultState = {
@ -70,6 +79,13 @@ export default handleActions(
...state, ...state,
hasFetchedSubscriptions: true, hasFetchedSubscriptions: true,
}), }),
[ACTIONS.SET_SUBSCRIPTION_LATEST]: (
state: SubscriptionState,
action: setSubscriptionLatest
): SubscriptionState => ({
...state,
subscriptions: state.subscriptions.map(subscription => subscription.channelName === action.data.subscription.channelName ? {...subscription, latest: action.data.uri} : subscription)
})
}, },
defaultState defaultState
); );