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.

Sign up for GitHub

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

Jump to bottom

fix: refactor navigation on User Profile Screen and fix avatar upload bugs [AR-1204][AR-1210][AR-1211] #427

Merged
merged 19 commits into from
Mar 10, 2022
Merged

fix: refactor navigation on User Profile Screen and fix avatar upload bugs [AR-1204][AR-1210][AR-1211] #427

merged 19 commits into from
Mar 10, 2022

Conversation

gongracr
Copy link
Contributor

@gongracr gongracr commented Mar 9, 2022 •
edited by github-actions bot
Loading

SubtaskAR-1204 Refactor navigation between UserProfile and Avatar Picker screens

PR Submission Checklist for internal contributors

  • The PR Title

    • conforms to the style of semantic commits messages¹ supported in Wire's Github Workflow²
    • contains a reference JIRA issue number like SQPIT-764
    • answers the question: If merged, this PR will: ... ³

  • The PR Description

    • is free of optional paragraphs and you have filled the relevant parts to the best of your ability

What's new in this PR?

Issues

  • Currently the navigation between the UserProfileScreen and the AvatarPickerScreen was made via the UserProfileViewModel instead of using its own View Model.
  • The way we had previously to share the picked image on the gallery/camera was also broken, hence not allowing to update the Avatar Icon on the UserProfileScreen. Also, the Camera option, was taking pictures on preview mode, hence pixelating the image too much.
  • The User Profile didn't have any persistence load/save mechanisms, making the whole process of picking a new image, only available during the ongoing session.

Solutions

  • Added a new AvatarPickerViewModel to deal with the avatar picker logic.
  • Added a new file provider to allow the Android Camera save the taken picture on a specific internal path.
  • Delegated the process of uploading the picture to the AvatarPickerViewModel.
  • Thanks to this PR I could refactored how the avatar image is retrieved and loaded (via the GetPublicAssetUseCase) and persisted to memory once a new picture has been chosen/taken (via the UploadUserAvatarUseCase) on the corresponding View Models.
  • Added Coil library to handle image loading on the app.

Attachments (Optional)

Grabacion.de.pantalla.2022-03-09.a.las.12.48.00.mov

PR Post Submission Checklist for internal contributors (Optional)

  • Wire's Github Workflow has automatically linked the PR to a JIRA issue

PR Post Merge Checklist for internal contributors

  • If any soft of configuration variable was introduced by this PR, it has been added to the relevant documents and the CI jobs have been updated.
References
  1. https://sparkbox.com/foundry/semantic_commit_messages
  2. https://github.com/wireapp/.github#usage
  3. E.g. feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764.

@gongracr gongracr mentioned this pull request Mar 9, 2022
Merged
7 tasks
@yamilmedina yamilmedina self-requested a review March 9, 2022 16:00
yamilmedina
yamilmedina previously approved these changes Mar 9, 2022
View reviewed changes
Copy link
Contributor

@yamilmedina yamilmedina left a comment

Choose a reason for hiding this comment

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

Awesome work! 👏 a pain in the a$$ working the FileProvider, but now it's working.

Just a small task before merging, taking care of this detekt issues, just a suppress will do it:
https://github.com/wireapp/wire-android-reloaded/runs/5482156311?check_suite_focus=true

exceptions - 40min debt
TooGenericExceptionCaught - [e] at /home/runner/work/wire-android-reloaded/wire-android-reloaded/app/src/main/kotlin/com/wire/android/ui/userprofile/UserProfileViewModel.kt:95:18
TooGenericExceptionCaught - [e] at /home/runner/work/wire-android-reloaded/wire-android-reloaded/app/src/main/kotlin/com/wire/android/ui/userprofile/image/AvatarPickerViewModel.kt:49:18
style - 10min debt
MagicNumber - [toByteArray] at /home/runner/work/wire-android-reloaded/wire-android-reloaded/app/src/main/kotlin/com/wire/android/util/FileUtil.kt:33:84

trOnk12
trOnk12 reviewed Mar 10, 2022
View reviewed changes
Copy link
Contributor

@trOnk12 trOnk12 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Generally, good job 🥇left some comments and questions

also the picture on the profile screen looks really weird :

image

@gongracr
Addressed more PR comments
d2b844d
@gongracr
Updated reference to kalium
609c73f
@gongracr
Merge branch 'develop' of github.com:wireapp/wire-android-reloaded in…
f04f910 
…to fix/improve_navigation

# Conflicts:
#	app/src/main/kotlin/com/wire/android/navigation/NavigationGraph.kt
#	app/src/main/kotlin/com/wire/android/navigation/NavigationItem.kt
@yamilmedina yamilmedina self-requested a review March 10, 2022 15:49
yamilmedina
yamilmedina previously approved these changes Mar 10, 2022
View reviewed changes
Copy link
Contributor

@yamilmedina yamilmedina left a comment

Choose a reason for hiding this comment

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

Just added a small comment on naming use cases, but other than that looks solid 🪨 🚀

app/src/main/kotlin/com/wire/android/ui/home/HomeViewModel.kt Outdated Show resolved Hide resolved
app/src/main/kotlin/com/wire/android/ui/userprofile/UserProfileViewModel.kt Outdated Show resolved Hide resolved
trOnk12
trOnk12 previously approved these changes Mar 10, 2022
View reviewed changes
Copy link
Contributor

@trOnk12 trOnk12 left a comment

Choose a reason for hiding this comment

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

small comments but looks good to me ! goob job 👍 🥇

@gongracr gongracr dismissed stale reviews from trOnk12 and yamilmedina via 96b3f56 March 10, 2022 16:43
trOnk12
trOnk12 previously approved these changes Mar 10, 2022
View reviewed changes
Copy link
Contributor

@trOnk12 trOnk12 left a comment

Choose a reason for hiding this comment

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

well done 🍰

trOnk12
trOnk12 approved these changes Mar 10, 2022
View reviewed changes
Copy link
Contributor

@trOnk12 trOnk12 left a comment

Choose a reason for hiding this comment

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

even better 💪

@gongracr gongracr merged commit 35efb02 into develop Mar 10, 2022
@gongracr gongracr deleted the fix/improve_navigation branch March 10, 2022 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@saleniuk saleniuk saleniuk left review comments

@trOnk12 trOnk12 trOnk12 approved these changes

@yamilmedina yamilmedina yamilmedina left review comments

@alexandreferris alexandreferris Awaiting requested review from alexandreferris

@MohamadJaara MohamadJaara Awaiting requested review from MohamadJaara

@vitorhugods vitorhugods Awaiting requested review from vitorhugods

@mchenani mchenani Awaiting requested review from mchenani

@ohassine ohassine Awaiting requested review from ohassine

Assignees
No one assigned
Labels
size/XL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@gongracr @yamilmedina @trOnk12 @saleniuk @vitorhugods