-
Notifications
You must be signed in to change notification settings - Fork 7
QuickStart Apps - Android Tasks app #1
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
Conversation
…apt to newer dependencies
…ll list items are visible
…ch task row in the tasks list
S4H
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll have to defer to @phatblat to review the android app as that is outside of my wheelhouse but I was able to easily follow the readme and build the application without issue so can say it passes the dummies check :)
android/README.md
Outdated
|
|
||
| ### Adding the Ditto SDK | ||
|
|
||
| At the bottom of `app/build.gradle.kts`, you will see this line that causes Android Studio to automatically download the Ditto SDK from Maven Central and add it to the project: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we run a formatter on this file? It's a little jarring having inconsistant line lengths.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we can. Different Markdown processors handle list items differently, and adding newlines to break them into shorter lines might not be handled consistently. I think GitHub-flavored Markdown has changed the way it handles newlines more than once.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've fixed the line wrapping on a couple of paragraphs.
| viewModelScope.launch { | ||
| try { | ||
| val item = ditto.store.execute( | ||
| "SELECT * FROM tasks WHERE _id = :_id", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this DQL query include NOT deleted or deleted != true in the WHERE clause?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would only matter if another device deletes the task between the time a user taps the Edit button and the Edit Task screen appears, in which case the user will be editing a just-deleted task. When they tap Save, the task's properties will be edited, but will also be deleted.
But I agree we should include the WHERE clause for correctness.
| androidComponents { | ||
| onVariants { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't used this type of config before but it looks like a good way to reduce duplication when you're using the same properties in every build variant.
| compose = true | ||
| } | ||
| composeOptions { | ||
| kotlinCompilerExtensionVersion = "1.5.14" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not something that needs to be done in this PR, but since the kotlin compiler version will need to change with the kotlin dependencies, this would be a good value to extract into the version catalog. There are several examples in the Ditto Kotlin SDK of using the version catalog for arbitrary version numbers:
https://github.com/getditto/ditto/blob/ba44fb0f182fe5274f976d0462ec7cfef421916f/android/ditto/build.gradle.kts#L32-L33
https://github.com/getditto/ditto/blob/ba44fb0f182fe5274f976d0462ec7cfef421916f/android/gradle/libs.versions.toml#L23-L28
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll review that. I actually had a few problems with getting a compatible set of versions of the Kotlin compiler, gradle, Android compileSdk setting, and other tools, so I've probably overlooked an easy way to keep them in sync.
| viewModelScope.launch { | ||
| try { | ||
| val doc = ditto.store.execute( | ||
| "SELECT * FROM tasks WHERE _id = :_id", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this query need to filter out deleted tasks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it should. However, similar to the earlier case, the task would only be displayed in the UI if it is not deleted, so this would only matter if another device deletes it during the interval between the last observer notification and the app responding to the toggle click.
|
Everything looks great in this PR. I left a couple comments and minor questions. |
…es, to avoid changes to deleted items
For more details, see QuickStart Apps Android Task App Development Notes