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

Prevent wrong or null endpoints returned if updating apps with variant changed. #25

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

naturecurly
Copy link

When endpoint configuration was changed, the shared preference should clear all old data and store the modified data immediately. If the Editor isn't applied, it will return wrong or null endpoint.

… clear all old data and store the modified data immediately. If the Editor isn't applied, it will return wrong or null endpoint.
@kamal-kamalmohamed kamal-kamalmohamed self-requested a review August 28, 2017 03:39
@kamal-kamalmohamed
Copy link

I agree that there's an issue here. We should call apply() on the editor once we're done or in some scenarios there's going to be data loss.

Instead of adding an apply() on line 77 I would suggest to move the editor.apply() on line 66 of VariantSharedPreferencesRepository to line 68. This way there's always going to be an editor.apply() call at the end of the init block.

Thoughts?

And, thanks for your contribution!

@naturecurly
Copy link
Author

Taking the editor.apply() out from the block like you said still has an issue, which is the method getCurrentVariant() will return a null because the sharedPreferences does not have a key called CURRENT_VARIANT.

@kamal-kamalmohamed
Copy link

kamal-kamalmohamed commented Aug 28, 2017

getCurrentVariant() returns Variant?, so a null is acceptable and should be handled by the calling class. Do you see a problem with that?

@naturecurly
Copy link
Author

Yes. When updating an app with modified variants, what we normally need is to replace the existed endpoint variants with the new one. If we only take out the editor.apply(), the app will get a null variant after updating. Then I need to add an if block to deal with the null endpoint, and urls will be hardcoded in the java/kt files, which may not be elegant. I reckon the original aim of this library is to make all endpoints in a json file which is easy to be managed.

@kamal-kamalmohamed
Copy link

kamal-kamalmohamed commented Aug 29, 2017

Yep, that's definitely the aim of the library. :)

Sorry, maybe I didn't explain my idea very well. I'm not proposing to just remove the apply() call from line 66. I'm proposing to move it to line 68, so outside the if-statement. This would make it so that the changes we make in the init block are always saved.

Did I misunderstand your comment? Is there an issue with this approach?

Also, could I get accurate reproduction steps for the issue you're trying to fix?

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.

2 participants