Replace remaining AsyncTask with RxAndroid #2681
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report
@@ Coverage Diff @@
## master #2681 +/- ##
=========================================
+ Coverage 2.84% 3.84% +0.99%
=========================================
Files 268 267 -1
Lines 12735 12524 -211
Branches 1127 1083 -44
=========================================
+ Hits 362 481 +119
+ Misses 12348 12010 -338
- Partials 25 33 +8
Continue to review full report at Codecov.
|
Use rxjava for upload task Use rxjava for thumbnail fetcher Use rxjava for media details fetch Fix build more fixes
|
@misaochan Does this PR also require unit test cases? I have not added anything new, have just refactored the existing async tasks to Rx. |
|
A lot of the time, media details don't seem to load for me - and do fine on the previous version. Logs (auth and location information has been removed) |
|
Actually I think the issue might have started in #2714 - either way until it's fixed I can't be sure that this PR is okay. |
Nice! It would be great if you could help in doing the same programmatically, so that when people send logs then all sensitive information is removed automatically. :) |
Were you able to pin-point the issue? In #2714, I didn't change anything related to media details. The changes were specific to peer review. Also, is media details just taking more time to load or does it never load even after waiting for a long time? |
|
I've just done a lot more testing, and actually it's okay previously, I think something might have been caching weirdly - completely removing and reinstalling app makes it work on master (tested I've attached some GIFs which show exactly what I see. I'm on an extremely fast and stable internet connection and I can repeat it now (by completely reinstalling) every time. No matter how long I leave it it never seems to load (even after leaving for 5+ minutes). Also there are some logs which suggest it has the data but isn't displaying it - will upload in a few minutes.
Apologies for the poor GIF quality, needed to be compressed to upload to Github. |
|
These are just the logs captured while navigating to this image (I started recording, cleared the logs, swiped, copy pasted the logs to a different app, stopped recording). It's clear there's definitely some info (line 28) there - e.g. it has the
Also for having logs without personal data (e.g. auth tokens, GeoIP data) I've just used find and replace, but I agree it'd be good to have some feature in the app that does that, or just some script as a dev tool idk. |
Use rxjava for upload task Use rxjava for thumbnail fetcher Use rxjava for media details fetch Fix build more fixes
|
Hi @maskaravivek , thanks for this. :) Yes, all grant tasks, including (and IMO, especially) the backend overhaul do require unit tests even if they aren't adding new functions - the idea is that after the grant is completed, our core functionality cannot be broken by future code changes without alerting us via failing tests. It is a bit of a pain now admittedly, but I believe this will be very beneficial in the future by reducing time spent hunting down bugs introduced unknowingly by a PR. Also, with TDD (writing the tests first), the code would need to be written with testability in mind - that is one of the benefits of TDD. It is probably a bit late to "write tests first" for this particular PR, haha, but you can just add them to this PR, and then try and use the TDD methodology for your next one? Also, shouldn't this be sent to Aside from that, this looks good to me. Thanks for the detailed Javadocs! ;) Edit: I tested on API 27 Nexus S emulator and @domdomegg 's issue occurs for me as well. It seems to happen only for specific pictures - some of them load and some don't. https://commons.m.wikimedia.org/wiki/File:Christchurch_city_tram_.jpg is one that never loads for me. |
misaochan
requested changes
Mar 25, 2019
Thanks for clarifying. Happy to add tests :)
There are couple of properties in the model classes which the wikimedia-data-client library currently doesn't have. I have raised pull requests for those properties (wikimedia/wikimedia-android-data-client#3 and wikimedia/wikimedia-android-data-client#2). So, for the time being, I have duplicated these properties in our existing classes. Once the library is updated, I can start consuming that.
Regarding the backend-overhaul, given the number of changes being made IMO it would be best to have things merged in master itself. With the library integrated, we have started consuming a few model classes from the library itself and people working on master won't have access to these classes. Also, with a lot of people contributing to our project at this point, it might lead to big merge conflicts. Also, with the master branch, we can rely on alpha testers to point our more issues. |
|
Yeah, I was wondering about using a development branch vs master. I think there are pros and cons to each:
I think I will leave the final decision for this to you and @ashishkumar468 since you guys will be the ones primarily working on this. But if you do decide to use master, please merge the development branch into it first, since some of @dbrant 's changes are in |
|
Some tests have started failing for this PR. Have marked this PR as WIP. |
domdomegg
requested changes
Mar 27, 2019
There was a problem hiding this comment.
Tested 2.10.1-debug-rxjava~f3b170087 on beta and prod
Issue: Description field displays raw HTML
| Example 1 (desc) | Example 2 (no desc) |
|---|---|
 |
 |
Issue: License link doesn't work on click
Log:
2019-03-27 22:53:33.144 2220-2220/fr.free.nrw.commons.beta D/MediaDetailFragment: Media license is: CC-BY-SA-4.0
2019-03-27 22:53:33.484 2220-2220/fr.free.nrw.commons.beta D/MediaDetailFragment: Unable to fetch license URL for CC-BY-SA-3.0
Automated tests all pass
|
Another couple of example description issues which are varied (i.e. not just in a
|
|
@domdomegg Have fixed description.
Also, the license is now clickable. Have gone ahead and deleted Had to simply update the |
ashishkumar468
approved these changes
Mar 28, 2019
domdomegg
requested changes
Mar 28, 2019
There was a problem hiding this comment.
Issue found when testing:
- Clicking license doesn't open web browser (no web browser found to open url), despite one being installed (e.g. overflow menu > show in browser, or links in AboutActivity work fine)
Also possible enhancements (could be done as separate PRs - all I want to merge is bug fixed above)
- Description shows 'this file has no description, and may be...' when there is no description. Would probably be simpler if we could detect this somehow and just display 'No description' as we did before.
- Show some loading status until we display the media meta data (e.g. just a spinner in the media details panel).
Sorry for constantly requesting changes!
domdomegg
approved these changes
Mar 28, 2019
Can you share an example file name for which this happens? License click works for me for whatever media I checked.
Again, can you share an example. If theres no description then we simply show "No description". 'this file has no description, and may be...' text might be coming from the API itself.
Yup, sure. Will add in another PR.
Infact, I feel sorry for making you retest the PR again and again. :( |
Only happens on Beta - maybe upstream bug? I think my workaround fixes it for now, but will also submit bug upstream.
Tested this more in depth - it just happens on beta so it's almost certainly an API problem - maybe worth submitting upstream bug.
I'm happy to merge now - and will open issues for beta related bugs. |
|
Great job with this PR, @maskaravivek ! Btw, your tests increased our coverage from 2.84% to 3.84%, quite a significant jump! ;) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description (required)
Fixes part of #1092
Screenshots
Screenshots of things that were affected.