Fix recent album not showing count #17498
Merged
Conversation
This file contains 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
flexsurfer
requested changes
Oct 4, 2023
| {:title (i18n/label :t/my-albums) :data (:my-albums albums)}] | ||
| window-height (:height (rn/get-window))] | ||
| (let [albums (rf/sub [:camera-roll/albums]) | ||
| add-recent-count-to-recent-album (-> albums |
There was a problem hiding this comment.
this should be inside subscription :camera-roll/albums-with-count
There was a problem hiding this comment.
I went with a new subscription of :camera-roll/total-photos-count
There was a problem hiding this comment.
Kindly take a look
Jenkins BuildsClick to see older builds (104)
|
flexsurfer
reviewed
Oct 4, 2023
| {:db (assoc db :camera-roll/albums albums)}) | ||
| {:db (-> db | ||
| (assoc :camera-roll/albums albums) | ||
| (assoc :camera-roll/total-photos-count |
There was a problem hiding this comment.
still better to move to a subscription
There was a problem hiding this comment.
sometimes its better to make this in events yes, but not in this case
There was a problem hiding this comment.
Do you mind taking another look?
flexsurfer
approved these changes
Oct 4, 2023
60% of end-end tests have passedFailed tests (17)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Passed tests (26)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePRTwo:
|
|
Hi @ibrkhalil! Thanx for the PR. Please take a look at the issues ISSUE 1 Recent album count number is incorrect for IOSActual result: Recent album shows the sum of images of the rest of the albums. But in fact, Recent album includes bunch of images which do not belong to other albums. So such count logic cannot be applied for IOS
Expected result: here is the actual number of images that Recent album includes.
|
J-Son89
reviewed
Oct 4, 2023
J-Son89
approved these changes
Oct 4, 2023
OmarBasem
reviewed
Oct 5, 2023
src/status_im2/subs/chat/chats.cljs
Outdated
Comment on lines
441
to
450
|
|
||
| (re-frame/reg-sub | ||
| :camera-roll/total-photos-count | ||
| (fn [{:keys [camera-roll/albums]}] | ||
| (->> albums | ||
| :my-albums | ||
| (reduce | ||
| (fn [total-album-count curr-album] | ||
| (+ total-album-count (:count curr-album))) | ||
| 0)))) |
There was a problem hiding this comment.
This is incorrect. This will not get you the total photos count, but only the total of photos that are classified into albums. The camera roll library we are using does not have an option to get the total photo count. This needs to be done in native code.
There was a problem hiding this comment.
I am back Saturday and will implement it, Thanks Omar!
There was a problem hiding this comment.
Done PR here
|
@pavloburykh |
|
Done, And as per request from @OmarBasem this adds a method that returns favorites and their count fe93660 |
|
@ibrkhalil please take a look at the new issue ISSUE 2 Scrolling through Recent album results in loading the same images over and over (Android)Reproducing Android 12, Samsung Galaxy A52 Steps:
Actual result:
telegram-cloud-document-2-5271698414512386993.mp4 |
Fixed. |
|
@ibrkhalil could you please rebase the PR and resolve conflicts? |
60% of end-end tests have passedFailed tests (14)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityMultipleDeviceMerged:
Expected to fail tests (4)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Passed tests (27)Click to expandClass TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
|
|
Should be working now. |
73% of end-end tests have passedFailed tests (9)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Expected to fail tests (3)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Passed tests (33)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityOneDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
|
ISSUE 6: on Android 10 images are not shown in all albumsThis is the reason for failed e2e. FILE.2023-10-23.15.35.55.mp4Logs: Status-debug-logs.zip |
|
Cross-posting with #17241 (comment) |
Fixed. |
87% of end-end tests have passedFailed tests (3)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMergedTwo:
Expected to fail tests (3)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (39)Click to expandClass TestActivityMultipleDevicePR:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
|
|
@ibrkhalil something is wrong with the builds after recent rebase. Please take a look 
|
|
@pavloburykh Should be working now, Thank you! |
@ibrkhalil is it okay that there is still warning from packages-check-bot? 
|
It's just a warning about a library's git endpoint wanting to end with .git, But unfortunately adding .git to its path doesn't work. (And it's not required to merge, Which I think it means we can ignore it) |
|
@ibrkhalil thank you for the PR. Tested and ready for merge. |
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.
fixes #17486
Summary
Fixes an issue that caused the recent album in the photo picker to not have the count of all images rendered.
status: ready