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

Update lists #403

Merged
merged 13 commits into from
Jan 28, 2017
Merged

Update lists #403

merged 13 commits into from
Jan 28, 2017

Conversation

di72nn
Copy link
Member

@di72nn di72nn commented Jan 19, 2017

Adds "endless scrolling" instead of limited list. Memory may still be an issue.

Use DiffUtil to update lists partially.

I'm not completely sure about the implementation, so suggestions are welcome.

Also, I think I had an issue with the lists not being updated after actions like "open an article - add it to favorite - go back to lists". I couldn't reproduce it yet. I added logging of warning messages "fragment is null". I'm pretty sure it should either update list or log the warning. I have no idea why the fragments can be null after they were initialized (initially I thought, that it might be related to threading, but only the main thread is supposed to access fragments). Probably fixed with 2630998.

Related issues:

Testing needed.

@di72nn
Copy link
Member Author

di72nn commented Jan 19, 2017

Added basic search (using SQL's LIKE %search string%) on article title and text.
Current list is updated on every search string change - not sure about performance.

@tcitworld
Copy link
Member

Hmm. I guess it will provoke some performance issues with huge articles, some maybe limit to title for now. Anyway we'll probably propose a search API soon on the server so it will have to be integrated with it at some point.

@di72nn di72nn changed the title [WIP] Update lists Update lists Jan 22, 2017
@di72nn
Copy link
Member Author

di72nn commented Jan 25, 2017

Added an option to reverse sort order. The UI part should probably be improved (as for now it is just a menu item that reads "Change sort order"). Suggestions are welcome.

There is an issue that fragments reinit their lists after orientation change. It is not critical (slight delay), but I'll have to fix it. Fixed.

@di72nn
Copy link
Member Author

di72nn commented Jan 26, 2017

Started to use AsyncTask to update lists. Synchronization issues are possible. Removed from this PR.

@Strubbl
Copy link
Contributor

Strubbl commented Jan 26, 2017

is the read to review label still valid atm?

@di72nn
Copy link
Member Author

di72nn commented Jan 26, 2017

I removed the async loading from this PR - now it is ready for review.

@tcitworld
Copy link
Member

Looks good. Will review and eventually build tomorrow.

@tcitworld tcitworld merged commit 6603df1 into master Jan 28, 2017
@tcitworld tcitworld deleted the lists branch January 28, 2017 18:03
@di72nn
Copy link
Member Author

di72nn commented Jan 29, 2017

@tcitworld

Hmm. I guess it will provoke some performance issues with huge articles, some maybe limit to title for now.

I forgot to answer that. It seemed to me that the performance was decent even on old devices, so I left it as is. I considered "powerful" search (being able to search in content) more important than minor performance issues (which don't affect anything but search). Though, anyone is free to improve the feature.

@tcitworld
Copy link
Member

I tested and it was okay for me so I merged. :)

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

Successfully merging this pull request may close these issues.

3 participants