Page MenuHomePhabricator

Reboot the Android client
Needs RevisionPublic

Authored by TCGman on Sun, May 12, 5:12 PM.

Details

Summary

WildFyre Android client base proposal

Summary

This is the current state of my attempt my making an Android client for WildFyre.
Since the original repository I started working on was a fresh new one, I put my code in this one using git replace --graft.
This means this diff would be a complete reboot of the codebase.

The 3 main parts are going to be the following:

  • Current features
  • Project dependencies
  • Code organization

Current features

Logging in and logging out

Users can use their username and password to receive an authentication token. Only the token is stored, in the application preferences for simplicity. Logging out will remove the token from storage and bring back the user to the login screen.

User interface

The user interface can be divided in 3 parts:

  • The toolbar at the top, containing (when necessary) a spinner to switch between areas, and displaying a badge with the user's notification count (if any)
  • The navigation panel on the left, allowing the user to, from top to bottom:
    • see and edit their profile bio and picture
    • navigate through the app (obviously)
    • changing settings
    • visit various parts of the WildFyre website such as Terms and conditions or Privacy policy
    • logout
  • The central zone, where content is displayed
Profile editor

The profile editor is a simple popup allowing the user to change their bio and choose a picture as their profile avatar.
The client checks the size of the picture to disallow images above 512k.
Only documents on the phone can be selected; no support for taking a picture with the camera has been implemented.

Navigation destinations
  • Home, where new posts are to be shown
  • Notifications, listing the user's notifications, and allowing them to clear all notifications
  • Archive, listing the posts the user is subscribed to
  • My posts, listing the user's own posts
Settings
  • Theme, allowing the user to switch between a light and a dark theme (dark mode ftw)
  • Notification badge, allowing the user to toggle the notification badge on the toolbar
WildFyre links

The links are the same that can be found in the web client: About us, Open source, FAQ, Terms and conditions, and Privacy policy.

Post lists: notifications, archive, own posts

3 out of the 4 destinations are roughly the same: Notifications, Archive and My posts.
The post are listed as previews containing part of their content, their author (unless the post is anonymous), as well as a subtitle.
In the case of notifications, the subtitle is the number of unread comments. In the case of archive posts or the user's own posts, the subtitle is the date at which the post was created.

Clicking on any post preview will display the post's full content as markdown.

Some design choices

Navigation menu replacing the bottom navigation

The bottom navigation is faster to use than having to open the left menu first, but has a major drawback: it permanently takes up vertical space.
Vertical space is also going to be used for extenguish/ignite buttons; and in general, a maximum amount of space should be reserved for the content rather than what's surrounding it.

Inverted icon colors

The icon of the current WildFyre application is the WildFyre logo in red/orange on a white background.
The icon used in this code is a white logo on an orange background instead. It looks at least 20% cooler this way.

Project dependencies

Kotlin

The application is written entirely in Kotlin, so it naturally depends on the standard Kotlin library.

  • org.jetbrains.kotlin:kotlin-stdlib-jdk7
  • org.jetbrains.kotlin:kotlin-reflect

Androidx, Google material

Standard libraries providing widgets conforming to material design and backwards-compatibility APIs.

  • androidx.appcompat:appcompat
  • androidx.core:core-ktx
  • androidx.constraintlayout:constraintlayout
  • androidx.fragment:fragment-ktx
  • androidx.recyclerview:recyclerview
  • androidx.vectordrawable:vectordrawable
  • com.google.android.material:material
Lifecycle

Used for building ViewModels from the MVVM architecture principles.

  • androidx.lifecycle:lifecycle-extensions
  • androidx.lifecycle:lifecycle-viewmodel-ktx
Navigation

Used for creating the navigation graph and linking it to the navigation view.

  • androidx.navigation:navigation-fragment-ktx
  • androidx.navigation:navigation-ui-ktx

Glide

Used to easily load and transform images from the web such as profile avatars.

  • com.github.bumptech.glide:glide
  • com.github.bumptech.glide:okhttp3-integration

GSON, Retrofit

Used for interfacing with the server API in place of the WildFyre cross-platform library for now.

  • com.google.android.gms:play-services-base
  • com.google.code.gson:gson
  • com.squareup.retrofit2:retrofit
  • com.squareup.retrofit2:converter-gson

Markwon

Used for displaying posts content as markdown.

  • ru.noties.markwon:core
  • ru.noties.markwon:ext-strikethrough
  • ru.noties.markwon:ext-tables
  • ru.noties.markwon:image-okhttp
  • ru.noties.markwon:recycler
  • ru.noties.markwon:recycler-table

Code organization

The application loosely follows the MVVM architecture, as per Google now recommends for Android apps.
The code hierarchy somewhat reflects it:

  • net.wildfyre.client.data contains code related to data fetching and server API calls.
  • net.wildfyre.client.viewmodels contains ViewModels keeping runtime data independenly from the UI.
  • net.wildfyre.client.views contains UI classes: the main activity and its fragments.

This architecture separates layers so that each of them only accesses a single other layer: The UI elements use their viewmodels, the viewmodels use the repositories, and the repositories use the services.
This architecture disallows things like, for example, an activity or fragment directly making API calls using the web service.

Another design element following Google's recommendations is the usage of LiveData and a reactive pattern.
Instead of requesting data and then expecting the results, data is observed, and change requests are independent from observation.

Conventions for the following paragraphs:

  • I: interface
  • A: abstract class
  • C: concrete class implementation
  • O: object
  • M: multiple classes/objects/functions

Global scope classes

TypeFileDescription
OConstants.ktCentralized place to store constant values used by other classes
CApplication.ktApplication subclass doing application scope initializations

views package

TypeFileDescription
IAreaSelectingFragment.ktFragment for displaying an area spinner in the menu
AFailureHandlingActivity.ktActivity implementing FailureHandler (shows a toast with the failure's error message)
AFailureHandlingFragment.ktFragment implementing FailureHandler (shows a toast with the failure's error message)
AItemsListFragment.ktFragment displaying a list of post previews that can be clicked to show the full post
APostsFragment.ktFragment extending ItemsListFragment and implementing AreaSelectingFragment to serve as base for ArchiveFragment and OwnPostsFragment
CMainActivity.ktThe only activity instantiated the project, hosting the base UI (toolbar, navigation view) as well as fragments for displaying content
CLoginFragmentFragment requiring the user to authenticate themselves, and navigating to HomeFragment when the user has done so
CHomeFragment.ktFragment for displaying new posts to the user
CNotificationsFragment.ktFragment displaying previews of the user's notifications
CArchiveFragment.ktFragment displaying post previews from a user's archive
COwnPostsFragment.ktFragment displaying previews of the user's own posts
CPostFragment.ktFragment displaying the full content of a single post to the user

Quick visualization of fragment classes hierarchy:

├─FailureHandlingFragment           ├─AreaSelectingFragment
│                                   │
├─────LoginFragment                 │
│                                   │
├─────HomeFragment──────────────────┤
│                                   │
├───┬─ItemsListFragment             │
│   │                               │
│   ├─────NotificationsFragment     │
│   │                               │
│   └───┬─PostsFragment─────────────┘
│       │
│       ├─────ArchiveFragment
│       │
│       └─────OwnPostsFragments
│
└─────PostFragment
views.adapters subpackage
TypeFileDescription
AItemsAdapter.ktBase adapter for RecyclerViews used by ItemsListFragment implementations
CNotificationsAdapter.ktAdapter used by NotificationsFragment
CPostsAdapter.ktAdapter used by PostsFragment implementations
views.markdown subpackage
TypeFileDescription
CMarkdownRecyclerView.ktRecyclerView ensuring that a post's images are always properly loaded
CPostPlugin.ktMarkwon plugin that forces newlines to be honored and makes images grow as large as possible
MUtils.ktContains a function replacing the [img: 42] image pattern with a markdown image pattern

viewmodels package

ViewModel classes mostly just hold and transform data for usage by activities and fragments.
They provide data fetched by repositories in a way that can be used in XML layouts with databinding.

data package

TypeFileDescription
IFailure.ktType used for representing errors occuring during operations such as API calls
CFailureHandler.ktInterface used to help failures bubble their way from the data layer up to the UI layer
MModels.ktModels used in the server API, as simple classes without any method
MRepositories.ktSingletons providing access to data independently from the service used. They abstract data aggregation from the rest of the app
MServices.ktServices (although there is only one for now) able to fetch the data needed by the app. The only service implemented is the web service connecting to api.wildfyre.net. It uses Retrofit in place of the WildFyre library until it is ready to be used
Test Plan

Test the proposed client base and point out what's wrong

Diff Detail

Repository
rANDROID Android Client
Branch
client-reboot
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 677
Build 677: arc lint + arc unit

Event Timeline

TCGman created this revision.Sun, May 12, 5:12 PM
TCGman edited the summary of this revision. (Show Details)Sun, May 12, 5:13 PM
TCGman added reviewers: CLOVIS, WyldBot, Info-Screen.
Hackintosh5 requested changes to this revision.Sun, May 12, 7:45 PM

I think you should focus on using wflib rather than rewriting the array.

This revision now requires changes to proceed.Sun, May 12, 7:45 PM

Imo wflib isnt stable enough

Well first, wow that's pretty nice.

I'm fine with this until the lib is ready, so here are a few things to be taken into account so migration from Retrofit to the lib will be easy:

  • The lib will also use Ktor to handle HTTPS. This is important because Ktor is multiplatform, so it allows to have everything done once and then all platforms will work the same. AFAIK Ktor uses Gson for the JVM target, but I'm not 100% sure. If that's the case, since you used Gson too, it won't be too hard to change.
  • Because of the Ktor usage, everything network related will be done with coroutines. They were just announced to finally be stable, so it's pretty convenient for us. Because of this we will have virtually no threading to do Android-side, so I don't think we will need LiveData. Still, pretty nice that you implemented it.

Don't change anything related to that for now though, the lib will hide most details away; but at least now you know what changes will be needed (and maybe you can build your version while keeping that in mind to make the migration easier).
Before I can say if I agree with this diff or not I will have to test it, but I really don't have any time right now, sorry :/

Also, I don't think I like that you rewrote the Git history... I'd rather keep the history, delete everything you don't need, and go from there (especially because the automated publishing of releases via Git & Gradle is extremely convenient).

WyldBot, why not fix the library then? I was going to do that but y'all told me the server isn't stable yet. So why aren't we versioning the api server endpoints?

Info-Screen added 1 blocking reviewer(s): Info-Screen.Mon, May 13, 10:48 AM

Large one will take some time to review.

TCGman updated this revision to Diff 913.Mon, May 13, 4:04 PM
TCGman edited the summary of this revision. (Show Details)

Automated release, upgrading, automated version number, private signing

Technically I didn't "rewrite" the Git history; I just added mine on top of it... But it did erase every file, yes.
I've updated the revision after cherry-picking your commit adding automated versioning, and updated the plugin to its latest version.

In D251#6235, @TCGman wrote:

Technically I didn't "rewrite" the Git history; I just added mine on top of it... But it did erase every file, yes.

Oh, sorry I misunderstood. Well, rewriting everything is fine as long as it makes us move forward. Sorry I don't have time for review right now, plus this one will probably be quite long.

WyldBot requested changes to this revision.Tue, May 14, 3:33 AM

Gitignore all .idea folders/files
You have 2 gitignore files, should only be 1

app/src/main/ic_launcher-web.png
app/src/main/res/mipmap-xxxhdpi/ic_launcher_round.png (and all the others)

Arent using the correct hue of orange, should be #ed763e

Lots of files dont have new lines at the end of them, use editorconfig for this

Markwon will need to be tested for security vulnerabilities

app/src/main/java/net/wildfyre/client/Constants.kt
47

Possibly use template literals here and in our other places where you do concatenation

app/src/main/java/net/wildfyre/client/data/Models.kt
7

I dont think body should start with an underscore here

app/src/main/java/net/wildfyre/client/data/Services.kt
99

This endpoint doesnt always require authentication

235

This endpoint doesnt always require authentication

app/src/main/java/net/wildfyre/client/viewmodels/LoginFragmentViewModel.kt
12

Username and password should never be stored

app/src/main/res/values-night/colors.xml
4

Will need to make sure we use WildFyre grey here: #263238

app/src/main/res/values/strings.xml
1–2

File should likely be renamed strings_en.xml to allow for future translation

This revision now requires changes to proceed.Tue, May 14, 3:33 AM
TCGman updated this revision to Diff 920.Tue, May 14, 9:22 AM
  • Remove unnecessary authentication token usage
  • Use template literals
  • Merge .gitignore files, fully ignore /.idea
  • Use official WildFyre orange hue
  • Add .editorconfig, reformat project
  • For _body: this came from webcl, where it seemed to be where error messages are stored
  • WildFyre grey: it would require adapting the rest of the color palette, it's too blue-ish compared to the rest of the app in night mode
  • strings.xml: Android needs default values, so that one should not change (the app doesn't build without a default strings.xml file)
  • Username and password: they stay in memory only, and the LoginFragmentViewModel is disposed of as soon as the login step is finished and the LoginFragment is destroyed; these variables won't keep credentials info any longer than the UI EditTexts themselves will anyway
In D251#6261, @WyldBot wrote:

Gitignore all .idea folders/files

Better not to, instead use this.

CLOVIS added inline comments.Tue, May 14, 10:34 AM
app/.editorconfig
1 ↗(On Diff #920)

Shouldn't this be in the project root?

app/src/main/java/net/wildfyre/client/Application.kt
12 ↗(On Diff #920)

Are you sure you want to name your child class Application too?

14 ↗(On Diff #920)

Can you put a comment that explicitly tells that this is initialized in onCreate?

app/src/main/java/net/wildfyre/client/Constants.kt
23

Please reformat to get the else on a line beginning

app/src/main/java/net/wildfyre/client/data/FailureHandler.kt
12

Since this is Kotlin, we don't need functional interfaces. Maybe this could be replaced by something like:

typealias FailureHandler = (Failure) -> Unit

app/src/main/java/net/wildfyre/client/data/Models.kt
7

I second this, the only place I've seen underscores in names was for private properties to avoid name clashing, but that doesn't seem to be the case here.

11

id cannot be null, maybe you meant to use lateinit?

15

Why use inner classes?

app/src/main/java/net/wildfyre/client/data/Repositories.kt
27

This probably should be a non-backed property with a single setter

32

Maybe use a property here

47

Merge this with setAuthToken below as a single property with a setter than accepts null to unset the value?

59

Probably should be a property

108

Should probably be a property

128

I understand an Exception probably needs to be thrown here, but I don't think NullPointerException is the best choice if we're to catch it and recover from it

202

The NPEs seem dangerous here

232

Suspicious NPE

239

Suspicious NPE

app/src/main/java/net/wildfyre/client/data/Services.kt
38

Suspicious NPE

app/src/main/java/net/wildfyre/client/viewmodels/MainActivityViewModel.kt
29

Why make two different properties, since they hold the same reference anyway?

67

Suspicious NPE

app/src/main/java/net/wildfyre/client/views/AreaSelectingFragment.kt
94

Suspicious NPE

104

Suspicious NPE

app/src/main/java/net/wildfyre/client/views/ItemsListFragment.kt
117

Suspicious NPE

124

Empty method

138

NPE

app/src/main/java/net/wildfyre/client/views/LoginFragment.kt
61

Explicit return

64

Explicit return

96

NPE

app/src/main/java/net/wildfyre/client/views/MainActivity.kt
2

(i'm stopping here for today)

In D251#6285, @TCGman wrote:
  • For _body: this came from webcl, where it seemed to be where error messages are stored

Can you elaborate? I don't understand what you mean

CLOVIS requested changes to this revision.Tue, May 14, 10:40 AM
This revision now requires changes to proceed.Tue, May 14, 10:40 AM
TCGman updated this revision to Diff 921.Tue, May 14, 12:26 PM
  • Tune ignored files according to Jetbrains recommendations
  • Move .editorconfig to project root
  • Rename main Application class to WildFyreApplication
  • Comment on WildFyreApplication instance
  • Better format AUTOMATIC theme definition
  • Add explicit returns
  • Reduce potential NPEs
  • Explain null assertion usage in then extension function
  • Remove useless ApiItem parent class
  • Move inner model classes to global Models.kt scope
TCGman added inline comments.Tue, May 14, 12:57 PM
app/src/main/java/net/wildfyre/client/data/FailureHandler.kt
12

FailureHandler interface is meant to be implemented by other classes, this is why I made it an actual interface

app/src/main/java/net/wildfyre/client/data/Models.kt
7

I saw it in files like https://phabricator.wildfyre.net/source/webcl/browse/master/src/app/_services/post.service.ts, but I probably misunderstood it. I removed it since I didn't use it anyway. If results from the API can contain errors, it should be added again later on

11

All properties of classes in Models.kt are nullable because I wasn't sure which would be mandatory or not; https://phabricator.wildfyre.net/source/webcl/browse/master/src/app/_models/area.ts has rep and spread but they aren't actually filled when fetching the area so it made me a bit paranoïd when writing Models.kt

15

The inner classes in that same file were an attempt at organizing related classes, but it does look weird more than anything else.

app/src/main/java/net/wildfyre/client/data/Repositories.kt
27

I don't quite see how to make properties out of the functions you point out (maybe it's just my lack of proper Kotlin knowledge and experience here)

47

setAuthToken is private and should only be used by AuthRepository itself when fetching the token. clearToken on the other hand should be accessible from the outside, so it's public

128

There are indeed lots of places that you pointed out where this can be avoided; however it should always be safe to use authToken, it's initialized to at least an empty string in AuthRepository, and never set to a null value.

app/src/main/java/net/wildfyre/client/data/Services.kt
38

Didn't change this one, I think it shouldn't pose a problem. I added a small comment explaining a bit more though.
The result given to the callback can only be null if the API call has no result, in which case [T] will be [Unit] and thus the second overload will be used instead.

app/src/main/java/net/wildfyre/client/viewmodels/MainActivityViewModel.kt
29

It's for use in the XML layout wiht databinding; the number is used to know if there are any notifications at all, the text counterpart is what's going to be displayed in the badge in the Toolbar (that's why it stops at 99). Maybe notificationCountText could be compared with "0" directly in the XML, but I tried and it didn't compile :/

app/src/main/java/net/wildfyre/client/views/ItemsListFragment.kt
124

That's just because of inheriting from RecyclerView.OnChildAttachStateChangeListener, I have to implement this method

CLOVIS added inline comments.Wed, May 15, 8:08 AM
app/src/main/java/net/wildfyre/client/data/FailureHandler.kt
12

Hm is it important to use an interface instead of a Lambda? Since it only represents an action I think it's cleaner to use a Lambda here (or maybe I've overlooked some usage)

app/src/main/java/net/wildfyre/client/data/Models.kt
11

Okay, please discuss this with @Info-Screen he will tell you which are mandatory and which are not. It's better to this before we land this, otherwise any change in the future will be breaking. And using nullables everywhere will surely lead to a surprising NPE one day if you're forced to use !! everywhere

15

You can use:

//region Something
//endregion

This creates a custom folding region in Android Studio and JetBrains IDEs, and we already use that a lot in the lib ^^

In particular, they appear in the "structure" tab which makes navigation easy

app/src/main/java/net/wildfyre/client/data/Repositories.kt
27

You could write something like:

var theme: Int
  get = //something here
  set(value) {WildFyreApplication... //Your code here}

This is a non-backed property because neither the getter nor the setter use field, so the compiler doesn't generate an variable for it. If you look at the Java code (Tools › Kotlin › Show Bytecode › Decompile) you will see it only generates a setter and a getter but no attribute.

This allows to use SettingsRepository.theme = ... everywhere instead of SettingsRepository.setTheme(...) which is easier to read and a bit cleaner in my opinion

Plus, it's a bit infuriating to use getters and setters in Kotlin when they are integrated into properties

The basic idea would be to replace everything that is a getter or setter by a property that combines the two

47

I missed that it was private, nevermind

128

You should be able to make authToken not null then, and every !! can go away

But using "" is a bit risky too because if for some reason it's not initialized we will make a request and get back a "server refused the credentials" or something similar and I don't think we'll think to check the token was not empty then

app/src/main/java/net/wildfyre/client/data/Services.kt
38

Instead of !! use ?: throw Exception ("your explanation here")

This way if for some reason it ever happens, we will know what the problem is and why it's not supposed to happen instantly, this will likely make debugging a lot faster

That's why I have a ProgrammingException in the lib, I throw it everytime there's an impossible situation with an explanation so that if it ever happens the lib user will be able to report it easily

app/src/main/java/net/wildfyre/client/views/ItemsListFragment.kt
124

Maybe put the brackets on the same line so it's obvious it's meant to be this way:

override fun ... {}

Tbh that's a bit picky so it's fine this way too

WyldBot requested changes to this revision.Sun, May 19, 1:42 PM
This revision now requires changes to proceed.Sun, May 19, 1:42 PM