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

user profile picture in the create profile screen and the hamburger menu #358

Merged
merged 11 commits into from
May 31, 2024

Conversation

unglazedstamp
Copy link

@unglazedstamp unglazedstamp commented May 27, 2024

need #346 merged first

@unglazedstamp unglazedstamp self-assigned this May 28, 2024
@unglazedstamp unglazedstamp added this to the Milestone 4 milestone May 28, 2024
@unglazedstamp unglazedstamp linked an issue May 28, 2024 that may be closed by this pull request
@unglazedstamp unglazedstamp force-pushed the feature/userprofile-picture-frontend branch 4 times, most recently from baff9ad to 79ec557 Compare May 29, 2024 20:08
@unglazedstamp unglazedstamp marked this pull request as ready for review May 29, 2024 20:16
@unglazedstamp unglazedstamp force-pushed the feature/userprofile-picture-frontend branch 2 times, most recently from 03dea5d to ad0f19a Compare May 29, 2024 22:18
@ntissieres ntissieres self-requested a review May 30, 2024 07:45
singleLine = true,
isError = lastName.isBlank()
)
Spacer(modifier = modifier.height(5.dp))
Copy link
Contributor

Choose a reason for hiding this comment

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

The height of the spacer can be set into a val at the beginning of the file, while it is use multiple times

}
}
val pictureDisplaySize = 100.dp
Column(modifier = Modifier.padding(start = 5.dp)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, try to avoid magic values in padding

)
},
painter =
if (picture != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice idea to use the echo logo if no profile picture !

painterResource(R.drawable.echologoround)
},
contentDescription = "",
alpha = 0.5f
Copy link
Contributor

Choose a reason for hiding this comment

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

magic value

)
}
IconButton(
modifier = Modifier.align(Alignment.End).offset(x = 10.dp, y = (-10).dp),
Copy link
Contributor

Choose a reason for hiding this comment

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

magic values

* @param picture: the picture to center
* @param onConfirm: callback called when the picture is centered
* @param onCancel: callback when the action is canceled
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

What a determination to implement that haha, nice feature !

Copy link
Author

@unglazedstamp unglazedstamp May 30, 2024

Choose a reason for hiding this comment

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

Yes I really wanted to do something similar to what most social media app do with the profile picture, it's not as good as these apps but it's clearly usable.

style = Stroke(strokeSize)
)
}
Card(modifier = Modifier.align(Alignment.TopCenter).offset(y = 20.dp)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

magic value

}
Card(modifier = Modifier.align(Alignment.TopCenter).offset(y = 20.dp)) {
Text(
modifier = Modifier.padding(5.dp),
Copy link
Contributor

Choose a reason for hiding this comment

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

magic value

}
Row(
modifier =
Modifier.align(Alignment.BottomEnd).padding(horizontal = 0.dp, vertical = 15.dp)
Copy link
Contributor

Choose a reason for hiding this comment

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

magic values

@srsingh04 srsingh04 self-requested a review May 30, 2024 08:19
Copy link
Contributor

@ntissieres ntissieres left a comment

Choose a reason for hiding this comment

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

Truly impressive code ! You realy manage to make the feature of adding profiles pictures very handy for the user, nice work ! Still, there is some minor changes required :

  • A lot of magic values to put into val in order to make the maintenance easier in the case we want to change things
  • The profile picture in the hamburger menu is a bit overlapping the switch dark/light mode. It could be good to replace the "close hamburger menu" button (the cross) which is not necessary anymore with the light/dark mode button.
  • The center picture feature is good, but instead of using the circle to indicate the center of the picture, it would be better to enlarge it to show instead what part of the picture will be selected

Otherwise, I tried to change my profile picture and it works great ;)

Copy link
Contributor

@srsingh04 srsingh04 left a comment

Choose a reason for hiding this comment

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

Nice code, thank you for working on the feature. Some minor changes to be done and then it should be good to go.
Overall good work.

rememberLauncherForActivityResult(ActivityResultContracts.PickVisualMedia()) { uri ->
if (uri == null) {
Log.w("photo picker", "photo not found")
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of a nested if else loop, to break down, after the first if: you could break it down by just putting a return@rememberLauncherForActivityResult when uri or rawPicture are null. This coudl help get rid of the nested if else block, which could be better for the cognitive complexity. What do you think?

Copy link
Author

Choose a reason for hiding this comment

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

I believe that it's better for maintainability to have a clear if else statement, furthermore the else statement allow for smart casting which is good for code readability and safety.

var showPictureDialog by remember { mutableStateOf(false) }
val localContext = LocalContext.current
if (!isPhotoPickerAvailable(localContext)) {
Log.e("CreateProfile", "Photo picker not available")
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe you could add an empty return statement after the log if the photo picker is not available so that it doesn't go through the rest of the code and is more optimized? What do you think?

Copy link
Author

Choose a reason for hiding this comment

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

If the photo picker isn't available, android uses a fallback, so the feature is still relevant, however our application isn't supposed to be used on these old device and some error could arise (e.g. select a file which isn't a picture) so I log an error

if (pictureByteArray != null) {
BitmapFactory.decodeByteArray(pictureByteArray, 0, pictureByteArray.size)
} else {
null
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the else block necessary? The default value is set to null, so you could safely get rid of the else block.

pictureByteArray.size
)
} else {
null
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly, the else block is not required here because the default value is null already.

return offs.x > -picture.width * sca / 2f + circleRadius &&
offs.y > -picture.height * sca / 2f + circleRadius &&
offs.x < picture.width * sca / 2f - circleRadius &&
offs.y < picture.height * sca / 2f - circleRadius &&
Copy link
Contributor

Choose a reason for hiding this comment

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

you could modularize the return statement by defining each set of conditions as variables and then using them in the return statement. This would also allow someone in the future to easily edit the conditions without much hassle.

newScale
} else {
scale
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, I think the else block can be safely deleted because the scale variable just keeps its value when the condition is not respected.

if (isTransformInside(newOffset, scale)) {
newOffset
} else {
offset
Copy link
Contributor

Choose a reason for hiding this comment

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

idem, else block can be safely deleted.

image.assertPositionInRootIsEqualTo(15.dp, 15.dp)
}

// this test needs to be run there because of the Bitmap class which require android
Copy link
Contributor

Choose a reason for hiding this comment

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

Knitpick: the comment is a bit confusing: what exactly do you mean by 'test needs to be run there'? run where? Maybe I am just confused for nothing here...

Copy link
Author

Choose a reason for hiding this comment

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

ok I will remove this comment

@unglazedstamp unglazedstamp enabled auto-merge (rebase) May 30, 2024 17:39
srsingh04
srsingh04 previously approved these changes May 30, 2024
Copy link
Contributor

@srsingh04 srsingh04 left a comment

Choose a reason for hiding this comment

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

Great! Thank you for the nice feature. Ready to be merged from my side.

onConfirm: (picture: Bitmap) -> Unit,
onCancel: () -> Unit
) {
val circleRadius = min(picture.width, picture.height) * 3 / 8
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it's a good idea to set the circle radius according the image size. I tried it with different image, and some mystical things happens if the image is not the same size as the screen. I think it would be preferable to use a fix size

Copy link
Author

Choose a reason for hiding this comment

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

The problem here is that if the picture is smaller than the fixed size then the app crashes

@unglazedstamp unglazedstamp force-pushed the feature/userprofile-picture-frontend branch 2 times, most recently from 8195b5d to b1bba06 Compare May 31, 2024 08:41
@ntissieres ntissieres self-requested a review May 31, 2024 08:41
Copy link
Contributor

@ntissieres ntissieres left a comment

Choose a reason for hiding this comment

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

Thanks for the changes !!

@unglazedstamp unglazedstamp force-pushed the feature/userprofile-picture-frontend branch from b1bba06 to b0a5444 Compare May 31, 2024 08:47
Copy link

sonarcloud bot commented May 31, 2024

@unglazedstamp unglazedstamp merged commit aac986d into main May 31, 2024
2 checks passed
@unglazedstamp unglazedstamp deleted the feature/userprofile-picture-frontend branch May 31, 2024 08:59
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.

Add image to user profile creation/edition screen
3 participants