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

Crop image to square #952

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open

Conversation

warahiko
Copy link

@warahiko warahiko commented Sep 3, 2024

Issue

#413

Overview (Required)

  • Add CropImageScreen to crop image to square
    • There is no design corresponding to this screen, right?
  • If selected image is not square, transition to CropImageScreen
    • otherwise, apply selected image
  • I tried to implement in iOS, but due to some problems I couldn't complete. So temporarily split process
    • in iOS with compose multiplatform, when backing from CropImageScreen, the state of bottom navigation is cleared (app goes to TimeTable)
      • it happens other case, i.e., Contributers, Staff, ....
      • we can continue editing during first time, but not once we create profile
    • Simply, it takes time to implement features because I am not familiar with SwiftUI
  • Multi language support is left due to large diff

Links

Screenshot (Optional if screenshot test is present or unrelated to UI)

see videos

Before After

Movie (Optional)

Before After
android_before.mp4
android.mp4

for various images

various_images.mp4

in landscape screen

landscape.mp4

cropping in iOS (currently unavailable due to the above issue)

iOS_crop.mp4

Comment on lines +18 to +29
override fun equals(other: Any?): Boolean {
if (this === other) return true
if (other == null || this::class != other::class) return false

other as ProfileImage

return bytes.contentEquals(other.bytes)
}

override fun hashCode(): Int {
return bytes.contentHashCode()
}
Copy link
Author

Choose a reason for hiding this comment

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

It is necessary to resolve the warning.


@Composable
internal fun cropImageScreenPresenter(
onConfirm: () -> Unit,
Copy link
Author

Choose a reason for hiding this comment

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

It seems there are no arguments like this in other screens, but I added it because I wanted to transition after event processing.
(Is there another way to achieve this?)

Copy link
Member

Choose a reason for hiding this comment

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

Thank you for asking this. I recommend using this pattern.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks! refactored at e9e04d2.

Comment on lines +41 to +43
withContext(Dispatchers.Default) {
croppedProfileImage = requireNotNull(profileImageCandidate).crop(event.rect)
}
Copy link
Author

@warahiko warahiko Sep 3, 2024

Choose a reason for hiding this comment

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

Because it can take time to process

This dispatcher might need to be injected...

val cardUiState: ProfileCardUiState.Card? by rememberUpdatedState(profileCard.toCardUiState())
var cardError by remember { mutableStateOf(ProfileCardError()) }
var uiType: ProfileCardUiType by remember { mutableStateOf(ProfileCardUiType.Loading) }
var uiType: ProfileCardUiType by rememberSaveable { mutableStateOf(ProfileCardUiType.Loading) }
Copy link
Author

Choose a reason for hiding this comment

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

To maintain the editing state when backing from CropImageScreen

Comment on lines +34 to +35
val strokeOuterColor = MaterialTheme.colorScheme.onSurface
val strokeInnerColor = MaterialTheme.colorScheme.surface
Copy link
Author

Choose a reason for hiding this comment

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

To ensure the border is not difficult to see if there are images with similar colors to surface/onSurface

@github-actions github-actions bot temporarily deployed to deploygate-distribution September 4, 2024 11:43 Inactive
@takahirom
Copy link
Member

@warahiko
The movie looks great! This could take a lot of time! Thank you for your effort. Could you take a look at the format using ./gradlew detekt --auto-correct and the tests?

@warahiko
Copy link
Author

warahiko commented Sep 5, 2024

@takahirom
Thanks for reviewing!
I checked that the following tasks have passed. Please check.
(Or please tell me other tasks that they should pass.)

  • ./gradlew detekt --auto-correct
  • ./gradlew testDevDebugUnitTest testDebugUnitTest --stacktrace
  • ./gradlew compareRoborazziDebug compareRoborazziDevDebug --stacktrace


@OptIn(ExperimentalMaterial3Api::class)
@Composable
internal fun CropImageScreen(
Copy link
Member

@takahirom takahirom Sep 6, 2024

Choose a reason for hiding this comment

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

It is vital to test this method as we don't want to rely on manual testing. Could you try creating it? I think it is sufficient to check the launch for the first time.

Copy link
Author

Choose a reason for hiding this comment

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

Added at e22abae.

Copy link

github-actions bot commented Sep 6, 2024

@takahirom
Copy link
Member

I tried to implement in iOS, but due to some problems I couldn't complete. So temporarily split process
in iOS with compose multiplatform, when backing from CropImageScreen, the state of bottom navigation is cleared (app goes to TimeTable)
it happens other case, i.e., Contributers, Staff, ....
we can continue editing during first time, but not once we create profile

Thank you for sharing this. I also tried to solve the problem, but we haven't been able to resolve it so far. Although I mentioned creating a test, I think we might not have enough time to ship this by the DroidKaigi. 😭 We might have to solve the problem first
#882

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants