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

Creates a screen to allow the unblocking of users, communities and instances #1483

Open
wants to merge 20 commits into
base: main
Choose a base branch
from

Conversation

rodrigo-fm
Copy link
Contributor

@rodrigo-fm rodrigo-fm commented Apr 24, 2024

This is my biggest PR on jerboa so far, users now have a dedicated screen to unblock anything they want, here's a video demonstrating:

unblock-demo.mp4

P.S: I also updated the AGP version

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!

I'll give it a proper review later. But can you fix the conflicts. Diff is hard to read atm

That delete icon, is it a M3 icon? I would like to keep the same style across the app

app/build.gradle.kts Outdated Show resolved Hide resolved
gradle/wrapper/gradle-wrapper.properties Outdated Show resolved Hide resolved
@OptIn(ExperimentalMaterial3Api::class)
@Composable
fun BlocksActivity(
siteViewModel: SiteViewModel,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would make a viewModel for this screen. Keep the UI and data logic separate.

Copy link
Member

Choose a reason for hiding this comment

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

Absolutely, but I'd also add the block actions to the SiteViewModel rather than creating a new one, so that it can update the blocks. We may also need to change / fix some other block actions, such as CommunityViewModel.blockCommunity, so that it can stay in sync with SiteViewModel.my_user.community_blocks... etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dessalines I don't think I understand the idea of adding the block actions to the SiteViewModel. I had to create an apiState for each element on the list to avoid the same state being displayed to every element (this would cause the loading state to be displayed on each element, for instance).

Copy link
Collaborator

@MV-GH MV-GH May 5, 2024

Choose a reason for hiding this comment

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

Add what you can to the blockviewmodel. I don't understand what you would need a apistate per block Ah you need a Loading state per element? I would probably look into having a list for this?

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 you suggest a list of ApiStates inside the BlockViewModel? That's interesting

Copy link
Collaborator

@MV-GH MV-GH May 5, 2024

Choose a reason for hiding this comment

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

Well or a BlockListEntryViewModel and have a ViewModel per entry.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

As @MV-GH mentioned this needs to be reworked to follow the conventions of all the other examples: ViewModel, Activity, and read-only composables in their own file. IMO it makes sense to add the block responses to the siteViewModel, that makes sense, so that they can update those values without having to refetch the site.

Start with an existing somewhat similar example like ReportsActivity.kt, copy-paste the entire folder, then change as necessary.

edit: one more thing, add this screen to within the SettingsActivity, IE where look and feel and your user settings are. For now you can put it right below my user name settings, but in the future we'll rework this to put it within that my_user_name settings screen, since that needs to be reorganized.

app/src/main/java/com/jerboa/Utils.kt Outdated Show resolved Hide resolved
app/src/main/java/com/jerboa/Utils.kt Outdated Show resolved Hide resolved
app/src/main/java/com/jerboa/feat/Block.kt Show resolved Hide resolved

@Composable
private fun BlockedElementListItem(
apiState: ApiState<*>,
Copy link
Member

Choose a reason for hiding this comment

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

Don't have this take in the api state. The standard is for composables is to be separated from API calls, only take in the data structures, and then push up any events to the top-level activity.

Move this composable to its own file, Blocks.kt, and make sure this file only contains what's necessary for the activity, not composables.

Check out ReportsActivity.kt and Reports.kt for an example of how we separate these.

Copy link
Member

Choose a reason for hiding this comment

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

None of this is necessary.

@dessalines
Copy link
Member

Sry, been really busy lately. I'll try to get to this sometime this week.

Comment on lines +128 to +131
val personBlocks = siteRes.data.my_user?.person_blocks?.toMutableStateList()
val communityBlocks =
siteRes.data.my_user?.community_blocks?.toMutableStateList()
val instanceBlocks = siteRes.data.my_user?.instance_blocks?.toMutableStateList()
Copy link
Collaborator

@MV-GH MV-GH May 9, 2024

Choose a reason for hiding this comment

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

Needs to be in the viewmodel.

image

The onSuccessfulUnblock needs to be removed from the UI. Instead the ViewModel updates the SnapShotList (on Block event). The UI will react to this change and update. Dont use a list, use mutableStateList and only expose SnapShotList (Its what I am doing with changes to the feed, atm we create a new list there constantly but causes the entire feed to recompose each time)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would be a different ViewModel? The ones I created so far are ViewModels for each list element. This suggestion feels like a ViewModel that contains every element on the list.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes that was what I had in mind. But if you have a solution that works with viewmodel per element you can do that too. But I think it would be a lot simpler if you used just one viewmodel.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, a BlocksViewModel sounds good. I'll see later what I can do about the unique viewmodel that represents a list item

}

item { Spacer(modifier = Modifier.padding(vertical = MEDIUM_PADDING)) }
item { Title(stringResource(R.string.blocked_instances)) }
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you want https://developer.android.com/develop/ui/compose/lists#sticky-headers

Might look interesting for the subtitles

@rodrigo-fm
Copy link
Contributor Author

Hey, sorry for being away for weeks, had other things to take care of. I'll be marking the comments addressed by the commits as resolved.

@MV-GH
Copy link
Collaborator

MV-GH commented Jun 10, 2024

No problem, but in the meantime I was working on this feature too. I'll post my PR soon.

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.

None yet

3 participants