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

chore: add support for jetpack paging on persistence (AR-1882) #886

Merged

Conversation

vitorhugods
Copy link
Contributor

@vitorhugods vitorhugods commented Sep 11, 2022


PR Submission Checklist for internal contributors

  • The PR Title

    • conforms to the style of semantic commits messages¹ supported in Wire's Github Workflow²
    • contains a reference JIRA issue number like SQPIT-764
    • answers the question: If merged, this PR will: ... ³
  • The PR Description

    • is free of optional paragraphs and you have filled the relevant parts to the best of your ability

What's new in this PR?

Issues

On Android, dealing with LazyColumn + manual implementation of Paging with reactive data is a bit painful.

Solutions

Add support for Jetpack Paging on Android, using SQLDelight's extensions for that.

Issues with SQLDelight implementation

There are a few issues related to the current implementation of PagingSource on SQLDelight. Especially for us that are a few versions behind and can't upgrade right now:

Because of this, I've altered the implementation in a couple of lines to suit our needs.

Platform-specific extension

Adding a platform-specific extension was a first (I believe ?) to this project.
The chosen approach was to create an expect class MessageExtensions that is added to the current MessageDAO.
This way, each platform can create its own extensions on their actual implementation of the MessageExtensions.
The alternative would be flipping this completely upside-down, like UserSessionScope does:
A MessageDAOCommon and actual implementations of MessageDAO that extend the common implementation. However, empty extension classes seem easier to maintain than creating empty actual implementations of the main behaviour.

Small refactoring on MessageMapper

As the mapper is needed for both the MessageDAO and the MessageExtensions, I've moved it to its own file, and moved some of the mapping functions from the DAO to the Mapper, as it probably should've been done before anyway. This way these functions can be used by both classes.

Testing

Test Coverage

  • I have added automated test to this contribution

PR Post Merge Checklist for internal contributors

  • If any soft of configuration variable was introduced by this PR, it has been added to the relevant documents and the CI jobs have been updated.

References
  1. https://sparkbox.com/foundry/semantic_commit_messages
  2. https://github.com/wireapp/.github#usage
  3. E.g. feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764.

@github-actions
Copy link
Contributor

github-actions bot commented Sep 11, 2022

Unit Test Results

   195 files   -   19     195 suites   - 19   1m 35s ⏱️ + 1m 17s
1 051 tests +    5  1 038 ✔️  -     2  13 💤 +7  0 ±0 
1 837 runs  +791  1 824 ✔️ +784  13 💤 +7  0 ±0 

Results for commit 1a722e2. ± Comparison against base commit 75cbb7c.

♻️ This comment has been updated with latest results.

@codecov-commenter
Copy link

codecov-commenter commented Sep 11, 2022

Codecov Report

Merging #886 (1a722e2) into develop (75cbb7c) will decrease coverage by 0.01%.
The diff coverage is 30.90%.

@@              Coverage Diff              @@
##             develop     #886      +/-   ##
=============================================
- Coverage      60.36%   60.35%   -0.02%     
- Complexity       673      679       +6     
=============================================
  Files            455      456       +1     
  Lines          12114    12127      +13     
  Branches        1223     1225       +2     
=============================================
+ Hits            7313     7319       +6     
- Misses          4279     4286       +7     
  Partials         522      522              

@vitorhugods vitorhugods removed the WIP work in progress label Sep 11, 2022
@vitorhugods vitorhugods marked this pull request as ready for review September 11, 2022 23:48
Copy link
Contributor

@Garzas Garzas left a comment

Choose a reason for hiding this comment

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

LGTM 🎸

@vitorhugods vitorhugods merged commit 9d25094 into develop Sep 12, 2022
@vitorhugods vitorhugods deleted the android/chore/add_support_for_jetpack_paging_on_persistence branch September 12, 2022 05:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants