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

Feat: Real-time sync shopping lists addition/removal #578

Merged
merged 1 commit into from
Dec 13, 2024

Conversation

RedX2501
Copy link
Contributor

Add real-time synchronization to shopping lists

Adding or removing shopping lists now immediately
updates them in the available shopping list selector
in the "add items" overview.

I'm not entirely sure if calling refresh() is enough. At least in my tests it was but I'm not sure if there are side-effects missing that I don't know about.

When you use the Linux or Android App and have Chrome open at the same time the update in Chrome happens once the window has focus. Not sure what that is about.

Again this PR is only a draft as it depends on other PRs. I'll rebase onto main once those are complete.

@TomBursch
Copy link
Owner

I'm not entirely sure if calling refresh() is enough. At least in my tests it was but I'm not sure if there are side-effects missing that I don't know about.

Yeah, that's fine. Also, thanks for the refactor.

I know this is a draft, but in general, I'd recommend branching from main, since this PR doesn't really depend on the other changes. This saves you some time when changes have to be made in other PRs (and you won't need to update this branch because of that) and makes it a bit easier to review, and we could have merged this already 😉

@TomBursch TomBursch added the enhancement New feature or request label Dec 13, 2024
@RedX2501
Copy link
Contributor Author

Yeah, at first glance it does not depend but on my dev side it does because of the changes on the launch.json (which keeps giving me git errors when changing branches due to being overwritten on the checkout of the other branches). Maybe I'll just be more patient in the future with opening these PRs.

@RedX2501 RedX2501 force-pushed the feature/synch-shopping-lists branch 2 times, most recently from 962bd71 to 774c1bb Compare December 13, 2024 11:44
@RedX2501 RedX2501 marked this pull request as ready for review December 13, 2024 11:44
@TomBursch
Copy link
Owner

Looks good, I noticed the off methods are missing. If you add those, then I'll approve this.

@RedX2501
Copy link
Contributor Author

I didn't add them as nobody uses them. So there would be less API to test later.

@TomBursch
Copy link
Owner

TomBursch commented Dec 13, 2024

They are used inside the close method of the cubit.

@override
  Future<void> close() async {
    ApiService.getInstance().offShoppinglistItemAdd(onShoppinglistItemAdd);
    ApiService.getInstance().offShoppinglistItemRemove(onShoppinglistItemRemove);
    super.close();
  }

If you switch households, for example, the cubit is closed but would remain a listener on the socket events. This would result in errors and a memory leak, since it couldn't be garbage collected.

@RedX2501 RedX2501 force-pushed the feature/synch-shopping-lists branch 2 times, most recently from 7d0298a to c65c559 Compare December 13, 2024 12:28
Adding or removing shopping lists now immediately
updates them in the available shopping list selector
in the "add items" overview.
@TomBursch TomBursch force-pushed the feature/synch-shopping-lists branch from c65c559 to 3015952 Compare December 13, 2024 12:45
@TomBursch TomBursch changed the title Feature/synch shopping lists Feat: Sync shopping lists addition/removal Dec 13, 2024
@TomBursch TomBursch changed the title Feat: Sync shopping lists addition/removal Feat: Real-time sync shopping lists addition/removal Dec 13, 2024
@TomBursch TomBursch merged commit 4828f32 into TomBursch:main Dec 13, 2024
3 checks passed
@RedX2501 RedX2501 deleted the feature/synch-shopping-lists branch December 17, 2024 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants