`NotificationDao.pagingSource` missed the `reportId` column and
mis-soelled `report_ruleIds`, risking an NPE.
Fix that.
While I'm here, add suggested indices to `NotificationEntity`.
# Status storage
Re-work how statuses are stored and managed to separate the cached home
timeline from the cached notification timeline.
Previously, the home timeline pulled all statuses in `StatusEntity`.
Since that table also includes statuses that are referenced from
`NotificationEntity` this could show the wrong data. It also makes it
difficult to cache other timelines in the future.
To fix this:
- Introduce `TimelineStatus` which associates a given timeline with the
statuses on that timeline.
- Use the the `StatusEntity` table as a general cache of statuses.
wherever they're used.
- Create the home timeline by joining `TimelineStatus` (where the
timeline's kind is `Home`) with the statuses in `StatusEntity`.
This has a number of knock-on effects.
- Deleting from the home timeline now deletes the association from
`TimelineStatus`. The cached status is unaffected, so if it is
referenced from another cached timeline (currently, notifications)
there is no change.
- Modifying a status on one timeline (translating, expanding,
collapsing, etc) modifies it on all timelines that reference that
status.
- `cleanup()` and related functions no longer need to take `limit` or
`keep` parameter, as it's known whether a status is referenced from a
timeline.
Rewriting one of the queries exposed an issue where `TimelineDaoTest`
run locally could return different (incorrect) results to the same test
run on a device (https://issuetracker.google.com/issues/393685887).
So re-implement `TimelineDaoTest` as an `androidTest`, and update the CI
workflow to include a step to run these tests on an API 31 emulator.
# Repositories
- Allow `null` as an initial key.
# Fragments
- Remove unnecessary `refreshAdapterAndScrollToVisibleId`.
TimelineDao contained operations on timelines and statuses which made it
large and confusing.
Factor out the status-specific operations in to the new StatusDao class
to make things more understandable.
TimelineStatusEntity is going to be repurposed for the table that maps
between a timeline (not just the home timeline) and the statuses on that
timeline.
The database queries in the @Query annotations were in a range of
different styles which made them difficult to read, and difficult to
write new ones in a consistent style.
Fix this.
Write a new tool, `sqlfmt`. This processes the DAO files looking for
`@Query(...)` annotations. It extracts the SQL from those annotations
and calls `sqlfluff` (https://github.com/sqlfluff/sqlfluff, which must
be installed separately) to lint and fix formatting issues in the SQL.
The file is re-written with the newly formatted SQL queries.
Previous queries to delete stale data from the database could fail due
to the new foreign key constraints.
Rewrite them so statuses and accounts referenced by cached notifications
are not deleted.
The default migration code copies existing NotificationEntity rows to
the new table. This might fail because of the new FK constraint on
TimelineAccountEntity. Fix this by not copying the data over; it's a
local cache, so nothing of importance is lost.
The modifications to the Notifications* classes highlighted different
(and better) ways of writing the code that manages status timelines.
Follow those practices here.
Changes include:
- Move `pachliAccountId` in to `IStatusViewData` so the adapter does not
need to be initialised with the information. This allows the parameter
to be removed from functions that operate on `IStatusViewData`, and the
adapter does not need to be marked `lateinit`.
- Convert Fragment/ViewModel communication to use the `uiResult`
pattern instead of separate `uiSuccess` and `uiError`.
- Show a `LinearProgressIndicator` when refreshing the list.
- Restore the reading position more smoothly by responding when the
first page of results is loaded.
- Save the reading position to `RemoteKeyEntity` instead of a dedicated
property in `AccountEntity`.
- Fixed queries for returning the row number of a notification or
status in the database.
Fixes#238, #872, #928, #1190
Previous code could crash because of a foreign key constraint between
`TimelineStatusEntity` and `TimelineAccountEntity`.
Specifically, `removeAllAccounts` would remove accounts that were still
referenced by cached notifications or statuses (the statuses were
retained because they were referenced by notifications).
Fix this by only removing accounts that are not referenced by anything.
While I'm here, `NotificationEntity` should have an FK constraint to
`TimelineAccountEntity`, so add that.
Persist the user's notification filtering decisions (i.e., the decision
to show a filtered notification) by caching all notification data,
including the filtering decision, in the database.
## Structure changes
This means re-writing the notification management system to use Room and
the Paging library to manage the notification data.
Implement a repository and remote mediator for notifications that does
this, with knock on effects for the viewmodel and the fragment. Take the
opportunity to rewrite these to reflect (current understanding of) best
practice for state management.
Active account information is included in the viewdata for each
notification when sent to the adapter. This allows the adapter to be
created before the fragment knows the active account from the view
model.
`RemoteKeyDao` is extended to support sorting the "refresh" key for
a timeline. This is used to persist the notifications refresh key
instead of the `lastNotificationId` property in the account (which has
been removed).
## UX changes
A linear progress bar is used to show progress when notifications are
refreshed, as part of the ongoing effort to migrate the UI.
Previous code managed account deletions by having specific functions to
call when an account was deleted that would delete all related data.
Replace this with proper foreign key references back to the account ID,
and cascade account deletes to the related data.
Add tests to ensure the deletes happen as expected. Update existing
tests to create an account where necessary so the new foreign key
constraints are kept.
A few places in the code were calling `moshi.adapter` to marshall
to/from strings in the database where type converters either already
exist, or are straightforward to create.
Create the missing type converters, and use them throughout. This
simplifies several places where a Moshi instance no longer needs to be
passed through several layers of method calls.
Since this doesn't change the underlying database representation of the
data there's no need to bump the database version number.
Allow the user to define filtering rules for notifications by sending
account:
- Not followed
- Younger than 30d
- Limited by moderators
and a policy for each of either show, warn, or hide.
To do this:
## Manage followers
- Create a new `FollowingAccountEntity`, to record accounts the logged
in account is following.
- Fetch the account's followers when an account is made active, and
persist to this table.
- Provide the followers as a property on `PachliAccount`
- Update this table if the user follows/unfollows accounts during normal
operation.
## Track account creation time
- Record account creation time in `TimelineAccount`.
## Track notification creation time
- Record notification creation time in `Notification`.
## API
- Always fetch all notifications, including those the server is
filtering.
## UX and storage for account filters
- Show a new Account preference to edit account notification filters.
- Display a dialog to manage account notification filters.
- Persist the user's choice to new properties in `AccountEntity`.
- New `AccountManager` methods to update the properties
## Filtering notifications
- New `NotificationFilter.filterNotificationByAccount()` method to make
the filtering decision based on the user's preferences.
- Use this in `NotificationFetcher` to filter notifications before
creating Android notifications.
- Use this in `NotificationsViewModel` to filter notifications before
display in `NotificationsFragment`.
## UX for filtered notifications
- Display filtered (with warning) notifications inline with other
notifications, with UI to disclose the notification or edit the filters.
Chooser dialog could start before any accounts have loaded. Fix by
collecting the account flow and waiting for the first emission (convert
the flow to shared instead of state so there's no initial empty list).
Guard against the potential for a similar issue when fetching
notifications.
Order the list of accounts with active account first so that code that
skips it by ignoring the first item works correctly.
Previous migration code could crash if the `clientId` or `clientSecret`
columns were null during the migration (unclear how that could happen
but there's at least one user report of this crash).
Re-write the migration to set these columns to the empty string if NULL
first.
Continue the work to remove the "activeAccount" idiom.
- Uses a new PachliAccount type through most of the app. This holds
information that was previously accessed separately (e.g., content
filters, lists) in one place. The information is loaded when the app
launches or the active account switches.
- Fetching data when the account is switched / loaded simplifies error
handling, as more code can now assume the data has already been loaded.
If it hasn't the code path is simply unreachable.
- This opens up the possibility of "acting as one account while logged
in as another". E.g., have two accounts, and be logged in to one account
and boost a post you've seen from your other account.
- Add a database migration to populate existing accounts with default
data when the user updates the app.
- Refactor code that used those list and filter repositories to get the
data from the PachliAccount instead. New local and remote data sources
are implemented, and the list and filter repositories mediate between
those sources.
- Start a ViewModel for MainActivity, which includes:
- Sending user actions as UiAction objects
- Providing a flow of uiState for MainActivity to react to
- Remove most uses of SharedPreferencesRepository from MainActivity
- Show messages about errors that occur when logging in
- Refactor intent routing in MainActivity to make the logic clearer.
- Add new `core.data` types to push more `core.network` types out of the
UI code
- `core.data.model.MastodonList` for `core.network.model.MastoList`
- `core.data.model.Server` for `core.network.model.Server`
- Continue the work to send the Pachli account ID to the code that uses
it.
- Most view models now get the account ID via assisted injection.
- QueuedMedia now includes the AccountEntity so it can operate with any
account. Modify the `uploadMedia` API call to include explicit
authentication details.
---------
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Previous code accepted the `scheduledAt` value as a String, and kept it
as a String (including when serialising as part of a draft). Then it was
converted to an actual Date for display.
Refactor to keep it as a Date for as long as possible. Moshi decodes
Dates correctly over the network, and the database is configured to
serialise Dates as Longs.
This necessitates two migration steps to preserve any existing
`scheduledAt` values for drafts. The first step adds a new column to
store the date as a Long and copies over existing data. The second step
replaces the old column with the new column.
ForeignKey constraints can be invalidated in the middle of a transaction
even if a later statement in the same transaction will make them valid
again. This seems to be causing production crashes.
Defer foreign key constraint checks until the end of the transaction to
prevent this.
Clean up the notification handling code and fix a lot of bugs, hopefully
without introducing new ones in the process.
Specific bugs discovered and fixed:
- The code that tried to sync notification filtering state between the
server and Pachli could fail, leaving things in an inconsistent state,
resulting in dropped notifications. Remove that code, do filtering
client-side.
- Logging out of an account would disable push notifications for all
accounts.
- If any account did not support push notifications then push
notifications were disabled for all accounts.
- If any account did not support push notifications the user was
prompted to log out of all accounts. Drop that entirely.
- The UnifiedPush library could get to a state where configuring the
notification mechanism would silently fail,
The preferences UI now has a section for notifications, showing:
- The Unified Push distributor in use (if any)
- A mechanism to change the distributor
- Per-account configuration and notification fetch details
- Battery optimisation state
General changes:
- Update to UnifiedPush library 2.4.0.
- NotificationFetcher.fetchAndShow() can now fetch a single account's
notifications, or all accounts, depending on data passed to the worker.
- Use ApiResult for `push/subscription` responses.
- Drop the "needs migration" terminology for the more specific "has push
scope", to make it clear what the issue with the account is.
`TabData` recorded the type of the timeline the user had added to a tab.
`TimelineKind` is another type that records general information about
configured timelines, with identical properties.
There's no need for both, so remove `TabData` and use `TimelineKind` in
its place.
`TimelineKind` is itself mis-named; it's not just the timeline's kind
but also holds data necessary to display that timeline (e.g., the list
ID if it's a `.UserList`, or the hashtags if it's a `.Hashtags`) so
rename to `Timeline` to better reflect its usage. Move it to a new
`core.model` module.
The previous code forgot to close the DB after TimelineDaoTest was run,
so a warning message was displayed when the test was run locally.
Close the database using the `@After` annotation.
Fixes#511
Once desugaring is enabled it needs to be enabled for up/down the
dependency chain, so enable it in the shared configuration defined by
the build convention code.
Highlighted a failing test that wasn't being run, so fix that too.
Many servers that claim to be Mastodon-API compatible are not, as
evidenced by the content they include in the responses to
`/api/v1/instance` and `/api/v2/instance` requests.
Work around the worst of the breakage by providing defaults or marking
some fields as nullable (with a default null).
Bugs have been reported against the relevant projects.
Previous code expected all incoming enums values to map directly to
Kotlin enum constants.
This is a problem for servers with additional features -- e.g.,
"reaction" as a notification type.
Fix this with a new Moshi adapter that will set the incoming value to a
given constant if it's not recognised.
Apply this to the enum constants in core.network to ensure they are
handled.
Clean up enum handling in Converters.kt, ComposeViewModel.kt, and
Status.kt by using the existing `.ordinal` property and some extension
functions for idiomatic code.
Fixes#461
Some users report that Pachli is not retrieving/displaying notifications
in a timely fashion.
To assist in diagnosing these errors, provide an additional set of tabs
on the "About" screen that contain information about how Pachli is
fetching notifications, and if not, why not.
Allow the user to save notification related logs and other details to a
file that can be attached to an e-mail or bug report.
Recording data:
- Provide a `NotificationConfig` singleton with properties to record
different aspects of the notification configuration. Update these
properties as different notification actions occur.
- Store logs in a `LogEntryEntity` table. Log events of interest with a
new `Timber` `LogEntryTree` that is planted in all cases.
- Add `PruneLogEntryEntityWorker` to trim saved logs to the last 48
hours.
Display:
- Add a `NotificationFragment` to `AboutActivity`. It hosts two other
fragments in tabs to show details from `NotificationConfig` and the
relevant logs, as well as controls for interacting with them.
Bug fixes:
- Filter out notifications with a null tag when processing active
notifications, prevents an NPE crash
Other changes:
- Log more details when errors occur so the bug reports are more helpful
Moshi is faster to decode JSON at runtime, is actively maintained, has a
smaller memory and method footprint, and a slightly smaller APK size.
Moshi also correctly creates default constructor arguments instead of
leaving them null, which was a source of `NullPointerExceptions` when
using Gson.
The conversion broadly consisted of:
- Adding `@JsonClass(generateAdapter = true)` to data classes that
marshall to/from JSON.
- Replacing `@SerializedName(value = ...)` with `@Json(name = ...)`.
- Replacing Gson instances with Moshi in Retrofit, Hilt, and tests.
- Using Moshi adapters to marshall to/from JSON instead of Gson `toJson`
/ `fromJson`.
- Deleting `Rfc3339DateJsonAdapter` and related code, and using the
equivalent adapter bundled with Moshi.
- Rewriting `GuardedBooleanAdapter` as a more generic `GuardedAdapter`.
- Deleting unused ProGuard rules; Moshi generates adapters using code
generation, not runtime reflection.
The conversion surfaced some bugs which have been fixed.
- Not all audio attachments have attachment size metadata. Don't show
the attachment preview if the metadata is missing.
- Some `throwable` were not being logged correctly.
- The wrong type was being used when parsing the response when sending a
scheduled status.
- Exceptions other than `HttpException` or `IoException` would also
cause a status to be resent. If there's a JSON error parsing a response
the status would be repeatedly sent.
- In tests strings containing error responses were not valid JSON.
- Workaround Mastodon a bug and ensure `filter.keywords` is populated,
https://github.com/mastodon/mastodon/issues/29142
Previously when the user interacted with a status the operation (reblog,
favourite, etc) travels through multiple layers of code, carrying with
it the position of the item in the list that the user operated on.
At some point the status is retrieved from the list using its position
so that the correct status ID can be used in the network operation.
If this happens while the list is also refreshing there's a possible
race condition, and the original status' position may have changed in
the list. Looking up the status by position to determine which status to
perform the action on may cause the action to happen on the wrong
status.
Fix this by passing the status' viewdata to any actions instead of its
position. This includes all the information necessary to make the API
call, so there is no chance of a race.
This is quite an involved change because there are three types of
viewdata:
- `StatusViewData`, used for regular timelines
- `NotificationViewData`, used for notifications, may wrap a status that
can be operated on
- `ConversationViewData`, used for conversations, does wrap a status
The previous code treated them all differently, which is probably why it
operated by position instead of type.
The high level fix is to:
1. Create an interface, `IStatusViewData`, that contains the data
exposed by any viewdata that contains a status.
2. Implement the interface in `StatusViewData`, `NotificationViewData`,
and `ConversationViewData`.
3. Change the code that operates on viewdata (`SFragment`,
`StatusActionListener`, etc) to be generic over anything that implements
`IStatusViewData`.
4. Change the code that handles actions to pass the viewdata instead of
the position.
Fixes#370
The previous code was serialising the `TabKind` enum without the `_`, so
when it was converted back to the enum name (which has a `_`) it failed
and immediately crashed.
Fixes#306
If a status was part of a thread, and it was not the "detailed" status,
and it had been translated, then the view data was marked as "show the
translation". But the translation was not loaded, so the status content
appeared as empty.
Fix that by loading the translated content of all statuses in the thead
and ensure that the translated content is rendered.
Throw an `IllegalStateException` in debug builds to catch any future
occurrences of this.
Fixes#281
The previous code generally started an activity by having the activity
provide a method in a companion object that returns the relevant intent,
possibly taking additional parameters that will be included in the
intent as extras.
E.g., if A wants to start B, B provides the method that returns the
intent that starts B.
This introduces a dependency between A and B.
This is worse if B also wants to start A.
For example, if A is `StatusListActivity` and B is`ViewThreadActivity`.
The user might click a status in `StatusListActivity` to view the
thread, starting `ViewThreadActivity`. But from the thread they might
click a hashtag to view the list of statuses with that hashtag. Now
`StatusListActivity` and `ViewThreadActivity` have a circular
dependency.
Even if that doesn't happen the dependency means that any changes to B
will trigger a rebuild of A, even if the changes to B are not relevant.
Break this dependency by adding a `:core:navigation` module with an
`app.pachli.core.navigation` package that contains `Intent` subclasses
that should be used instead. The `quadrant` plugin is used to generate
constants that can be used to launch activities by name instead of by
class, breaking the dependency chain.
The plugin uses the `Activity` names from the manifest, so when an
activity is moved in the future the constant will automatically update
to reflect the new package name.
If the activity's intent requires specific extras those are passed via
the constructor, with companion object methods to extract them from the
intent.
Using the intent classes from this package is enforced by a lint
`IntentDetector` which will warn if any intents are created using a
class literal.
See #291
The existing code base is a single monolithic module. This is relatively
simple to configure, but many of the tasks to compile the module and
produce the final app have to run in series.
This is unnecessarily slow.
This change starts to split the code in to multiple modules, which are:
- :core:account - AccountManager, to break a dependency cycle
- :core:common - low level types or utilities used in many other modules
- :core:database - database types, DAOs, and DI infrastructure
- :core:network - network types, API definitions, and DI infrastructure
- :core:preferences - shared preferences definitions and DI
infrastructure
- :core:testing - fakes and rules used across different modules
Benchmarking with gradle-profiler shows a ~ 17% reduction in incremental
build times after an ABI change. That will improve further as more code
is moved to modules.
The rough mechanics of the changes are:
- Create the modules, and move existing files in to them. This causes a
lot of churn in import arguments.
- Convert build.gradle files to build.gradle.kts
- Separate out the data required to display a tab (`TabViewData`) from
the data required to configure a tab (`TabData`) to avoid circular
dependencies.
- Abstract the repeated build logic shared between the modules in to
a set of plugins under `build-logic/`, to simplify configuration of
the application and library builds.
- Be explicit that some nullable types are non-null at time of use.
Nullable properties in types imported from modules generally can't be
smart cast to non-null. There's a detailed discussion of why this
restriction exists at
https://discuss.kotlinlang.org/t/what-is-the-reason-behind-smart-cast-being-impossible-to-perform-when-referenced-class-is-in-another-module/2201.
The changes highlight design problems with the current code, including:
- The main application code is too tightly coupled to the network types
- Too many values are declared unnecessarily nullable
- Dependency cycles between code that make modularisation difficult
Future changes will add more modules.
See #291.
