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

fix: make login and sync loading smoother [AR-3001] #1567

Merged
merged 4 commits into from
Mar 22, 2023

Conversation

saleniuk
Copy link
Contributor

@saleniuk saleniuk commented Mar 21, 2023

BugAR-3001 Loading spinner on setting up Wire Screen is not visible on android 9


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

Loading state UI is frozen during initial sync on Android 9.
Loading state when logging in is wrong and progress disappears after a split second.

Solutions

Login:
Remove turning off the loading flag on login screen right after the first call and let it be visible throughout the whole login flow.
Execute all important actions on IO to not block the main thread and let the loading be as smooth as possible.

Initial sync:
Call waitUntilSyncIsCompleted in LaunchedEffect with Unit key to make sure it's executed only once.
Change collect to firstOrNull to not collect items when it's not needed anymore.

Register device:
Open "initial sync" screen also after registering a device if the sync hasn't been done yet.

Testing

How to Test

Login and sync to see the difference.

Attachments (Optional)

Before:

login_sync_before.mp4


After:

login_sync_after.mp4

PR Post Submission Checklist for internal contributors (Optional)

  • Wire's Github Workflow has automatically linked the PR to a JIRA issue

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.

@AndroidBob
Copy link
Collaborator

Build 1 failed.

@github-actions
Copy link
Contributor

Unit Test Results

  56 files  ±0    56 suites  ±0   41s ⏱️ ±0s
387 tests ±0  386 ✔️ ±0  1 💤 ±0  0 ±0 

Results for commit baaf68f. ± Comparison against base commit 4281b86.

@AndroidBob
Copy link
Collaborator

Build 2 succeeded.

The build produced the following APK's:

@github-actions
Copy link
Contributor

Build available here. Scroll down to Artifacts!

@codecov
Copy link

codecov bot commented Mar 21, 2023

Codecov Report

Merging #1567 (baaf68f) into develop (4281b86) will increase coverage by 0.02%.
The diff coverage is 75.51%.

@@              Coverage Diff              @@
##             develop    #1567      +/-   ##
=============================================
+ Coverage      34.04%   34.06%   +0.02%     
  Complexity       653      653              
=============================================
  Files            243      243              
  Lines           8989     8997       +8     
  Branches        1143     1144       +1     
=============================================
+ Hits            3060     3065       +5     
  Misses          5632     5632              
- Partials         297      300       +3     
Impacted Files Coverage Δ
...ire/android/ui/initialsync/InitialSyncViewModel.kt 88.88% <66.66%> (-5.23%) ⬇️
...cation/devices/register/RegisterDeviceViewModel.kt 80.64% <71.42%> (-2.12%) ⬇️
.../authentication/login/email/LoginEmailViewModel.kt 69.74% <77.77%> (+0.78%) ⬆️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4281b86...baaf68f. Read the comment docs.

Copy link
Member

@vitorhugods vitorhugods left a comment

Choose a reason for hiding this comment

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

🪄✨

Copy link
Member

@MohamadJaara MohamadJaara left a comment

Choose a reason for hiding this comment

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

LGTM

@MohamadJaara MohamadJaara added this pull request to the merge queue Mar 22, 2023
@MohamadJaara MohamadJaara merged commit e5806d6 into develop Mar 22, 2023
@MohamadJaara MohamadJaara deleted the fix/loading-progress-login-and-sync branch March 22, 2023 13:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants