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

Not update model with saveViewState flag #308

Closed
SerhiiStrila opened this issue Oct 7, 2017 · 10 comments
Closed

Not update model with saveViewState flag #308

SerhiiStrila opened this issue Oct 7, 2017 · 10 comments

Comments

@SerhiiStrila
Copy link

Seems to be an issue with updating a model with saveViewState flag.
I attach a link to the project to reproduce the bug.
https://github.com/SerhiiStrila/EpoxyIssue

@elihart
Copy link
Contributor

elihart commented Oct 9, 2017

@SerhiiStrila Thanks for providing a project to reproduce, I will look into it soon.

Can you describe the issue in more detail first, though? I don't understand the issue completely

@SerhiiStrila
Copy link
Author

@elihart Sure

Step for reproduce bug:

  1. Set for model saveViewState flag
  2. Load some data to controller
  3. Scroll to any position, all ok
  4. Tap on button and update data in controller
    Result:
    Cell in recycler view updated to cell with error, but the error text of input field not showed.
    Seems, that after update the controller data, view trys to restore state from bundle with ignoring of new params or maybe my code has a mistake and all ok.

@elihart
Copy link
Contributor

elihart commented Oct 10, 2017

@SerhiiStrila Saved state is not intended to compete with values set via model props. It doesn't make sense to have a value set via both a prop and saved state. This generally works fine, like with textview selection indexes - the model doesn't need to control that.

In the case of error text it makes sense for the model to control it - the issue you're seeing happens because saved state is applied after the model data is bound to the view. An easy fix would be to swap that order. I think that would make more sense, but it could also break existing behavior for people so I will have to think about how best to handle it.

@SerhiiStrila
Copy link
Author

@elihart Thanks for your answer! Could you please explain where I can change order of applying saved state data for my case?

@elihart
Copy link
Contributor

elihart commented Oct 11, 2017

Unfortunately the order is not customizable, that is the problem. you can see it in BaseEpoxyAdapter#onBindViewHolder. I need to think about the best way to support changing this order without breaking existing behavior

@elihart
Copy link
Contributor

elihart commented Oct 23, 2017

After consideration, I don't think it is possible to reverse the order in the general case. The model data needs to be bound before saved state is applied, otherwise cases like restoring a carousel position, or an edit text cursor position, would not work.

I think a simple solution may be for Epoxy to just not restore saved state if the data has changed. ie restore state when the view is scrolled back on screen, but not when it is already on screen and it changes. I can experiment with that - it would be a simple code change, but I also want to make sure it doesn't break any use cases.

@SerhiiStrila
Copy link
Author

@elihart are there any changes on this issue?

@elihart
Copy link
Contributor

elihart commented Dec 12, 2017

Sorry, I haven't had time to spend on this. I will try to work on it in the next week.

@elihart
Copy link
Contributor

elihart commented Dec 20, 2017

With #367 I have changed state to not be reapplied when a view is updated. I think this will help your case.

However, you should really avoid having a model prop that conflicts with saved state. It is possible for them to get out of sync and cause weird issues - this would happen if your model prop is updated while the model is offscreen, when it is scrolled on screen the previous saved state won't include the model update.

In your case all of the state should probably be stored in the model instead of using view state.

@elihart elihart closed this as completed Dec 20, 2017
@SerhiiStrila
Copy link
Author

I see. @elihart Thank you for your time and work!

This issue was closed.
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

No branches or pull requests

2 participants