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

Add BlurNsfwExceptFromNsfwCommunities to blur types #1229

Merged
merged 18 commits into from
Oct 24, 2023

Conversation

MakcNmyc
Copy link
Contributor

My first pull request. I will be grateful for every advice

@MakcNmyc
Copy link
Contributor Author

It's also needed change nsfw type on server

app/schemas/com.jerboa.db.AppDB/26.json Outdated Show resolved Hide resolved
@MV-GH
Copy link
Collaborator

MV-GH commented Sep 10, 2023

It's also needed change nsfw type on server

What do you mean by this? Currently it is not even saved server side.

@MV-GH
Copy link
Collaborator

MV-GH commented Sep 10, 2023

Also move the listsetting to be below the other list settings

@MakcNmyc
Copy link
Contributor Author

MakcNmyc commented Sep 11, 2023

It's also needed change nsfw type on server

What do you mean by this? Currently it is not even saved server side.

I'm sorry. Perhaps I misunderstand how this works. I saw that the data in AccountSettings.kt is taken from siteViewModel.siteRes. There they are initialized as API.getInstance().getSite. Could you tell me where I can see how it works?

My response from the API server

My response from the API server - lemmynsfw.com

@MV-GH
Copy link
Collaborator

MV-GH commented Sep 11, 2023

show_nsfw is a account setting that controls wether you see any nsfw content at all in the feed. It's not about blurring images

@MakcNmyc
Copy link
Contributor Author

Made changes

Copy link
Collaborator

@MV-GH MV-GH left a comment

Choose a reason for hiding this comment

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

Looking better

@MakcNmyc
Copy link
Contributor Author

Made changes

Copy link
Collaborator

@MV-GH MV-GH left a comment

Choose a reason for hiding this comment

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

Other than that it looks good but until I am home I can't verify if the actual functionality works.

app/src/main/java/com/jerboa/feat/BlurNsfwTypes.kt Outdated Show resolved Hide resolved
@MV-GH MV-GH changed the title Change NSFW Blur toggle to drop-down menu which applies only to NSFW communities (#957) Change NSFW Blur toggle to drop-down menu with additional nsfwCommunityOnly and exceptNsfwCommunity options (#957) Sep 12, 2023
@MakcNmyc
Copy link
Contributor Author

Made changes

Copy link
Collaborator

@MV-GH MV-GH left a comment

Choose a reason for hiding this comment

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

Thanks for the changes, I will have tmmr some time to physically test these changes

app/src/main/java/com/jerboa/feat/BlurNsfwTypes.kt Outdated Show resolved Hide resolved
@MakcNmyc
Copy link
Contributor Author

Made changes

# Conflicts:
#	app/src/main/java/com/jerboa/ui/components/community/list/CommunityListActivity.kt
#	app/src/main/res/values/strings.xml
@MV-GH
Copy link
Collaborator

MV-GH commented Sep 14, 2023

Alright, I have confirmed the functionality and added my changes:

  • I moved the setting to bottom of the SettingsList
  • Added a Icon for the SettingsList option
  • Changed naming for the options again (This is clearer and shorter imo)
  • Probably by confusion before I had named them opposite (or you had the logic reversed)
  • Fixed the build errors due to referencings outdated stringRes. Now we use it directly from the enum to satisfy SSOT principles
  • Included the changes from main (Keep your PR upto date with main, smaller but more often merges keep the complexity lower)

Copy link
Collaborator

@MV-GH MV-GH left a comment

Choose a reason for hiding this comment

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

I have tested the functionality and did some changes

@MV-GH
Copy link
Collaborator

MV-GH commented Sep 14, 2023

Since I have made some changes, @dessalines, could you review my changes

@dessalines
Copy link
Member

Sure, testing now.

Copy link
Member

@dessalines dessalines left a comment

Choose a reason for hiding this comment

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

I'll allow myself to be overruled, but I don't think this is the right way to handle this. We already have a back-end blur_nsfw user setting, that appears to be ignored right now in favor of an app-specific setting, which we should do away with / deprecate.

I also don't see the additional blur types making too much sense. There's a blur_nsfw boolean, and the only additional one that might make sense, is the addition in the back end of a blur_all_previews setting.

app/src/main/java/com/jerboa/feat/BlurNsfwTypes.kt Outdated Show resolved Hide resolved
app/src/main/java/com/jerboa/feat/BlurNsfwTypes.kt Outdated Show resolved Hide resolved
app/src/main/java/com/jerboa/feat/BlurNsfwTypes.kt Outdated Show resolved Hide resolved
app/src/main/java/com/jerboa/feat/BlurNsfwTypes.kt Outdated Show resolved Hide resolved
app/src/main/java/com/jerboa/db/AppDB.kt Show resolved Hide resolved
app/src/main/java/com/jerboa/feat/BlurNsfwTypes.kt Outdated Show resolved Hide resolved
@MakcNmyc
Copy link
Contributor Author

Made changes

Copy link
Collaborator

@MV-GH MV-GH left a comment

Choose a reason for hiding this comment

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

Fix the conflicts too, I'll test it later

}

fun changeBlurTypeInsideCommunity(blurTypes: Int): Int =
if (blurTypes.toEnum<BlurTypes>() == NsfwExceptFromNsfwCommunities) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

compare on ordinal

Copy link
Collaborator

Choose a reason for hiding this comment

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

and blurType

enum class BlurTypes(@StringRes val resId: Int) : Parcelable {
Nothing(R.string.app_settings_nothing),
NSFW(R.string.app_settings_blur_nsfw),
NsfwExceptFromNsfwCommunities(R.string.app_settings_blur_nsfw_except_from_nsfw_communities),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove this type, as discussed we keep only, blur nothing, blur nsfw and blur nsfw but not in community view

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MV-GH I didn't understand you, only these 3 options remain. 4 option (ExceptFromNsfwInsideCommunity) it technical option. In community activity I swap NsfwExceptFromNsfwCommunities option for it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That's not how I meant that second approach. The second approach was, turn NSFWExceptInsideCommunity into Nothing if passed to communityActiviity else convert it to NSFW and then you only have to handle those two cases

Copy link
Contributor Author

Choose a reason for hiding this comment

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

else convert it to NSFW and then you only have to handle those two cases

I can't figure out how and where to do this. In Main Activity for every Activity?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes but you can do it once under appsetting and pass that. except for community activity where you then pass the other option. But first I have to know if Dessalines would accept such approach.

@dessalines Would you be against the above approach or do you rather that we have a extra param indicating that this is community view. Or do you have a better approach in mind?

Copy link
Member

Choose a reason for hiding this comment

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

NSFWExceptInsideCommunity into Nothing if passed to communityActiviity

Yes that seems fine. No point in adding an extra enum when it can figure that out by context, in the CommunityActivity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MV-GH Am I understanding correctly that "NsfwExceptFromNsfwCommunities" works as "Nothing" only for nsfw communities, otherwise it works as "NSFW"? If this is true then I can't change it in MainActivity because I need the community type.

Copy link
Collaborator

Choose a reason for hiding this comment

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

For what do you need the communityType? You map it there either to Nothing or NSFW ordinals depending if its for CommunityView

Copy link
Contributor Author

@MakcNmyc MakcNmyc Oct 9, 2023

Choose a reason for hiding this comment

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

@MV-GH what behavior should there be with the nsfw posts if Community.nsfw = false and blur option = "NsfwExceptFromNsfwCommunities"? Doesn't it then transform into, a "NSFW" not in "Nothing"?
I mean not communityType but Community.nsfw parameter

Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove the community nsfw option, we don't care anymore if the post comes from a nsfw community or not. And if the function receives NsfwExceptFromNsfwCommunity then throw with message likr this type should ve already been transformed into another

@MakcNmyc
Copy link
Contributor Author

MakcNmyc commented Oct 10, 2023

Pushed. I left only 3 options. "NsfwExceptFromNsfwCommunity" works like "Nsfw". And I transform "NsfwExceptFromNsfwCommunity" in "CommunityActivity" to "Nothing"

@MV-GH
Copy link
Collaborator

MV-GH commented Oct 10, 2023

You still need to resolve the conflicts,

Edit: i resolved them for you, If I did anything wrong, you can fix it.

@dessalines
Copy link
Member

Once CI passes I'll test to make sure its good too.

Copy link
Member

@dessalines dessalines left a comment

Choose a reason for hiding this comment

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

Lots of errors in PostListing.kt

@MakcNmyc
Copy link
Contributor Author

@dessalines fixed

@@ -82,7 +83,7 @@ fun CommunityActivity(
showVotingArrowsInListView: Boolean,
useCustomTabs: Boolean,
usePrivateTabs: Boolean,
blurNSFW: Boolean,
blurNSFW: Int,
Copy link
Member

Choose a reason for hiding this comment

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

Why can't these use the BlurType enum?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We don't do it either for other enums, see postActionMode below it

Copy link
Member

Choose a reason for hiding this comment

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

mmk, I'll test this shortly.

Copy link
Member

@dessalines dessalines left a comment

Choose a reason for hiding this comment

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

K I tested and its working fine. @MV-GH you're still on "request changes" btw.

@dessalines dessalines enabled auto-merge (squash) October 18, 2023 14:40
@MakcNmyc
Copy link
Contributor Author

@MV-GH Do I need to do anything else?

@MV-GH MV-GH changed the title Change NSFW Blur toggle to drop-down menu with additional nsfwCommunityOnly and exceptNsfwCommunity options (#957) Add BlurNsfwExceptFromNsfwCommunities to blur types Oct 24, 2023
@MV-GH
Copy link
Collaborator

MV-GH commented Oct 24, 2023

Lemme test this and I'll merge

@dessalines dessalines merged commit 4e89bbd into LemmyNet:main Oct 24, 2023
1 check passed
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.

3 participants