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

State for a MultiKey only stores the last View that save() was called for #229

Closed
edenman opened this issue Feb 10, 2017 · 6 comments
Closed
Labels
Milestone

Comments

@edenman
Copy link
Contributor

edenman commented Feb 10, 2017

KeyDispatcher passes KeyChanger a Map of contexts, but only one incomingState. State.save() looks like this:

  public void save(@NonNull View view) {
    SparseArray<Parcelable> state = new SparseArray<>();
    view.saveHierarchyState(state);
    viewState = state;
  }

Unfortunately this means if you're saving view state of multiple keys, only the last one actually gets stored in the bundle 😭 Should probably have a Map<Int, SparseArray<Parcelable>> viewState instead and use view ids as the key?

@dcow
Copy link
Contributor

dcow commented Feb 11, 2017

I noticed this too and held off on any attempt to build compound layouts using multiple keys.

@dcow
Copy link
Contributor

dcow commented Feb 11, 2017

Keep in mind: views don't always have ids. But I think the running convention is to only expect views with ids to be saved.

@loganj
Copy link
Collaborator

loganj commented Feb 13, 2017

Good catch. This is another weird wrinkle related to MultiKeys being keys themselves.

I like the idea of making State's viewState field a map. That opens the door to saving arbitrary state as well, if we wanted to do so later.

This does add a requirement that views have ids to be persisted, but that's fine IMO. Should be added to javadoc.

@loganj loganj added the bug label Feb 13, 2017
@loganj loganj added this to the 1.0 beta milestone Feb 13, 2017
@edenman
Copy link
Contributor Author

edenman commented Feb 13, 2017

Cool, I'll work on a PR

@edenman
Copy link
Contributor Author

edenman commented Feb 13, 2017

Should I blow up if somebody calls save on a view with no id? Feels like it'd be too easy to accidentally stop saving state on one of your screens if I don't blow up.

@loganj
Copy link
Collaborator

loganj commented Feb 13, 2017

blowing up sounds right.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants