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

Fix: Uploaded Photos List Now Refreshes Instantly After Uploads (#6084) #6097

Closed
wants to merge 19 commits into from

Conversation

Sujal-Gupta-SG
Copy link
Contributor

@Sujal-Gupta-SG Sujal-Gupta-SG commented Jan 3, 2025

Description (required)

Fixes

Fixes #6084

What changes did you make and why?

  • Implemented a polling mechanism to fetch new contributions from the server once every day.
  • Introduced a method to check only the latest picture identifier instead of fetching the full list of contributions, reducing server and network load.
  • Added logic to append new contributions to the existing list without refreshing the UI unnecessarily, preserving the currently displayed items.
  • Integrated LiveData updates to ensure seamless UI updates with new contributions.
  • Used efficient background handling to manage the daily polling interval while ensuring minimal resource usage.

Tests performed (required)

  • Tested the changes on Realme Narzo 20 with API level 30-ext15 in the ProdDebug build variant.
  • Verified that the polling mechanism correctly fetches and appends new contributions without performing a full refresh.
  • Ensured that the app efficiently handles cases where there are no new contributions, avoiding unnecessary network requests.

Suggestions for polling frequency

Polling frequency has been set to once per day.
This ensures:

  1. Minimal resource usage on Wikimedia servers.
  2. Efficient app performance, as most users upload contributions infrequently.

To further optimize, the app fetches only the identifier of the latest picture. If the identifier does not match the app's last known contribution, a full refresh is triggered.

@neeldoshii
Copy link
Contributor

Hi @Sujal-Gupta-SG,
Thanks for your contributing. Next time when you raise a PR try to have a good PR title which relates to the mentioned issue rather than just fixed issue number in the title. It becomes easier for reviewer/maintainer to understand the issue.

I'll take a look at your PR tomorrow.

@Sujal-Gupta-SG Sujal-Gupta-SG changed the title Fixed #6084 Fixed #6084 List of uploaded photographs not refreshing when uploaded via desktop/laptop Jan 4, 2025
@Sujal-Gupta-SG Sujal-Gupta-SG changed the title Fixed #6084 List of uploaded photographs not refreshing when uploaded via desktop/laptop Fix: Uploaded Photos List Now Refreshes Instantly After Desktop/Laptop Uploads (#6084) Jan 4, 2025
Copy link
Member

@nicolas-raoul nicolas-raoul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like every 1 minute or even 15 minutes is too often, how about 1 per day?

@nicolas-raoul
Copy link
Member

99.99% of the time there will be no new contributions, as most users upload using the website only once in a while, and many never.
To save Wikimedia server resources, I suggest first just getting from the server the identifier of the latest picture, rather than a full list with details. Only if that does not match the app's latest picture, we can perform the refresh.

@Sujal-Gupta-SG
Copy link
Contributor Author

@nicolas-raoul I have make some changes, Can you check it Whether it fullfill the requirement

@Sujal-Gupta-SG Sujal-Gupta-SG changed the title Fix: Uploaded Photos List Now Refreshes Instantly After Desktop/Laptop Uploads (#6084) Fix: Uploaded Photos List Now Refreshes Instantly After Uploads (#6084) Jan 4, 2025
@neeldoshii
Copy link
Contributor

neeldoshii commented Jan 4, 2025

Thanks a lot for changing the title @Sujal-Gupta-SG :) also curious why do you have commit named as fixed #issue-name? Next time can you try having good commit title explaining relevant to the changes check this link. Also try as much as possible to not duplicating the pr (like #6096) and try to have it on same PR as it helps to keep a track on historical logs, any ways its fine for now :)

Haven't tested locally yet but I do have some opinions regarding pooling.

Implemented polling mechanism for fetching new contributions from the server every minute.

Polling every minute will end up consuming too much network consumption at both server side & client side both. Polling is usually done when we want to constantly at a specified regular interval want to check the newly changes, as per the code (git diff) the polling will break here and won't make sense though the polling time is 1 day the code will still fetch the api again when the view is created, it doesn't tracks the last time polled and for obvious reason one won't be on 24 hours on commons. Though the alternative is to store last fetch time in shared pref and diff it by current time and fetch it still constant calculation will consume memory on client side assuming people with low on memory will have laggy user-experience. I think we need to think on some other strategy.

Other Reason:-

  1. What if the user has uploaded an image at 12:00pm, 2:00 pm, 4:00 pm the image interval which were between this range will still feels that his/her contributions are not shown on the app the user might have done any mistake. Next day the user will get result but the duration between this the user might be having bad experience.

  2. Personally I don't like pooling, thats the last thing I will think to consider.
    Question❓: @Sujal-Gupta-SG does the app auto refresh desktop contributions when the app is loaded once the app is killed and reloaded which I think as per List of uploaded photographs not refreshing when uploaded via desktop/laptop #6084 (comment) it probably does so polling will be useless to adopt it.

If the pull to refresh is solving the issue then I don't think the issue still arises, the least we can think of adding is a refresh button at top which will reload the contribution similar to pull to refresh for synchronisation, still I support pull to refresh solves the issue.

Wdyt? Open for suggestions 😊

@Sujal-Gupta-SG
Copy link
Contributor Author

Sujal-Gupta-SG commented Jan 5, 2025

Thanks a lot for changing the title @Sujal-Gupta-SG :) also curious why do you have commit named as fixed #issue-name? Next time can you try having good commit title explaining relevant to the changes check this link. Also try as much as possible to not duplicating the pr (like #6096) and try to have it on same PR as it helps to keep a track on historical logs, any ways its fine for now :)

Haven't tested locally yet but I do have some opinions regarding pooling.

Implemented polling mechanism for fetching new contributions from the server every minute.

Polling every minute will end up consuming too much network consumption at both server side & client side both. Polling is usually done when we want to constantly at a specified regular interval want to check the newly changes, as per the code (git diff) the polling will break here and won't make sense though the polling time is 1 day the code will still fetch the api again when the view is created, it doesn't tracks the last time polled and for obvious reason one won't be on 24 hours on commons. Though the alternative is to store last fetch time in shared pref and diff it by current time and fetch it still constant calculation will consume memory on client side assuming people with low on memory will have laggy user-experience. I think we need to think on some other strategy.

Other Reason:-

  1. What if the user has uploaded an image at 12:00pm, 2:00 pm, 4:00 pm the image interval which were between this range will still feels that his/her contributions are not shown on the app the user might have done any mistake. Next day the user will get result but the duration between this the user might be having bad experience.
  2. Personally I don't like pooling, thats the last thing I will think to consider.
    Question❓: @Sujal-Gupta-SG does the app auto refresh desktop contributions when the app is loaded once the app is killed and reloaded which I think as per List of uploaded photographs not refreshing when uploaded via desktop/laptop #6084 (comment) it probably does so polling will be useless to adopt it.

If the pull to refresh is solving the issue then I don't think the issue still arises, the least we can think of adding is a refresh button at top which will reload the contribution similar to pull to refresh for synchronisation, still I support pull to refresh solves the issue.

Wdyt? Open for suggestions 😊

@neeldoshii Thank you for your feedback, and sorry for duplicating the PR. As a newcomer, this is my first time working on this, and I’m still learning the process, so I encountered a few issues. In the future, I will ensure to manage it properly. I’d also like to apologize for not updating the description earlier, which may have caused some confusion. Let me clarify the changes made:

  • Reduced Polling Frequency: As per @nicolas-raoul request, the app now polls once every 24 hours, minimizing network and server usage.
  • Efficient Data Fetching: Only the latest contribution's identifier is fetched first, and the full list is retrieved only if the identifier differs, reducing unnecessary requests.

Additionally, manual refresh options like pull-to-refresh are available for users who upload frequently, ensuring their contributions are updated immediately.

  • Polling Reset on Navigation or App Close: If the user navigates to a different section or the app is killed and reopened, the polling timer resets. This means that the user has to stay on the contributions section for 24 hours before another request is made to the server through polling.

I hope this clears up any confusion. Let me know your thoughts! 😊

@Sujal-Gupta-SG
Copy link
Contributor Author

@nicolas-raoul Please check this PR, and give me suggestion afterwards i can go to next issue to work on it

@neeldoshii
Copy link
Contributor

neeldoshii commented Jan 6, 2025

Hi @Sujal-Gupta-SG 👋. Hope you had good weekend! 🤠

Polling Reset on Navigation or App Close: If the user navigates to a different section or the app is killed and reopened, the polling timer resets. This means that the user has to stay on the contributions section for 24 hours before another request is made to the server through polling.

I am not sure why you want users to keep the app open for 24 hours? The pooling is useless if the timer resets if the lifecycle state is newly created or app is reopened I don't think the usage of pooling is needed? Btw, can you take a look at the reason I mentioned above why pooling might have bad user experience and lmk your thoughts on it.

99.99% of the time there will be no new contributions, as most users upload using the website only once in a while, and many never.

I am still not seeing any advantage of adding pooling for trading user experience for a feature which is hardly used. Open for your thoughts.

Thanks a lot :)

@Sujal-Gupta-SG
Copy link
Contributor Author

Sujal-Gupta-SG commented Jan 6, 2025

Hi @Sujal-Gupta-SG 👋. Hope you had good weekend! 🤠

Polling Reset on Navigation or App Close: If the user navigates to a different section or the app is killed and reopened, the polling timer resets. This means that the user has to stay on the contributions section for 24 hours before another request is made to the server through polling.

I am not sure why you want users to keep the app open for 24 hours? The pooling is useless if the timer resets if the lifecycle state is newly created or app is reopened I don't think the usage of pooling is needed? Btw, can you take a look at the reason I mentioned above why pooling might have bad user experience and lmk your thoughts on it.

99.99% of the time there will be no new contributions, as most users upload using the website only once in a while, and many never.

I am still not seeing any advantage of adding pooling for trading user experience for a feature which is hardly used. Open for your thoughts.

Thanks a lot :)

I am not sure why you want users to keep the app open for 24 hours?

I don't wanted user to keep the app open but if user uses the app for much time then it may be useful at that time and i don't think it uses much network and server usage.
Anyway if there is any good option we can go with that, you can tell me what i have to do, I'll try my best

Also @nicolas-raoul what's your opinion about this
Thanks a lot :)

@nicolas-raoul
Copy link
Member

(commented at #6084 (comment))

savsch and others added 14 commits January 15, 2025 22:52
* Rename .java to .kt

* Migrate Profile Package to Kotlin commons-app#6118 commons-app#5928

* Migrate Profile Package to Kotlin commons-app#6118 commons-app#5928

* Migrate Profile Package to Kotlin commons-app#6118 commons-app#5928
…, then suggest categories from its P18 (commons-app#6130)

* Fix NPE with UploadMediaDetails.captionText

* Store P18 instead of processed image url in DepictedItem

* Add routes for fetching category info from titles

* Consider depict's P18 when suggesting categories

* Add tests

* Corrected DepictedItem constructor arguments

* Add test for DepictedItem::primaryImage
…-app#6139)

* fix: correctly handle permission callbacks on Main thread

The PermissionUtils was incorrectly executing permission callbacks on a background thread, leading to a Handler error. Now, it is using Main dispatcher.

* fix crash when opening camera while having partial storage access
…e either uploaded or marked. (commons-app#6095)

* feat: show the message "Congratulations, all pictures in this album have been either uploaded or marked as not for upload." in the custom picker when all images are either uploaded or marked.

* refactor: fix indentation in the code

* refactor: replace LiveData with StateFlow

* fix: fixed the bug that was causing one ImageFragment testcase to fail.

* fix: fixed the Congratulation message being shown for a brief moment while switching off the switch

* refactor: move hardcoded string to strings.xml.

* docs: add comment to clarify visibility logic in ImageFragment

---------

Co-authored-by: Nicolas Raoul <[email protected]>
…mons-app#6126)

* Rename Constants to Follow Kotlin Naming Conventions

>This PR refactors constant names in the project to adhere to Kotlin's UPPERCASE_SNAKE_CASE naming convention, improving code readability and maintaining consistency across the codebase.

>Renamed the following constants in LoginActivity:
>saveProgressDialog → SAVE_PROGRESS_DIALOG
>saveErrorMessage → SAVE_ERROR_MESSAGE
>saveUsername → SAVE_USERNAME
>savePassword → SAVE_PASSWORD

>Updated all references to these constants throughout the project.

* Update Project_Default.xml

* Refactor variable names to adhere to naming conventions

Renamed variables to use camel case:
-UPLOAD_COUNT_THRESHOLD → uploadCountThreshold
-REVERT_PERCENTAGE_FOR_MESSAGE → revertPercentageForMessage
-REVERT_SHARED_PREFERENCE → revertSharedPreference
-UPLOAD_SHARED_PREFERENCE → uploadSharedPreference

Renamed variables with uppercase initials to lowercase for alignment with Kotlin conventions:
-Latitude → latitude
-Longitude → longitude
-Accuracy → accuracy

Refactored the following variable names:
-NUMBER_OF_QUESTIONS → numberOfQuestions
-MULTIPLIER_TO_GET_PERCENTAGE → multiplierToGetPercentage

* Refactor Dialog View Initialization with Null-Safe Calls

This PR refactors the dialog setup code in CustomSelectorActivity to improve safety and readability by replacing explicit casts with null-safe generic calls for findViewById.

>Replaced explicit casting (as Button and as TextView) with the generic findViewById<T>() method for improved type safety.
>Added null-safety (?.) to avoid potential crashes if a view is not found in the dialog layout.

why changed:-
>Prevents runtime crashes caused by NullPointerException when a view is missing in the layout.

* Refactor Unit Test: Replace Unsafe Casting with Type-Safe Mocking for findViewById

>PR refactors the unit test for NearbyParentFragment by replacing unsafe casting in the findViewById mocking statements with type-safe

>Ensured all findViewById mocks now use a consistent, type-safe format (findViewById<View>(...)) to reduce verbosity and potential casting errors.

>Verified the functionality of prepareViewsForSheetPosition remains unchanged, ensuring no regression in test behavior.

* Update NearbyParentFragmentUnitTest.kt

* Refactor: Rename Constants to Follow CamelCase Naming Convention

>Updated all constant variable names to follow the camelCase naming convention, removing underscores in the middle or end.

>Ensured variable names remain descriptive and align with code readability best practices.

* Replace private val with const val for URL constants in QuizController

* Renaming the constant to use UPPER_SNAKE_CASE

* Renaming the constant to use UPPER_SNAKE_CASE

* Update Done

* **Refactor: Convert `minimumThresholdForSwipe` to a compile-time constant**

---------

Co-authored-by: Nicolas Raoul <[email protected]>
…s loading spinwheel) we check whether the server's last image is the app's last image.

If not, refresh.
New "refresh" button having the same effect as swipe down.
# Conflicts:
#	app/src/main/java/fr/free/nrw/commons/contributions/ContributionsListPresenter.java
#	app/src/main/java/fr/free/nrw/commons/contributions/ContributionsRemoteDataSource.kt
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

List of uploaded photographs not refreshing when uploaded via desktop/laptop
10 participants