thunder-app / thunder

Thunder - An open-source cross-platform Lemmy client for iOS and Android built with Flutter
https://thunderapp.dev
GNU Affero General Public License v3.0
732 stars 62 forks source link

Support for discussion languages #1289

Closed iparks99 closed 1 month ago

iparks99 commented 1 month ago

Pull Request Description

I added an additional section to the Account Settings -> General for discussion languages. Posts that are not of the filtered languages will not be displayed in the user's feed.

This is my first PR, and one of my first projects in Dart/Flutter. Any advice and tips are greatly appreciated!

Issue Being Fixed

As described here, we'd ideally be able to filter out posts/communities in languages we don't understand.

Issue Number: #744

Screenshots / Recordings

account_settings add_language remove_language remove_undetermined

Checklist

hjiangsu commented 1 month ago

Hey @iparks99, thanks for the contribution! I just took a closer look at the issue, and there's some additional information that wasn't conveyed properly in the initial issue report (the issue was created a while back, and I've gained more context into how it works now).

Within Lemmy, we can select discussion languages in the Settings page. If this setting is applied, the API will automatically filter out posts based on the language of the post. This eliminates the need for us to filter out posts in our logic.

image

We can take advantage of this, and let the user change this setting directly from within Thunder. This has the benefit that it applies to their account (and thus, is not app-dependent). I believe the best place to put this setting is in Account Settings (UserSettingsPage)

simulator_screenshot_C280E538-F1CA-455C-BDEA-036B9CA4299D

Additionally, we have a language selector dialog that could work nicely here (which I noticed you used!): showLanguageInputDialog in input_dialogs.dart.

I believe the existing examples (user/community/instance blocks) would have similar implementations so you can take a look at how they work in order to implement this setting. The one disadvantage here is that this only applies to logged in users. I do think we can limit the scope of this to logged in users for now and revisit it for anonymous users in the future (which would most likely require a refactor on how we handle anonymous users)

If you have any more questions, feel free to join the matrix room here: https://matrix.to/#/#development_thunder:matrix.org since I'll be able to answer questions faster there!

Also, if @micahmo doesn't mind, you can also ping him as well for any questions as he's pretty familiar with the codebase.

iparks99 commented 1 month ago

Thanks for the feedback @hjiangsu! Hopefully just finished getting this working based on your comments. Let me know what you think when you have a moment.

iparks99 commented 1 month ago

Sorry for last minute force pushed rebases and all the associated actions!

iparks99 commented 1 month ago

@micahmo okay, I think all the feedback has been addressed! Let me know what you think!

micahmo commented 1 month ago

@iparks99 It looks great!! Thank you so much for addressing all the feedback! 🎉

I only noticed one small glitch. After acknowledging the "Undetermined" warning, the dialog doesn't seem to disappear.

https://github.com/thunder-app/thunder/assets/7417301/89609669-07c1-4056-be76-e2fac0531546

iparks99 commented 1 month ago

@micahmo Fixed! Thanks for pointing that out, I must've missed the Navigator pop when moving that code block around.

hjiangsu commented 1 month ago

Sorry for getting back to this a bit late! I think it looks really good - thanks for doing this @iparks99 😄

I tested it out, and it seems to be working pretty well! Just had a couple of interesting observations:

https://github.com/thunder-app/thunder/assets/30667958/6fdfc4a2-7b1e-4004-b468-936a7051f331

https://github.com/thunder-app/thunder/assets/30667958/3134c211-0df6-4146-aa14-f229cbc86bd5


This being said, I think the only actionable thing we can do here is hide the "No language" entry. Aside from that, I think this is good to go!

iparks99 commented 1 month ago

@hjiangsu Just added the parameter :)

hjiangsu commented 1 month ago

Looks good! Thanks again 😄