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

Reload contents when activity start #97

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

unixzii
Copy link
Contributor

@unixzii unixzii commented Jun 12, 2017

This is a workaround for the enhancement I proposed in #85 .

It basically reloads anything when activity is started, and find out whether the selections are valid. If an album user previously selected is not existed after reloading, we will navigate to the initial album.

For now, I simply clear all selections when contents changed.

@gejiaheng
Copy link
Contributor

I've tested this PR. There is a severe issue. As the data reloads after MatisseActivity.onStart() got called, if you preview media content and come back, all data is reloaded with UI refreshed. You will be always on the top of the list. Users would have to scroll to the previous position again and again.

@unixzii
Copy link
Contributor Author

unixzii commented Jun 13, 2017

Hum, that's really a bad bug, I'll fix it soon.

@unixzii
Copy link
Contributor Author

unixzii commented Jun 13, 2017

@gejiaheng
I've fixed that issue, now any changes can be found out and only changed items will be refreshed. Additionally, users' selections will not lose.

@unixzii unixzii force-pushed the observer-workaround branch from 62f74bf to fe2db37 Compare June 14, 2017 16:32
iterator.remove();
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Under what conditions would the path be null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When users restarted the picker activity, we will reload the album list first, and then check out whether the previously selected items still exist, if users deleted the image, PathUtils will resolve nothing, that means we should move it out of selections.

@gejiaheng
Copy link
Contributor

I tested this PR again and got a little confused. When user goes to preview page or home to launcher, then comes back, Matisse restarts the loader and the onCreateLoader() got called. Actually we don't need to recreate the loader, right?
And I don't figure out how you keep the list untouched from a user perspective.

@unixzii
Copy link
Contributor Author

unixzii commented Jun 22, 2017

  1. I recreate the loader to reload albums because we need to figure out what changed, don't forget the case that users switch out and delete something then come back. And we've discussed the problems with loader's built-in observing mechanism, so I choose to just reload.
  2. I modified the logic of fragment management, and when users go back to our activity from other apps, I will just let the recycler view in existing fragment swap the cursor, so scrolling position and selections will not lose.

@gejiaheng
Copy link
Contributor

gejiaheng commented Jun 24, 2017

Yeah, I understand most of the work you do. Do we have to recreate the loader? What if we don't recreate but just reload the loader?

I'm sorry I am like a little cautious about this PR, because Zhihu App is heavily dependent on this library.

@unixzii
Copy link
Contributor Author

unixzii commented Jun 24, 2017

@gejiaheng
I really understand your worry, cuz we all want this library to be better. I reviewed my code again only just, and I might have made a mistake. Actually, we don't need to restart the loader. And in the newest commit, to be more moderate, I've changed it to Loader#forceLoad.

@gejiaheng
Copy link
Contributor

I'll conduct some more tests. Thanks for your patience.

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