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

Controversial posts and comments #1106

Merged
merged 19 commits into from
Aug 24, 2023

Conversation

iByteABit256
Copy link
Contributor

Closes #1105

@MV-GH
Copy link
Collaborator

MV-GH commented Jul 27, 2023

Compatibility is kinda a problem, this will probably only be supported in lemmy 18.3. We will still support 18.0. So, some compatibility checks will need to be added. An implementation that I was thinking about was adding the version all long each sorting type. Then have a getSortingTypes(version) which filters on version. And you can get the version from siteviewmodel.

@iByteABit256
Copy link
Contributor Author

hm ok, I'll give it a try

@iByteABit256
Copy link
Contributor Author

Then have a getSortingTypes(version) which filters on version

This would probably mean showing UI components dynamically though which isn't done atm, right?
We'd also need to have an 'introduction version' for every single sort type, even the ones that existed from the beginning.

I was thinking of doing it for this sort specifically and show the component conditionally, leaving the older ones unaffected. It would also be useful in the case that controversial votes should be hidden according to enableDownvotes.

@dessalines
Copy link
Member

@MV-GH w/ respect to versioning, since controversial won't come until v0.19.0

We'll adopt here what we do for the rest of lemmy, called stable mainline. In short:

  • I've created a release/v0.18 branch. This branch is an orphan, and never merged back into main.
    • Fixes and all PRs made to main, then cherry-picked to the release branch.
  • You can merge breaking changes into main, and use https://ds9.lemmy.ml , which should be the main branch lemmy, to test.

@MV-GH
Copy link
Collaborator

MV-GH commented Jul 29, 2023

This would probably mean showing UI components dynamically though which isn't done atm, right?

Actually for the sort types it should already do that, But there might be some remnants where it is still hardcoded. I made some changes in this regard. As I foresaw more sort types and it was rather troubling having to trackdown all these hardcoded values.

We'd also need to have an 'introduction version' for every single sort type, even the ones that existed from the beginning.

Yes but you can just use 0.18 as we wont support any lower version anyway

@iByteABit256
Copy link
Contributor Author

How about here for example? I guess we could have a map from component to version with a default value of 0.18, and then wrap each IconAndTextDrawerItem with an if statement for it.

                IconAndTextDrawerItem(
                    text = stringResource(R.string.dialogs_hot),
                    icon = Icons.Outlined.LocalFireDepartment,
                    onClick = { onClickSortType(SortType.Hot) },
                    highlight = (selectedSortType == SortType.Hot),
                )
                IconAndTextDrawerItem(
                    text = stringResource(R.string.dialogs_new),
                    icon = Icons.Outlined.BrightnessLow,
                    onClick = { onClickSortType(SortType.New) },
                    highlight = (selectedSortType == SortType.New),
                )
                IconAndTextDrawerItem(
                    text = stringResource(R.string.dialogs_old),
                    icon = Icons.Outlined.History,
                    onClick = { onClickSortType(SortType.Old) },
                    highlight = (selectedSortType == SortType.Old),
                )
                IconAndTextDrawerItem(
                    text = stringResource(R.string.dialogs_controversial),
                    icon = Icons.Outlined.ThumbsUpDown,
                    onClick = { onClickSortType(SortType.Controversial) },
                    highlight = (selectedSortType == SortType.Controversial),
                )

You can merge breaking changes into main, and use https://ds9.lemmy.ml/ , which should be the main branch lemmy, to test.

What I get from this though, is that we can push this to main even if it isn't intended for release 0.18, and it just won't be cherry picked for this release, but for the one where this sorting is introduced in the backend as well.

@MV-GH
Copy link
Collaborator

MV-GH commented Jul 29, 2023

Could you not there not do something like this getSortypes(version).intersect(listOf(SortType.Hot, SortType.New, ...)) ?

That stable mainline policy will probably only be implemented once 0.19 hits. Could still be far away.

One thing you also will be able to do is add the image to the enum so that we don't have to specify it again each time.

@iByteABit256
Copy link
Contributor Author

getSortypes(version).intersect(listOf(SortType.Hot, SortType.New, ...))

I could, but I would need to create the IconAndTextDrawerItems dynamically somehow for each item of the returned set instead of how it's done right now.

One thing you also will be able to do is add the image to the enum so that we don't have to specify it again each time

You mean the Icons.Outlined.ThumbsUpDown icon? I could create a mapping function like we do for the other fields, something like this:

fun getCommentSortTypeIcon(ctx: Context, commentSortType: CommentSortType): androidx.compose.ui.graphics.vector.ImageVector {
  // ...
}

@MV-GH
Copy link
Collaborator

MV-GH commented Jul 29, 2023

I could, but I would need to create the IconAndTextDrawerItems dynamically somehow for each item of the returned set instead of how it's done right now.

getSortypes(version).intersect(listOf(SortType.Hot, SortType.New, ...)).foreach(
 IconAndTextDrawerItem(
                    text =  getLocalizedSortingTypeLongName(ctx, it),
                    icon = Icons.Outlined.LocalFireDepartment, // Either map, or store it on the enum itself, like we do in NavTab
                    onClick = { onClickSortType(it) },
                    highlight = (selectedSortType == it),
                )
)

@iByteABit256
Copy link
Contributor Author

That looks good, thanks!

…el, and showing sort type dialogs dynamically according to supported sort types
@iByteABit256
Copy link
Contributor Author

@MV-GH take a look, I did it a bit differently after all

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.

Codewise it looks great! I'll test the functionality later.

In SettingsForm for the AccountSettingsActivity, should only be a able to be set to a sortType that the instance supports. So there you need to use that getSupportedSort types too.

app/src/main/java/com/jerboa/Utils.kt Outdated Show resolved Hide resolved
app/src/main/java/com/jerboa/datatypes/types/Others.kt Outdated Show resolved Hide resolved
app/src/main/java/com/jerboa/datatypes/types/Others.kt Outdated Show resolved Hide resolved
…SortType utils, moved getSupportedSortTypes to the sort type enums, used them in Account settings as well
# Conflicts:
#	app/src/main/java/com/jerboa/ui/components/person/PersonProfile.kt
#	app/src/main/java/com/jerboa/ui/components/person/PersonProfileActivity.kt
@iByteABit256 iByteABit256 requested a review from MV-GH August 1, 2023 17:26
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.

Almost there

app/src/main/res/values/strings.xml Show resolved Hide resolved
@@ -125,7 +124,7 @@ fun SettingsForm(
val sendNotificationsToEmail =
rememberBooleanSettingState(luv?.local_user?.send_notifications_to_email ?: false)
val sortTypeNames = remember {
MAP_SORT_TYPE_SHORT_FORM.values.map { ctx.getString(it) }
SortType.getSupportedSortTypes(siteViewModel.siteVersion()).map { ctx.getString(it.shortForm) }
Copy link
Collaborator

Choose a reason for hiding this comment

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

More needs to be done, currently it selects the right sort type by having this thing the same index as the index in the sorttype. In the future where it would be possible for controversial to be disabled (if downvotes disables). These indexes wont lineup.

A potential solution is keep the list separate. then on the dropdown where you select the sortType you have a onSelect that updates a separate rememberSaveable that stores that index. Then in the form you have default_sort_type = actualSortTypes[actualSortType.value]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm a bit confused by which indexes won't lineup. The one index is the one in the supported sort types list, which is the other one?

What I was thinking about this is to add a second parameter to getSupportedSortTypes with the enableDownvotes boolean, but if I do this now I might as well implement the actual hiding too.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That list of names gets fed to the dropdownsetting which sets the intsettingstate when you select a name. which is the index of that list

Then in form it uses that index to select a sorttype. If we use the index to get it from the filtered list instead of all the sorttypes then we can pass sortType.ordinal to the form. This way it will always have the right index.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe it's a bit of a clunky solution, I made it so that the sort type returned by the backend is matched to the supported types using indexOf, and then I pass the supported sort type to the form using the index returned by the dropdown.

What do you think?

…hort form which is the same, removed the text field from the SortType enum and used the long form in the Dialogs forms
# Conflicts:
#	app/src/main/java/com/jerboa/ui/components/person/PersonProfileActivity.kt
@iByteABit256
Copy link
Contributor Author

You dont need to add twice the same string

I removed the dialogue one, and I also removed the text field from the enum and I used the long form in the dialogue form instead

@iByteABit256 iByteABit256 requested a review from MV-GH August 3, 2023 15:06
@MV-GH
Copy link
Collaborator

MV-GH commented Aug 23, 2023

Could you rebase from main? @dessalines This isn't really a breaking change as it keeps the current behaviour for the current version (inline with the API rework).

I am kinda pushy about this because, this removes the hardcoded sorts we already have in place.

@dessalines
Copy link
Member

Mmk, I'll start that now.

@MV-GH MV-GH merged commit 8da2465 into LemmyNet:main Aug 24, 2023
@iByteABit256 iByteABit256 deleted the controversial-posts-and-comments branch September 12, 2024 08:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Controversial sorting for posts and comments
3 participants