LBRYCommunity/lbry-desktop

Fork 0

Repost settings #3712

Merged

Lukewh merged 6 commits from repost-settings into master

2020-02-28 18:37:44 +01:00

Lukewh commented

2020-02-20 13:37:21 +01:00

(Migrated from github.com)

PR Checklist

Please check all that apply to this PR using "x":

I have checked that this PR is not a duplicate of an existing PR (open, closed or merged)
I have checked that this PR does not introduce a breaking change
This PR introduces breaking changes and I have provided a detailed explanation below

PR Type

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting)
Refactoring (no functional changes)
Documentation changes
Other - Please describe:

Fixes

Issue Number:

What is the current behavior?

Currently there is no way to filter out reposts from those you follow.

What is the new behavior?

A setting that allows you to filter reposts.

Other information

The issue with this PR is that if showReposts is set to false claim searches are performed with a defined claim_type of stream. This may inadvertently filter out channels and any future claim types.
Ideally the LBRY SDK would have a not_repost filter as it does for not_tags and not_channels (though I understand the latter are both arrays of id's).

It's also not ideal that this is starting with reposts turned off by default - any advice on how to set the default to true would be appreciated.

## PR Checklist Please check all that apply to this PR using "x": - [x] I have checked that this PR is not a duplicate of an existing PR (open, closed or merged) - [ ] I have checked that this PR does not introduce a breaking change - [x] This PR introduces breaking changes and I have provided a detailed explanation below ## PR Type What kind of change does this PR introduce? - [ ] Bugfix - [x] Feature - [ ] Code style update (formatting) - [ ] Refactoring (no functional changes) - [ ] Documentation changes - [ ] Other - Please describe: ## Fixes Issue Number: ## What is the current behavior? Currently there is no way to filter out reposts from those you follow. ## What is the new behavior? A setting that allows you to filter reposts. ## Other information The issue with this PR is that if `showReposts` is set to `false` claim searches are performed with a defined `claim_type` of `stream`. This may inadvertently filter out channels and any future claim types. Ideally the LBRY SDK would have a `not_repost` filter as it does for `not_tags` and `not_channels` (though I understand the latter are both arrays of id's). It's also not ideal that this is starting with reposts turned off by default - any advice on how to set the default to true would be appreciated.

neb-b (Migrated from github.com) requested changes 2020-02-20 19:00:29 +01:00

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

Nice!

To set a default value for a new setting, you have to add a line here: https://github.com/lbryio/lbry-desktop/blob/master/ui/redux/reducers/settings.js#L57

Also can you add a changelog entry?
9f04dd5bfc

Nice! To set a default value for a new setting, you have to add a line here: https://github.com/lbryio/lbry-desktop/blob/master/ui/redux/reducers/settings.js#L57 Also can you add a changelog entry? https://github.com/lbryio/lbry-desktop/commit/9f04dd5bfc660715dfadde9257b50c58b333a5a9

ui/component/claimListDiscover/view.jsx

neb-b (Migrated from github.com) commented

2020-02-20 18:58:53 +01:00

this can be options.claim_type = ["stream", "channel"]

this can be `options.claim_type = ["stream", "channel"]`

Lukewh (Migrated from github.com) reviewed 2020-02-20 19:22:39 +01:00

ui/component/claimListDiscover/view.jsx

Lukewh (Migrated from github.com) commented

2020-02-20 19:22:39 +01:00

Great. The SDK docs seem to suggest you can only supply a string.

Will update later / tomorrow morning UK time

Great. The SDK docs seem to suggest you can only supply a string. Will update later / tomorrow morning UK time

neb-b commented

2020-02-20 19:31:37 +01:00

(Migrated from github.com)

That was added very recently. I guess the docs haven’t been updated yet.

On Thu, Feb 20, 2020 at 1:22 PM Luke Wesley-Holley notifications@github.com
wrote:

@Lukewh commented on this pull request.

In ui/component/claimListDiscover/view.jsx
https://github.com/lbryio/lbry-desktop/pull/3712#discussion_r382176047:

@@ -155,6 +158,10 @@ function ClaimListDiscover(props: Props) {
}
}

if (!showReposts) {

options.claim_type = 'stream';

Great. The SDK docs seem to suggest you can only supply a string.

Will update later / tomorrow morning UK time

—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
https://github.com/lbryio/lbry-desktop/pull/3712?email_source=notifications&email_token=AEAZZDUPBMAPZP33SKYRDO3RD3C7BA5CNFSM4KYN6F6KYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCWK2NEY#discussion_r382176047,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AEAZZDQOD2LVRQCYO5Y3GG3RD3C7BANCNFSM4KYN6F6A
.

--
Sean Yesmunt
Engineer for lbry.com

That was added very recently. I guess the docs haven’t been updated yet. On Thu, Feb 20, 2020 at 1:22 PM Luke Wesley-Holley <notifications@github.com> wrote: > *@Lukewh* commented on this pull request. > ------------------------------ > > In ui/component/claimListDiscover/view.jsx > <https://github.com/lbryio/lbry-desktop/pull/3712#discussion_r382176047>: > > > @@ -155,6 +158,10 @@ function ClaimListDiscover(props: Props) { > } > } > > + if (!showReposts) { > + options.claim_type = 'stream'; > > Great. The SDK docs seem to suggest you can only supply a string. > > Will update later / tomorrow morning UK time > > — > You are receiving this because you were assigned. > Reply to this email directly, view it on GitHub > <https://github.com/lbryio/lbry-desktop/pull/3712?email_source=notifications&email_token=AEAZZDUPBMAPZP33SKYRDO3RD3C7BA5CNFSM4KYN6F6KYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCWK2NEY#discussion_r382176047>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AEAZZDQOD2LVRQCYO5Y3GG3RD3C7BANCNFSM4KYN6F6A> > . > -- Sean Yesmunt Engineer for lbry.com

tzarebczan commented

2020-02-20 19:49:39 +01:00

(Migrated from github.com)

Thanks for the PR! I know @jessopb was going to add this to his filtering options under https://github.com/lbryio/lbry-desktop/pull/3667 - so I'm not sure we want a separate setting for this.

tzarebczan commented

2020-02-20 20:25:22 +01:00

(Migrated from github.com)

hey @Lukewh , if you'd like, send me an email at hello@lbry.com so we can get you added to our Tech Guests slack chat so you can communicate with us more easily!

Lukewh (Migrated from github.com) reviewed 2020-02-20 22:32:20 +01:00

ui/component/claimListDiscover/view.jsx

Lukewh (Migrated from github.com) commented

2020-02-20 22:32:20 +01:00

For some reason the app won't start for me to test these changes. I'll push tomorrow 👍

For some reason the app won't start for me to test these changes. I'll push tomorrow :+1:

Lukewh (Migrated from github.com) reviewed 2020-02-21 12:35:23 +01:00

ui/component/claimListDiscover/view.jsx

Lukewh (Migrated from github.com) commented

2020-02-21 12:35:23 +01:00

I added a comment here earlier then deleted it because I thought I was wrong... But I don't think I was - the SDK seems to only support this for the claim_list endpoint: https://github.com/lbryio/lbry-sdk/pull/2777
So I've left this as is.

Lukewh commented

2020-02-21 12:36:28 +01:00

(Migrated from github.com)

Thanks for the PR! I know @jessopb was going to add this to his filtering options under #3667 - so I'm not sure we want a separate setting for this.

No worries, I figure I'd update and keep this PR open anyway - there might be some reason the filters are delayed 🤷‍♂️

> Thanks for the PR! I know @jessopb was going to add this to his filtering options under #3667 - so I'm not sure we want a separate setting for this. No worries, I figure I'd update and keep this PR open anyway - there might be some reason the filters are delayed :man_shrugging:

neb-b (Migrated from github.com) reviewed 2020-02-21 15:39:46 +01:00

ui/component/claimListDiscover/view.jsx

neb-b (Migrated from github.com) commented

2020-02-21 15:39:45 +01:00

I gotcha. I left a comment on that asking for the other endpoints to be added too

neb-b commented

2020-02-21 15:40:18 +01:00

(Migrated from github.com)

I think we should hold off on this until other sdk commands take a list as the value for claim_type. We still want channels to surface from different searches

I think we should hold off on this until other sdk commands take a list as the value for `claim_type`. We still want channels to surface from different searches

👍 1

neb-b (Migrated from github.com) reviewed 2020-02-21 16:01:30 +01:00

ui/component/claimListDiscover/view.jsx

neb-b (Migrated from github.com) commented

2020-02-21 16:01:30 +01:00

@Lukewh After talking with @tzarebczan, I think this is supported, but you need to be on the latest version of the SDK. If you are using lbry.tv, it might not be

Lukewh (Migrated from github.com) reviewed 2020-02-21 16:56:01 +01:00

ui/component/claimListDiscover/view.jsx

Lukewh (Migrated from github.com) commented

2020-02-21 16:56:00 +01:00

I'm just downloading the latest release of the sdk from the github releases page - I guess I should pull and build myself

neb-b (Migrated from github.com) reviewed 2020-02-21 17:21:25 +01:00

ui/component/claimListDiscover/view.jsx

neb-b (Migrated from github.com) commented

2020-02-21 17:21:25 +01:00

That works, also this project downloads the sdk specified in the package json when you run yarn. I just updated it to 60 so it should work if you rebase

That works, also this project downloads the sdk specified in the package json when you run `yarn`. I just updated it to 60 so it should work if you rebase

kauffj (Migrated from github.com) reviewed 2020-02-24 21:14:00 +01:00

ui/component/claimListDiscover/view.jsx

kauffj (Migrated from github.com) commented

2020-02-24 21:13:59 +01:00

late to the party, but a better design for this would probably be:

if (defined) {
diff array
} else {
make default
}

late to the party, but a better design for this would probably be: if (defined) { diff array } else { make default }

Lukewh (Migrated from github.com) reviewed 2020-02-25 13:26:06 +01:00

ui/component/claimListDiscover/view.jsx

Lukewh (Migrated from github.com) commented

2020-02-25 13:26:05 +01:00

I originally had all the code be the negative form of "hideReposts" which would allow what you suggest, but the rest of the naming takes the positive form (from what I can tell) of "showThing" so I conformed :). Happy to change it back.

neb-b commented

2020-02-26 22:49:19 +01:00

(Migrated from github.com)

@Lukewh I pushed up a commit to make a change to the way the claimType prop worked since we added it after this PR was opened (and somehow brought a rogue commit into this).

I tested and noticed the sdk timing out on queries so maybe sending lists for claim_type doesn't work. I'll confirm. This can be merged once that is ready

@Lukewh I pushed up a commit to make a change to the way the claimType prop worked since we added it after this PR was opened (and somehow brought a rogue commit into this). I tested and noticed the sdk timing out on queries so maybe sending lists for claim_type doesn't work. I'll confirm. This can be merged once that is ready

👍 1 🎉 1

neb-b (Migrated from github.com) approved these changes 2020-02-28 18:13:25 +01:00

No reviewers

neb-b