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 ability to Ban and Ban from Community for posts and comments. #1325

Merged
merged 20 commits into from
Jan 25, 2024

Conversation

dessalines
Copy link
Member

@dessalines dessalines commented Jan 20, 2024

Edit: Should come after #1324

@dessalines dessalines marked this pull request as draft January 20, 2024 16:25
@dessalines dessalines marked this pull request as ready for review January 21, 2024 04:52
* A wrapper to store extra community ban info
*/
@Serializable
data class BanFromCommunityData(
Copy link
Member Author

Choose a reason for hiding this comment

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

This is necessary to bundle some info for community bans. site bans don't need this cause PersonView has all the data you need.

@@ -281,7 +284,8 @@ fun commentsToFlatNodes(comments: List<CommentView>): ImmutableList<CommentNode>
*/
fun buildCommentsTree(
comments: List<CommentView>,
rootCommentId: Int?, // If it's in CommentView, then we need to know the root comment id
// If it's in CommentView, then we need to know the root comment id
Copy link
Member Author

Choose a reason for hiding this comment

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

kotlinter complains about all same-line comments now, so I fixed these up.

person: Person,
): List<PostView> {
val newPosts = posts.toMutableList()
newPosts.replaceAll {
Copy link
Member Author

Choose a reason for hiding this comment

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

I couldn't find a better way to alter several items for an immutable list.

I would've liked to use .filter { }.forEach { }, but it didn't work since the internal items are immutable.

@@ -1568,3 +1633,9 @@ fun canMod(
false
}
}

fun futureDaysToUnixTime(days: Int?): Int? {
Copy link
Member Author

Choose a reason for hiding this comment

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

Copied from lemmy-ui. The back end requires a unix epoch, but the front end should just use days in its fields.

@@ -267,6 +268,62 @@ class PostViewModel(val id: Either<PostId, CommentId>) : ViewModel() {
}
}

fun updateBanned(personView: PersonView) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Wasn't sure what to name this one. Its only used for banning a user currently, but could potentially be used for any updates for every person in a post.

@@ -692,7 +705,7 @@ fun CommentFooterLine(
var showMoreOptions by remember { mutableStateOf(false) }

val canMod =
remember {
remember(admins) {
Copy link
Member Author

Choose a reason for hiding this comment

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

Need your help with this one.

This was an error with the last PR. On first app load, it would incorrectly not should the extra mod actions, because admins hadn't loaded yet.

This fixes it, but it might cause a lot of recompositions when admins show up? I'm having trouble deciphering when to use derivedState vs remember(key).

Also, should this remember also include account, IE remember(account, admins) { ...

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm having trouble deciphering when to use derivedState vs remember(key).

derivedState is to prevent recompositions when the result is the same. Here i have rather that the computation (the function call itself) is skipped as much as possible too. So I prefer a remember.

This fixes it, but it might cause a lot of recompositions when admins show up

As long as the key doesn't change it shouldn't cause any recompositions, so as long as the list stays the same it shouldnt be a problem.

Also, should this remember also include account

if this needs to change when the account changes then the account needs to be included in the key. But I think account change would force it to fully rebuild the compose tree due to navigation. So it wouldn't be necessary

Copy link
Member Author

Choose a reason for hiding this comment

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

Gotcha, thx.

OutlinedTextField(
modifier = Modifier.fillMaxWidth(),
value = value?.toString() ?: "",
onValueChange = {
Copy link
Member Author

Choose a reason for hiding this comment

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

Some weird logic here, but it seems to work. Mostly copied from stackoverflow. The expire days needs positive integers, or if its empty, send null.

import it.vercruysse.lemmyapi.v0x19.datatypes.Person

@Composable
fun BanPersonPopupMenuItem(
Copy link
Member Author

Choose a reason for hiding this comment

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

Externalized these to a common composable, since both post and comment items use them. Should maybe do the same thing for post / comment Removes

@@ -228,26 +224,6 @@ fun PostCommunitySelector(
}
}

@Composable
fun CheckboxIsNsfw(
Copy link
Member Author

Choose a reason for hiding this comment

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

I externalized this to a common CheckBoxField composable.

@@ -1236,7 +1236,6 @@ fun dracula(): Pair<JerboaColorScheme, JerboaColorScheme> {
val md_theme_light_inverseOnSurface = Color(0xFFF3F0F4)
val md_theme_light_inverseSurface = Color(0xFF303034)
val md_theme_light_inversePrimary = Color(0xFFB9C3FF)
val md_theme_light_shadow = Color(0xFF000000)
Copy link
Member Author

Choose a reason for hiding this comment

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

Compiler complaints.

@dessalines
Copy link
Member Author

@MV-GH This should probably be the next to merge, since its getting cumbersome to deal with conflicts.

@@ -1569,6 +1634,12 @@ fun canMod(
}
}

fun futureDaysToUnixTime(days: Int?): Int? {
return days?.let {
(Date.from(Instant.now().plus(Duration.ofDays(it.toLong()))).time / 1000L).toInt()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems overly complex

Why not Instant.now().plus(3, DAYS).toEpochMilli() ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thx, looks like this one works:

Instant.now().plus(it.toLong(), ChronoUnit.DAYS).epochSecond.toInt()

Copy link
Collaborator

Choose a reason for hiding this comment

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

There is reason that is a long, this will overflow in 2038

Copy link
Member Author

Choose a reason for hiding this comment

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

👀 Then we'll also need to change that in the api-client also. Rust uses an i64 so it should be fine there.

Copy link
Collaborator

Choose a reason for hiding this comment

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

api-client? lemmy-js-client? thats not gonna be a problem since its a float which has a much bigger range and will only start to lose precision in about 300k years

Copy link
Member Author

Choose a reason for hiding this comment

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

https://github.com/MV-GH/LemmyBackwardsCompatibleAPI/blob/master/app/src/commonMain/kotlin/it/vercruysse/lemmyapi/v0x18/datatypes/BanPerson.kt#L11 and the same for BanPersonFromCommunity

Those could be Long , but if you're auto-generating them, we might need to figure out a smart way to do it, because javascript had too many issues with BigInt when we tried to do that the first time.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm that is annoying, I am guessing what ever tool you use to generate those TS types can't annotate its actual type for numbers?

I'll keep a manual list for now

Copy link
Member Author

Choose a reason for hiding this comment

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

I commented over on your recent pr, but imo you might as well just replace every int with long.

@dessalines dessalines enabled auto-merge (squash) January 25, 2024 17:03
@dessalines dessalines merged commit d1b348e into main Jan 25, 2024
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.

2 participants