Feature/refactor contributions #3046

ashishkumar468 · 2019-06-30T18:33:22Z

Description (required)

Fixes #3045 Refactor Contributions

What changes did you make and why?

* Refactored ContributionsListFragment to use RecyclerView
* Added ContributionsPresenter
* Extracted out the cursor to presenter
* Probable fix for #3028
* Improved the logic for cache in ContributionViewHolder

Tests performed (required)

Tested {build variant, betaDebug} on Pixel-27

* Added ContributionsPresenter * Extracted out the cursor to presenter * Probable fix for commons-app#3028

pullrequest

✅ A review job has been created and sent to the PullRequest network.

Check the status or cancel PullRequest code review here.

pullrequest

	1 Message
⚠️	Due to its size, this pull request will likely have a little longer turnaround time and will probably require multiple passes from our reviewers.

codecov-io · 2019-06-30T18:46:14Z

Codecov Report

Merging #3046 into master will increase coverage by 0.25%.
The diff coverage is 14.47%.

@@            Coverage Diff            @@
##           master   #3046      +/-   ##
=========================================
+ Coverage    4.41%   4.67%   +0.25%     
=========================================
  Files         259     264       +5     
  Lines       12309   12354      +45     
  Branches     1056    1051       -5     
=========================================
+ Hits          544     577      +33     
- Misses      11726   11734       +8     
- Partials       39      43       +4

Impacted Files	Coverage Δ
.../nrw/commons/category/CategoryDetailsActivity.java	`0% <ø> (ø)`	⬆️
...va/fr/free/nrw/commons/explore/SearchActivity.java	`0% <ø> (ø)`	⬆️
...e/nrw/commons/category/CategoryImagesActivity.java	`0% <ø> (ø)`	⬆️
...r/free/nrw/commons/contributions/MainActivity.java	`0% <ø> (ø)`	⬆️
...rw/commons/explore/categories/ExploreActivity.java	`0% <ø> (ø)`	⬆️
.../free/nrw/commons/bookmarks/BookmarksActivity.java	`0% <ø> (ø)`	⬆️
...s/contributions/model/DisplayableContribution.java	`0% <ø> (ø)`	⬆️
...w/commons/contributions/ContributionsContract.java	`0% <0%> (ø)`
...fr/free/nrw/commons/media/MediaDetailFragment.java	`0% <0%> (ø)`	⬆️
...ommons/contributions/ContributionsListAdapter.java	`0% <0%> (ø)`	⬆️
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0c3b002...cfb3eb3. Read the comment docs.

…completed && local uri exists, use that uri to show image

neslihanturan

Hi @ashishkumar468 , I see same images on fast scroll, instead of new ones. Then after a while correct images are displayed. Do you think this is related with this pull request?

maskaravivek · 2019-07-01T11:50:54Z

@ashishkumar468 Does the PR fix #3028? If it does we can rebase the PR on 2.11-release branch.

ashishkumar468 · 2019-07-02T04:32:11Z

Thanks for testing this out @neslihanturan . Should not be related to this PR, but this PR intends to fix those things. I will look into it and add a fix for the same
@maskaravivek are w sure we wanna add in the latest release branch ?

maskaravivek · 2019-07-02T07:07:28Z

Yes, it is part of #3024 which is needed for 2.11. @misaochan can reconfirm.

misaochan · 2019-07-02T09:05:07Z

Hmm. Yes, we do need a fix for #3028 , however this PR introduces many other changes as well (which is expected because it is also part of the refactor task). Would it be simple to isolate the part that fixes #3028 ? If yes, it would be easier to simply submit a different PR for that, because otherwise I would need to run the comprehensive tests all over again. Normally once I finish pre-release testing, I try to have minimal code changes so that we don't hit unexpected side effects.

pullrequest

Very nice work! I think that most of my comments are questions and minor suggestions.

Reviewed with ❤️ by PullRequest

pullrequest · 2019-07-04T13:06:43Z

app/src/main/java/fr/free/nrw/commons/contributions/ContributionsLocalDatSource.java

+import javax.inject.Inject;
+import javax.inject.Named;
+
+class ContributionsLocalDatSource {


Should this be ContributionsLocalDataSource?

Yes, my bad, I will do the correction

pullrequest · 2019-07-04T13:06:43Z

app/src/main/java/fr/free/nrw/commons/contributions/ContributionViewHolder.java

+        imageView.setBackground(null);
+        if ((contribution.getState() != Contribution.STATE_COMPLETED) && fileExists(
+                contribution.getLocalUri())) {
+            imageView.setImageURI(contribution.getLocalUri());


If getState() != Contribution.STATE_COMPLETED, does contribution.getLocalUri() have some temporary image?

Yes, contribution.localUri has the temporary uri for the image which is saved in the db while the upload is in progress

pullrequest · 2019-07-04T13:06:43Z

app/src/main/java/fr/free/nrw/commons/contributions/ContributionsModule.java

+    @Binds
+    public abstract ContributionsContract.UserActionListener bindsContibutionsPresenter(
+            ContributionsPresenter
+                    presenter);


Looks like this should go on the previous line.

Do you mean the formatting ?

Yes—just formatting. :-)

pullrequest · 2019-07-04T13:06:43Z

app/src/main/java/fr/free/nrw/commons/contributions/ContributionsPresenter.java

+    }
+
+    @Override
+    public void onLoaderReset(@NonNull Loader<Cursor> loader) {


The documentation of LoaderManager.LoaderCallbacks doesn't mention whether onLoadFinished is guaranteed to be called before onLoaderReset. I found a StackOverflow thread where one commenter says:

onLoaderReset(Loader) asks you to fulfill the same contract [as onLoadFinished], but without providing a replacement [cursor]

Thus, I would suggest that you turn off the progress bar and all of the tip, just to be sure.

Thanks for letting me know, I will make the required changes

pullrequest · 2019-07-04T13:06:44Z

app/src/main/java/fr/free/nrw/commons/media/MediaDetailPagerFragment.java

-
-        void registerDataSetObserver(DataSetObserver observer);
-
-        void unregisterDataSetObserver(DataSetObserver observer);


Removing these unused methods from the interface is a good idea.

Sure, will do :-)

pullrequest · 2019-07-04T13:06:45Z

app/src/main/java/fr/free/nrw/commons/contributions/ContributionsPresenter.java

+
+    @Override
+    public void onLoadFinished(@NonNull Loader<Cursor> loader, Cursor cursor) {
+        view.showProgress(false);


Where is view.showProgress(true)?

That is called from the parent fragment, that is the one which initialises the loader, the parent fragment of ContributionsListFragment and MediaDetailPagerFragment

pullrequest · 2019-07-04T13:06:45Z

app/src/main/java/fr/free/nrw/commons/contributions/ContributionsListFragment.java

+            rvContributionsList.setLayoutManager(gridLayoutManager);
        } else if (newConfig.orientation == Configuration.ORIENTATION_PORTRAIT) {
            fab_layout.setOrientation(LinearLayout.VERTICAL);
+            rvContributionsList.setLayoutManager(linearLayoutManager);


One of the things that is unclear to me is whether switching view managers like this will maintain the current scroll position. You wouldn't want to to pop up to the top just because the orientation changed. Likewise, because I believe these two maintain different scroll positions, you could get some strange effects by scrolling to different places in each orientation and then rotating the device to change back and forth.

Yes, as a fix to that, we can store the id of the corresponding list item and use to id to request focus (bring to user's visible position) to that particular child

Sounds like a good plan.

pullrequest · 2019-07-04T13:06:45Z

app/src/main/java/fr/free/nrw/commons/ViewHolder.java

 public interface ViewHolder<T> {
-    void bindModel(Context context, T model);
+    void init(int position,
+            DisplayableContribution contribution);


I'm a bit confused by this. ViewHolder is meant to be a generic holder object for a model (the generic T) and the Android context. It doesn't see appropriate to convert this to such a specific method. Perhaps this happened with some automated refactoring? I would strongly urge you to change it back and to introduce a new interface if necessary.

If you really want to go down this route, then you need to at least change ViewHolder to not have a generic parameter and rename the class to something more specific, like DisplayableContributionHolder. You may also want to move it to commons/contributions since that's where it seems to be used.

Thanks for figuring this out, yes I am planning to change it to void bindModel(int position, T model);, context is not required by the view holder, it can get it from its itemView, what do you think ?

The way I think about it, ViewHolder<T> is a very generic interface—you should be able to call bindModel with any type. As soon as you add position to it, the interface is no longer generic—the model must have some idea of "position" that is an integer.

But maybe this doesn't really matter. In this change, you only call init(...) from onBindViewHolder, which takes a ContributionViewHolder. Thus you don't even need to implement ViewHolder.

Thus, I would suggest reverting the changes to ViewHolder<T> and making ContributionViewHolder not implement that interface.

What do you think?

Yes, I have made ContributionViewHolder not implement the generic ViewHolder, later on when the model holds the position, it could be refactored to implement the ViewHolder

pullrequest · 2019-07-04T13:06:45Z

app/src/main/java/fr/free/nrw/commons/contributions/ContributionViewHolder.java

+        if ((contribution.getState() != Contribution.STATE_COMPLETED) && fileExists(
+                contribution.getLocalUri())) {
+            imageView.setImageURI(contribution.getLocalUri());
+        }else {


Space before else.

Will add :-)

pullrequest · 2019-07-04T13:06:45Z

app/src/main/java/fr/free/nrw/commons/contributions/ContributionsListFragment.java

    }

+    public void onDataSetInvalidated() {
+        //Doing nothing as of now


Most other comments have a space after //. Also, you may want to describe briefly what should happen in the future.

Sure, will do

pullrequest

Sorry, one of the comments landed on the wrong line.

Reviewed with ❤️ by PullRequest

pullrequest · 2019-07-04T13:12:46Z

app/src/main/java/fr/free/nrw/commons/contributions/ContributionViewHolder.java

+     */
+    private String getKeyForLRUCache(Uri contentUri) {
+        String[] split = contentUri.toString().split("/");
+        return split[split.length-1];


Consider using Uri.getLastPathSegment()—it seems to do what you want and is probably more efficient.

Thanks for letting me know, I will use the same :-)

maskaravivek

Looks good to me.

maskaravivek · 2019-07-07T06:06:16Z

app/src/main/java/fr/free/nrw/commons/contributions/ContributionsContract.java

+import fr.free.nrw.commons.BasePresenter;
+import fr.free.nrw.commons.Media;
+
+public class ContributionsContract {


Please add java docs

maskaravivek · 2019-07-07T06:07:05Z

app/src/main/java/fr/free/nrw/commons/contributions/ContributionsRepository.java

+import android.database.Cursor;
+import javax.inject.Inject;
+
+public class ContributionsRepository {


maskaravivek · 2019-07-07T06:07:53Z

app/src/main/java/fr/free/nrw/commons/contributions/ContributionViewHolder.java

+     * @param localUri
+     * @return
+     */
+    private boolean fileExists(Uri localUri) {


Move this to FileUtils

maskaravivek · 2019-07-07T06:08:17Z

app/src/main/java/fr/free/nrw/commons/contributions/ContributionViewHolder.java

 import timber.log.Timber;

-public class ContributionViewHolder implements ViewHolder<DisplayableContribution> {
+public class ContributionViewHolder extends RecyclerView.ViewHolder{


ashishkumar468 · 2019-07-07T07:17:26Z

@neslihanturan, the image list bug (wrong image being showed), can it be taken as a separate bug fix ? I think that is anyways existing as of now (in master). That seems to be somewhat related to the underlying image library+ListView combination.

neslihanturan · 2019-07-07T07:19:16Z

Sure @ashishkumar468 , other parts seems good to me. And I didn't experienced any issues on my previous tests except displaying wrong image. So I can merge it after tests passed.

ashishkumar468 · 2019-07-07T07:19:56Z

Thanks @neslihanturan :-)

ashishkumar468 added 2 commits June 30, 2019 23:54

* Refactored ContributionsListFragment to use RecyclerView

cd6ea1e

* Added ContributionsPresenter * Extracted out the cursor to presenter * Probable fix for commons-app#3028

Improved the logic for cache in ContributionViewHolder

2d6b81b

pullrequest bot reviewed Jun 30, 2019

View reviewed changes

Some more refactoring

a8ac3af

While displaying images in ContributionsList, check if status is not …

b6757cd

…completed && local uri exists, use that uri to show image

neslihanturan reviewed Jul 1, 2019

View reviewed changes

pullrequest bot reviewed Jul 4, 2019

View reviewed changes

ashishkumar468 added 9 commits July 7, 2019 09:03

typo correction in LocalDataSource

99c2e69

Fixed formatting in ContributionsPresenter

cfb45d7

retain adapter position when orientation changes

d76dd0a

retain child position with its id

d3d56c1

Made ContributionViewHolder not implement ViewHolder

b1810c0

Code formatting, review suggested changes

9f597fb

initialise the rv layout managers only when needed

ce2ca18

added test cases for ContributionPresenter

fc1b0df

removed not needed semi colon

e18fb14

maskaravivek reviewed Jul 7, 2019

View reviewed changes

added more java docs and code formatting

cfb3eb3

maskaravivek approved these changes Jul 7, 2019

View reviewed changes

neslihanturan approved these changes Jul 7, 2019

View reviewed changes

neslihanturan merged commit 60b1eb1 into commons-app:master Jul 7, 2019

PavelAplevich mentioned this pull request Jul 9, 2019

Achivements doesn't work #3058

Closed

ashishkumar468 deleted the feature/refactor-contributions branch August 14, 2020 08:54

ashishkumar468 mentioned this pull request Aug 14, 2020

Wrong photo displayed in ContributionsActivity during upload process [current master] #3028

Closed


		void registerDataSetObserver(DataSetObserver observer);

		void unregisterDataSetObserver(DataSetObserver observer);

Feature/refactor contributions #3046

Feature/refactor contributions #3046

Uh oh!

Conversation

ashishkumar468 commented Jun 30, 2019

Uh oh!

pullrequest bot left a comment

Choose a reason for hiding this comment

Uh oh!

pullrequest bot left a comment

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Jun 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

neslihanturan left a comment

Choose a reason for hiding this comment

Uh oh!

maskaravivek commented Jul 1, 2019

Uh oh!

ashishkumar468 commented Jul 2, 2019

Uh oh!

maskaravivek commented Jul 2, 2019

Uh oh!

misaochan commented Jul 2, 2019

Uh oh!

pullrequest bot left a comment

Choose a reason for hiding this comment

Uh oh!

pullrequest bot Jul 4, 2019

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pullrequest bot Jul 4, 2019

Choose a reason for hiding this comment

Uh oh!

ashishkumar468 Jul 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pullrequest bot Jul 4, 2019

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pullrequest bot Jul 5, 2019

Choose a reason for hiding this comment

Uh oh!

pullrequest bot Jul 4, 2019

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pullrequest bot Jul 4, 2019

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pullrequest bot Jul 4, 2019

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pullrequest bot Jul 4, 2019

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pullrequest bot Jul 5, 2019

Choose a reason for hiding this comment

Uh oh!

pullrequest bot Jul 4, 2019

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pullrequest bot Jul 5, 2019

Choose a reason for hiding this comment

codecov-io commented Jun 30, 2019 •

edited

Loading

ashishkumar468 Jul 4, 2019 •

edited

Loading