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

Refactor/lazy provider #823

Merged
merged 25 commits into from
Jul 14, 2023
Merged

Refactor/lazy provider #823

merged 25 commits into from
Jul 14, 2023

Conversation

bdmendes
Copy link
Collaborator

@bdmendes bdmendes commented Jul 6, 2023

Closes #659

This PR:

  • Removes the old behaviour of loading all app data upon startup and only does so the first time a Provider needs it. For that logic to work, a custom LazyConsumer is used to replace Consumer.
  • Makes the last refresh time Provider-dependent (instead of global), saving it on the shared preferences instead of the db
  • Makes the refresh page-based
  • Allows caching the offline data for a while, avoiding multiple requests in a very close period of time
  • Simplifies RequestDependantWidgetBuilder to make loading status more consistent

Please review this carefully as this changes the main flow of the app.

Review checklist

  • Terms and conditions reflect the current change
  • Contains enough appropriate tests
  • If aimed at production, writes a new summary in whatsnew/whatsnew-pt-PT
  • Properly adds an entry in changelog.md with the change
  • If PR includes UI updates/additions, its description has screenshots
  • Behavior is as expected
  • Clean, well-structured code

@bdmendes bdmendes force-pushed the refactor/lazy-provider branch 5 times, most recently from c3da4df to a3de86f Compare July 10, 2023 14:41
@bdmendes bdmendes force-pushed the refactor/lazy-provider branch from a3de86f to 1d13aa3 Compare July 10, 2023 14:43
@bdmendes bdmendes force-pushed the refactor/lazy-provider branch from 63b25e4 to a231d07 Compare July 10, 2023 15:38
@bdmendes bdmendes marked this pull request as ready for review July 10, 2023 15:39
@bdmendes bdmendes requested review from thePeras and LuisDuarte1 July 10, 2023 16:51
@bdmendes bdmendes added this to the Mid-Summer 2023 Release milestone Jul 12, 2023
Copy link
Member

@LuisDuarte1 LuisDuarte1 left a comment

Choose a reason for hiding this comment

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

Congratulations! 🥳 This one is a tough one and you managed it super well 😃 with no visible regressions that I can test. I just have some concerns below related more to maintainability and knowing how some parts of the system works

@bdmendes bdmendes force-pushed the refactor/lazy-provider branch from 0564e6f to 8142b3f Compare July 12, 2023 20:38
@bdmendes bdmendes requested a review from LuisDuarte1 July 12, 2023 20:50
LuisDuarte1
LuisDuarte1 previously approved these changes Jul 13, 2023
Copy link
Member

@LuisDuarte1 LuisDuarte1 left a comment

Choose a reason for hiding this comment

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

LGTM, good work! 🚀 🎉

Copy link
Collaborator

@Sirze01 Sirze01 left a comment

Choose a reason for hiding this comment

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

The only thing I don't agree with is when using a non-persistent session the homepage loads with no widgets, not even the default ones (Is this intentional?).

Otherwise good job, a great refactor, both in size and quality.

@bdmendes
Copy link
Collaborator Author

@LuisDuarte1 @Sirze01 I have opted to always load from storage, even if the session is not persistent, to generalize the loading and avoid bugs like the one @Sirze01 found. To make sure data does not get loaded into storage when the session is not persistent, I suggest tackling #835 in another PR.

@Sirze01 Sirze01 self-requested a review July 14, 2023 11:24
Copy link
Collaborator

@Sirze01 Sirze01 left a comment

Choose a reason for hiding this comment

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

It's great now. Working as expected

Copy link
Member

@LuisDuarte1 LuisDuarte1 left a comment

Choose a reason for hiding this comment

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

LGTM! nice catch 🎉

@Sirze01 Sirze01 merged commit b726c30 into develop Jul 14, 2023
@Sirze01 Sirze01 deleted the refactor/lazy-provider branch July 14, 2023 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make each page responsible for loading their content
4 participants