In depictions selection screen, suggest recently selected items #4361

Prince-kushwaha · 2021-04-17T11:05:22Z

Description (required)

What changes did you make and why?

Implement RoomDataBase to save recent selected item of DepictedItem
when query is empty show the recent selected item of DepictedItem in recyclerview

Tests performed (required)
Android Device of API(25,29)

Screenshots (for UI changes only)

… items *use RoomDataBase * Add Javadoc

codecov-commenter · 2021-04-17T11:16:53Z

Codecov Report

Merging #4361 (0bd93b5) into master (4bca142) will decrease coverage by 0.05%.
The diff coverage is 1.88%.

@@             Coverage Diff              @@
##             master    #4361      +/-   ##
============================================
- Coverage     10.22%   10.17%   -0.06%     
  Complexity      469      469              
============================================
  Files           342      343       +1     
  Lines         13084    13156      +72     
  Branches       1062     1078      +16     
============================================
  Hits           1338     1338              
- Misses        11678    11749      +71     
- Partials         68       69       +1

Impacted Files	Coverage Δ	Complexity Δ
...rc/main/java/fr/free/nrw/commons/db/AppDatabase.kt	`0.00% <ø> (ø)`	`0.00 <0.00> (ø)`
...c/main/java/fr/free/nrw/commons/db/Converters.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`
.../free/nrw/commons/di/CommonsApplicationModule.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`
...n/java/fr/free/nrw/commons/upload/UploadModel.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`
...a/fr/free/nrw/commons/upload/depicts/DepictsDao.kt	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
...ee/nrw/commons/upload/depicts/DepictsFragment.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`
...ree/nrw/commons/upload/depicts/DepictsPresenter.kt	`85.71% <0.00%> (-6.60%)`	`12.00 <0.00> (ø)`
...ommons/upload/structure/depictions/DepictedItem.kt	`88.67% <100.00%> (ø)`	`16.00 <1.00> (ø)`
...rw/commons/bookmarks/BookmarkListRootFragment.java	`8.65% <0.00%> (-0.17%)`	`1.00% <0.00%> (ø%)`
...va/fr/free/nrw/commons/explore/SearchActivity.java	`0.00% <0.00%> (ø)`	`0.00% <0.00%> (ø%)`
... and 12 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 4bca142...0bd93b5. Read the comment docs.

nicolas-raoul

It mostly runs great, good work!
I started testing it and I noticed a problem: when you search for a depiction (depiction1) select it, then empty the text from the search bar (for instance to search for another depiction depiction2), the selected depiction1 is not visible anymore.

Prince-kushwaha · 2021-04-18T03:26:43Z

ok I fix it

nicolas-raoul

The problem I mentioned in my previous comment seems to be fixed, thanks! :-)

nicolas-raoul · 2021-04-19T12:27:51Z

app/src/main/java/fr/free/nrw/commons/upload/depicts/DepictsDao.kt

+import androidx.room.*
+
+@Dao
+interface DepictsDao {


What is the equivalent class for categories? (the DAO that saves previously selected categories)

nicolas-raoul

I noticed a small bug:

Feel free to either fix it now, or create a new issue to solve later, as it is not a big issue compared to the merits that history brings.

nicolas-raoul · 2021-04-19T12:28:37Z

app/src/main/java/fr/free/nrw/commons/upload/depicts/DepictsFragment.java

+    /**
+     * Get the depictesItem form DepictsRoomdataBase
+     */
+    List<DepictedItem> getRecentDepictesItem(final Application application) {


getRecentDepictesItem -> getRecentDepictedItems

neslihanturan

Some bad practices found.

neslihanturan · 2021-04-19T14:08:24Z

app/src/main/java/fr/free/nrw/commons/upload/depicts/DepictsRoomDataBase.kt

+            // if it is, then create the database
+            return INSTANCE ?: synchronized(this) {
+                val instance = Room.databaseBuilder(
+                    context.applicationContext,


to get the context here, you have passed application at the beginning of method calls. No need to pass Application, just pass the context.

Why do we need to create a new Database? Why are we not using the existing database and adding a table in that?

ashishkumar468

@Prince-kushwaha Thanks for the PR, even though this PR solves the issue, I would very much recommend to rediscuss the approach on the thread and probably revise the PR with the suggestions

ashishkumar468 · 2021-04-19T16:36:41Z

app/src/main/java/fr/free/nrw/commons/upload/depicts/DepictsFragment.java

@@ -149,7 +153,7 @@ public void setDepictsList(List<DepictedItem> depictedItemList) {

    @OnClick(R.id.depicts_next)
    public void onNextButtonClicked() {
-        presenter.verifyDepictions();
+        presenter.verifyDepictions(getActivity().getApplication());


Why are we passing the context to the presenter? Let it be the other way around, the presenter should ask the view for the things which need context

app/src/main/java/fr/free/nrw/commons/upload/depicts/DepictsFragment.java

ashishkumar468 · 2021-04-19T16:39:25Z

app/src/main/java/fr/free/nrw/commons/upload/depicts/DepictsPresenter.kt

        if (repository.selectedDepictions.isNotEmpty()) {
+            if (application.applicationContext != null) {


We have dagger in place for DB read write operations, we need not worry about the context, please refer other similar classes to see how we have used the corresponding DAO's to query the DB

ashishkumar468 · 2021-04-19T16:40:11Z

app/src/main/java/fr/free/nrw/commons/upload/depicts/DepictsRoomDataBase.kt

+            // if it is, then create the database
+            return INSTANCE ?: synchronized(this) {
+                val instance = Room.databaseBuilder(
+                    context.applicationContext,


Why do we need to create a new Database? Why are we not using the existing database and adding a table in that?

ashishkumar468 · 2021-04-19T16:41:20Z

app/src/main/java/fr/free/nrw/commons/upload/depicts/handleDepictsDoa.kt

@@ -0,0 +1,63 @@
+package fr.free.nrw.commons.upload.depicts


This is not needed I guess, Room handles the threading part on its own, even if it does not why make a separate class and not let the callers switch threads as needed?

ashishkumar468 · 2021-04-19T16:42:21Z

app/src/main/java/fr/free/nrw/commons/upload/UploadModel.java

@@ -177,8 +178,10 @@ public void deletePicture(final String filePath) {
    public void onDepictItemClicked(DepictedItem depictedItem) {
        if (depictedItem.isSelected()) {
            selectedDepictions.add(depictedItem);
+            DepictsFragment.selectedDepictedItem.add(depictedItem);


We should ideally avoid static variables, can this not be done via callbacks?

i thing no can you tell me other way

It has to be done with "callbacks". You can basically search how to avoid static variables and how to use callbacks and you will find several online examples that shows you the required structure.

Prince-kushwaha · 2021-04-19T17:30:03Z

@ashishkumar468 contributionDao is implement in nice way i change DepictsDao like this and use AppDataBase to store table
AppDatabase file is in db folder contribution data is also save in AppDataBase

Prince-kushwaha · 2021-04-19T17:37:25Z

@ashishkumar468 Room need thread except livedata for threading what i use kotlin coroutines or rxandroid ?

neslihanturan · 2021-05-24T09:43:04Z

It seems like there are still several reviews to be fixed on this PR, am I wrong?

Prince-kushwaha · 2021-05-26T03:09:20Z

@neslihanturan i fixed all the issue in the PR in commit improve implemention strategy

"

app/src/main/java/fr/free/nrw/commons/db/Converters.java

app/src/main/java/fr/free/nrw/commons/di/CommonsApplicationModule.java

app/src/main/java/fr/free/nrw/commons/upload/depicts/DepictsDao.kt

nicolas-raoul · 2021-05-26T05:42:13Z

app/src/main/java/fr/free/nrw/commons/upload/depicts/DepictsPresenter.kt

            view.goToNextScreen()
        } else {
            view.noDepictionSelected()
        }
    }
+
+    /**
+     * Get the depictesItem form DepictsRoomdataBase


Typo: depictesItem

nicolas-raoul · 2021-05-27T01:33:14Z

I just tested, it works perfectly :-)

neslihanturan · 2021-05-27T13:40:41Z

Seems like we can merge this one as soon as small fixes are done. @Prince-kushwaha can you let us know if you are not interested anymore or plan to continue?

Prince-kushwaha · 2021-05-27T14:03:33Z

@neslihanturan i work

Prince-kushwaha · 2021-05-29T05:05:31Z

@nicolas-raoul @neslihanturan i fixed small issue you can merge this

nicolas-raoul · 2021-05-31T10:20:43Z

Thanks, very useful addition, it will save users a lot of time! :-)

…ons-app#4361) * implement in depictions selection screen to suggest recently selected items *use RoomDataBase * Add Javadoc * fix an bug * minar change and remove extra line of code * minar changes * improve implemention strategy * fix unittest * Add javadoc * added javadoc

implement in depictions selection screen to suggest recently selected…

c8592a5

… items *use RoomDataBase * Add Javadoc

nicolas-raoul requested changes Apr 18, 2021

View reviewed changes

fix an bug

fbdc734

neslihanturan mentioned this pull request Apr 19, 2021

Achievements shows zero Nearby uploads, despite leaderboard showing I have hundreds Nearby uploads #4365

Closed

nicolas-raoul reviewed Apr 19, 2021

View reviewed changes

nicolas-raoul requested changes Apr 19, 2021

View reviewed changes

neslihanturan requested changes Apr 19, 2021

View reviewed changes

ashishkumar468 requested changes Apr 19, 2021

View reviewed changes

Prince-kushwaha added 2 commits April 20, 2021 14:09

minar change and remove extra line of code

72a6951

minar changes

0bd93b5

Prince-kushwaha requested review from ashishkumar468 and neslihanturan April 21, 2021 06:16

Prince-kushwaha requested a review from nicolas-raoul May 4, 2021 04:18

Prince-kushwaha added 2 commits May 11, 2021 14:52

improve implemention strategy

0a2d0be

fix unittest

baf85b8

nicolas-raoul requested changes May 26, 2021

View reviewed changes

Add javadoc

8fd2357

added javadoc

4a1d85f

Prince-kushwaha requested a review from nicolas-raoul May 30, 2021 13:52

nicolas-raoul approved these changes May 31, 2021

View reviewed changes

nicolas-raoul merged commit 4fa18e5 into commons-app:master May 31, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

In depictions selection screen, suggest recently selected items #4361

In depictions selection screen, suggest recently selected items #4361

Prince-kushwaha commented Apr 17, 2021

codecov-commenter commented Apr 17, 2021 •

edited

Loading

nicolas-raoul left a comment

Prince-kushwaha commented Apr 18, 2021

nicolas-raoul left a comment

nicolas-raoul Apr 19, 2021

nicolas-raoul left a comment

nicolas-raoul Apr 19, 2021

neslihanturan left a comment

neslihanturan Apr 19, 2021

ashishkumar468 Apr 19, 2021

ashishkumar468 left a comment

ashishkumar468 Apr 19, 2021

ashishkumar468 Apr 19, 2021

ashishkumar468 Apr 19, 2021

ashishkumar468 Apr 19, 2021

ashishkumar468 Apr 19, 2021

Prince-kushwaha Apr 20, 2021

neslihanturan May 24, 2021

Prince-kushwaha commented Apr 19, 2021

Prince-kushwaha commented Apr 19, 2021

neslihanturan commented May 24, 2021

Prince-kushwaha commented May 26, 2021

nicolas-raoul May 26, 2021

nicolas-raoul May 26, 2021

nicolas-raoul commented May 27, 2021

neslihanturan commented May 27, 2021

Prince-kushwaha commented May 27, 2021

Prince-kushwaha commented May 29, 2021

nicolas-raoul commented May 31, 2021

		if (repository.selectedDepictions.isNotEmpty()) {
		if (application.applicationContext != null) {

In depictions selection screen, suggest recently selected items #4361

In depictions selection screen, suggest recently selected items #4361

Conversation

Prince-kushwaha commented Apr 17, 2021

codecov-commenter commented Apr 17, 2021 • edited Loading

Codecov Report

nicolas-raoul left a comment

Choose a reason for hiding this comment

Prince-kushwaha commented Apr 18, 2021

nicolas-raoul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-raoul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neslihanturan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashishkumar468 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Prince-kushwaha commented Apr 19, 2021

Prince-kushwaha commented Apr 19, 2021

neslihanturan commented May 24, 2021

Prince-kushwaha commented May 26, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicolas-raoul commented May 27, 2021

neslihanturan commented May 27, 2021

Prince-kushwaha commented May 27, 2021

Prince-kushwaha commented May 29, 2021

nicolas-raoul commented May 31, 2021

codecov-commenter commented Apr 17, 2021 •

edited

Loading