Fixes #4589 :- The empty screen while no images found in custom picture selector #4863

Rishavgupta12345 · 2022-02-28T07:01:23Z

The empty screen while no images found in custom picture selector #4589 is fixed now

I was solving this issue in the Image fragment as mentioned by @4D17Y4 but it created a few unwanted bugs so I have to delete my previous pull request, I created a new one with a different approach and I as this is working perfectly so, for now, this can be an ideal solution

@nicolas-raoul screenshots added

No image found

Image found

codecov · 2022-02-28T07:19:54Z

Codecov Report

Merging #4863 (c02f38c) into master (9431e37) will decrease coverage by 0.17%.
The diff coverage is 37.50%.

❗ Current head c02f38c differs from pull request most recent head 35ca4dc. Consider uploading reports for the commit 35ca4dc to get more accurate results

@@             Coverage Diff              @@
##             master    #4863      +/-   ##
============================================
- Coverage     50.57%   50.40%   -0.18%     
+ Complexity     2224     2215       -9     
============================================
  Files           338      338              
  Lines         15546    15561      +15     
  Branches       1359     1362       +3     
============================================
- Hits           7862     7843      -19     
- Misses         7100     7133      +33     
- Partials        584      585       +1

Impacted Files	Coverage Δ
...stomselector/ui/selector/CustomSelectorActivity.kt	`72.00% <37.50%> (-6.83%)`	⬇️
...ava/fr/free/nrw/commons/media/MwParseResponse.java	`0.00% <0.00%> (-40.00%)`	⬇️
...fr/free/nrw/commons/review/ReviewPagerAdapter.java	`30.76% <0.00%> (-38.47%)`	⬇️
...r/free/nrw/commons/explore/media/MediaConverter.kt	`71.42% <0.00%> (-23.81%)`	⬇️
...a/fr/free/nrw/commons/OkHttpConnectionFactory.java	`52.27% <0.00%> (-20.46%)`	⬇️
...main/java/fr/free/nrw/commons/media/MediaClient.kt	`87.75% <0.00%> (-6.13%)`	⬇️
.../java/fr/free/nrw/commons/review/ReviewHelper.java	`77.77% <0.00%> (-5.56%)`	⬇️
...ava/fr/free/nrw/commons/review/ReviewActivity.java	`47.28% <0.00%> (-3.11%)`	⬇️
...w/commons/contributions/ContributionsFragment.java	`31.59% <0.00%> (-0.33%)`	⬇️
... and 2 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 9431e37...35ca4dc. Read the comment docs.

arinmodi · 2022-02-28T11:34:54Z

Review by student

Before the Changes :

before.mp4

After the Changes :

after.mp4

Status : issue solved

Rishavgupta12345 · 2022-02-28T12:13:01Z

Review by student

Before the Changes :

before.mp4

After the Changes :

after.mp4
Status : issue solved

thanks a lot for the review

nicolas-raoul

Wonderful! Thanks for adding javadoc.
Just a few requests added.

And thanks Arin for the review!

nicolas-raoul · 2022-03-02T12:27:26Z

app/src/main/java/fr/free/nrw/commons/customselector/ui/selector/CustomSelectorActivity.kt

+        if(isImageGalleryEmpty()) {
+            val linearLayout: LinearLayout = findViewById<View>(R.id.linear_layout_noimage) as LinearLayout
+            val textView = TextView(this)
+            textView.text = "No Image Found"


Could you please make this string localizable? See how internationalization is done for other strings of the app.
Also, could you make the English one No images found. (plural, only first letter uppercase, point at the end)

nicolas-raoul · 2022-03-02T12:29:44Z

app/src/main/java/fr/free/nrw/commons/customselector/ui/selector/CustomSelectorActivity.kt

+
+        // Checks if the Image gallery is empty.
+        if(isImageGalleryEmpty()) {
+            val linearLayout: LinearLayout = findViewById<View>(R.id.linear_layout_noimage) as LinearLayout


Unit test for this block

nicolas-raoul · 2022-03-02T12:29:51Z

app/src/main/java/fr/free/nrw/commons/customselector/ui/selector/CustomSelectorActivity.kt

+     */
+    private fun isImageGalleryEmpty(): Boolean {
+        try {
+            val projection = arrayOf(MediaStore.Images.Media._ID)


Unit test for this block

4D17Y4 · 2022-03-02T12:50:34Z

Hey @Rishavgupta12345,
Seems like u have reverted to your previous approach. Any reasons ?
You mentioned unwanted bugs, would be nice if you could list them. I am sure we can figure out those.

The basic reason why we don't want to go with this approach is that it queries images again which is inefficient as we are just concerned with the number of images returned. So I suggested to handle this in the results function. On a plus point all the test are done on the custom selector functions, major changes wont be necessary.

Rishavgupta12345 · 2022-03-02T13:10:34Z

Hey @Rishavgupta12345, Seems like u have reverted to your previous approach. Any reasons ? You mentioned unwanted bugs, would be nice if you could list them. I am sure we can figure out those.

The basic reason why we don't want to go with this approach is that it queries images again which is inefficient as we are just concerned with the number of images returned. So I suggested to handle this in the results function. On a plus point all the test are done on the custom selector functions, major changes wont be necessary.

I am not getting the perfect way of implementing it on Image fragments, because there I tried some approaches and they are not working as they should

4D17Y4 · 2022-03-02T13:33:09Z

@Rishavgupta12345 you can try again if you want. I'll unassign the issue for now so that others can work on it. In case you plan to work on it again you can ping us to get assigned.

Rishavgupta12345 · 2022-03-02T13:42:24Z

@Rishavgupta12345 you can try again if you want. I'll unassign the issue for now so that others can work on it. In case you plan to work on it again you can ping us to get assigned.

don't un assign me I will work on it

Rishavgupta12345 · 2022-03-04T14:50:26Z

Hey @Rishavgupta12345, Seems like u have reverted to your previous approach. Any reasons ? You mentioned unwanted bugs, would be nice if you could list them. I am sure we can figure out those.
The basic reason why we don't want to go with this approach is that it queries images again which is inefficient as we are just concerned with the number of images returned. So I suggested to handle this in the results function. On a plus point all the test are done on the custom selector functions, major changes wont be necessary.

I am not getting the perfect way of implementing it on Image fragments, because there I tried some approaches and they are not working as they should

@4D17Y4 If I am making this in image and folder fragment then can I make another function isImageGalleryEmpty() and call them inside the handleResult() funtion? like I did in this pull request

4D17Y4 · 2022-03-04T16:28:06Z

Yes you can do that. This time you don't have to query for the images.

Rishavgupta12345 · 2022-03-05T05:45:08Z

Yes you can do that. This time you don't have to query for the images.

@4D17Y4 @nicolas-raoul finally done in best possible way. Ready to merge

4D17Y4

Best approach indeed.
Nice work.
Just some minor changes.

app/src/main/res/layout/fragment_custom_selector.xml

app/src/main/java/fr/free/nrw/commons/customselector/ui/selector/FolderFragment.kt

Rishavgupta12345 · 2022-03-06T06:44:17Z

Best approach indeed. Nice work. Just some minor changes.

@4D17Y4 done the requested changes

4D17Y4 · 2022-03-07T09:30:45Z

Works.
Good to merge.
Any final edits @nicolas-raoul ?

madhurgupta10 · 2022-03-07T10:09:10Z

@Rishavgupta12345 It seems like 2 of the tests are failing after this PR was merged, can you take a look into it, please?

fr.free.nrw.commons.customselector.ui.selector.ImageFragmentTest > testHandleResult FAILED
    java.lang.Exception at AndroidTestEnvironment.java:538
        Caused by: java.lang.reflect.InvocationTargetException at ImageFragmentTest.kt:128
            Caused by: java.lang.NullPointerException at ImageFragmentTest.kt:128

fr.free.nrw.commons.customselector.ui.selector.FolderFragmentTest > testHandleResult FAILED
    java.lang.Exception at AndroidTestEnvironment.java:538
        Caused by: java.lang.reflect.InvocationTargetException at FolderFragmentTest.kt:128
            Caused by: java.lang.NullPointerException at FolderFragmentTest.kt:128

Rishavgupta12345 · 2022-03-07T15:04:43Z

@Rishavgupta12345 It seems like 2 of the tests are failing after this PR was merged, can you take a look into it, please?

fr.free.nrw.commons.customselector.ui.selector.ImageFragmentTest > testHandleResult FAILED
    java.lang.Exception at AndroidTestEnvironment.java:538
        Caused by: java.lang.reflect.InvocationTargetException at ImageFragmentTest.kt:128
            Caused by: java.lang.NullPointerException at ImageFragmentTest.kt:128

fr.free.nrw.commons.customselector.ui.selector.FolderFragmentTest > testHandleResult FAILED
    java.lang.Exception at AndroidTestEnvironment.java:538
        Caused by: java.lang.reflect.InvocationTargetException at FolderFragmentTest.kt:128
            Caused by: java.lang.NullPointerException at FolderFragmentTest.kt:128

I have never faced any issue like this before , so can you please guide me what should I do now ?

BY my knowledge i think I have to make some changes in the tests as new things were added which doesn't have tests on it

madhurgupta10 · 2022-03-07T16:06:39Z

You can run the tests locally from Android Studio and see the logs why they are failing and update the tests accordingly.

madhurgupta10 · 2022-03-08T15:04:59Z

@Rishavgupta12345 can you prioritise this issue over others for the time, build fails make it difficult for us to review PRs.

Rishavgupta12345 · 2022-03-09T03:12:53Z

@Rishavgupta12345 can you prioritise this issue over others for the time, build fails make it difficult for us to review PRs.

okay

Rishavgupta12345 · 2022-03-09T06:47:27Z

@Rishavgupta12345 can you prioritise this issue over others for the time, build fails make it difficult for us to review PRs.

@madhurgupta10 I have done the changes, merge #4883 and its done

no image found

85442c0

nicolas-raoul requested changes Mar 2, 2022

View reviewed changes

old removed

49b46cc

Rishavgupta12345 added 2 commits March 5, 2022 11:06

new way

362fb00

minor changes

a5b262f

4D17Y4 suggested changes Mar 5, 2022

View reviewed changes

app/src/main/res/layout/fragment_custom_selector.xml Show resolved Hide resolved

app/src/main/java/fr/free/nrw/commons/customselector/ui/selector/FolderFragment.kt Outdated Show resolved Hide resolved

reqeusted changes done

35ca4dc

nicolas-raoul approved these changes Mar 7, 2022

View reviewed changes

nicolas-raoul merged commit c02d569 into commons-app:master Mar 7, 2022

devarsh-mavani-19 mentioned this pull request Mar 7, 2022

In app feedback #4845

Merged

This was referenced Mar 9, 2022

Fixes #4589 :- The empty screen while no images found in custom picture selector [bug] #4883

Closed

Fixes #4589 :- The empty screen while no images found in custom picture selector [bug] #4884

Merged

Rishavgupta12345 mentioned this pull request Mar 11, 2022

Unit tests are failing #4888

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #4589 :- The empty screen while no images found in custom picture selector #4863

Fixes #4589 :- The empty screen while no images found in custom picture selector #4863

Rishavgupta12345 commented Feb 28, 2022

codecov bot commented Feb 28, 2022 •

edited

Loading

arinmodi commented Feb 28, 2022

Rishavgupta12345 commented Feb 28, 2022

nicolas-raoul left a comment

nicolas-raoul Mar 2, 2022

nicolas-raoul Mar 2, 2022

nicolas-raoul Mar 2, 2022

4D17Y4 commented Mar 2, 2022 •

edited

Loading

Rishavgupta12345 commented Mar 2, 2022

4D17Y4 commented Mar 2, 2022 •

edited

Loading

Rishavgupta12345 commented Mar 2, 2022

Rishavgupta12345 commented Mar 4, 2022

4D17Y4 commented Mar 4, 2022

Rishavgupta12345 commented Mar 5, 2022

4D17Y4 left a comment

Rishavgupta12345 commented Mar 6, 2022

4D17Y4 commented Mar 7, 2022

madhurgupta10 commented Mar 7, 2022

Rishavgupta12345 commented Mar 7, 2022

madhurgupta10 commented Mar 7, 2022

madhurgupta10 commented Mar 8, 2022 •

edited

Loading

Rishavgupta12345 commented Mar 9, 2022

Rishavgupta12345 commented Mar 9, 2022 •

edited

Loading

Fixes #4589 :- The empty screen while no images found in custom picture selector #4863

Fixes #4589 :- The empty screen while no images found in custom picture selector #4863

Conversation

Rishavgupta12345 commented Feb 28, 2022

codecov bot commented Feb 28, 2022 • edited Loading

Codecov Report

arinmodi commented Feb 28, 2022

Rishavgupta12345 commented Feb 28, 2022

nicolas-raoul left a comment

Choose a reason for hiding this comment

nicolas-raoul Mar 2, 2022

Choose a reason for hiding this comment

nicolas-raoul Mar 2, 2022

Choose a reason for hiding this comment

nicolas-raoul Mar 2, 2022

Choose a reason for hiding this comment

4D17Y4 commented Mar 2, 2022 • edited Loading

Rishavgupta12345 commented Mar 2, 2022

4D17Y4 commented Mar 2, 2022 • edited Loading

Rishavgupta12345 commented Mar 2, 2022

Rishavgupta12345 commented Mar 4, 2022

4D17Y4 commented Mar 4, 2022

Rishavgupta12345 commented Mar 5, 2022

4D17Y4 left a comment

Choose a reason for hiding this comment

Rishavgupta12345 commented Mar 6, 2022

4D17Y4 commented Mar 7, 2022

madhurgupta10 commented Mar 7, 2022

Rishavgupta12345 commented Mar 7, 2022

madhurgupta10 commented Mar 7, 2022

madhurgupta10 commented Mar 8, 2022 • edited Loading

Rishavgupta12345 commented Mar 9, 2022

Rishavgupta12345 commented Mar 9, 2022 • edited Loading

codecov bot commented Feb 28, 2022 •

edited

Loading

4D17Y4 commented Mar 2, 2022 •

edited

Loading

4D17Y4 commented Mar 2, 2022 •

edited

Loading

madhurgupta10 commented Mar 8, 2022 •

edited

Loading

Rishavgupta12345 commented Mar 9, 2022 •

edited

Loading