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

Always recompute screen size related calculations #654

Merged
merged 5 commits into from
May 13, 2024

Conversation

prudrabhat
Copy link
Contributor

@prudrabhat prudrabhat commented May 8, 2024

Description

Problem: Currently, screen size related calculations are done once and remembered on recompositions. This works well generally when adapting to the new size when orientation of the device is changed.
However, when the activity hosting the composable restricts configuration change (via android:configChanges="orientation|..." flags in the activity manifest), the remembered value is not recomputed preventing the presentables from adapting to the new screen size.

Solution: Do not remember the size related calculations. Always calculate them on composition.

  • Rename current orientation test to reflect that they test configuration
  • Add UiAutomator as an android test dependency and simulate rotation using and test using config restricted activity.

Related Issue

Internal (MOB-20923)

How Has This Been Tested?

  • Manual tests
  • Existing instrumentation tests
  • New instrumentation tests for orientation changes

Screenshots (if appropriate):

MessageScreenOrientationTests.webm

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have signed the Adobe Open Source CLA.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Currently, screen shize related calculations are done once
and remembered on recompositions. This works well when adapting
to the new size when orientation of the device is changed.

However, when the activity hosting the composable restricts configuration
changes, the remembered value is not recomputed preventing the presentables
from adapting to the new screen size.

As a solution, do not remember the size related calculation. Always calculate
them on composition.
Copy link

codecov bot commented May 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.52%. Comparing base (774df76) to head (91292ea).

Additional details and impacted files
@@             Coverage Diff              @@
##                dev     #654      +/-   ##
============================================
- Coverage     81.52%   81.52%   -0.00%     
- Complexity     2129     2130       +1     
============================================
  Files           191      191              
  Lines          8909     8903       -6     
  Branches       1114     1114              
============================================
- Hits           7263     7258       -5     
+ Misses         1090     1089       -1     
  Partials        556      556              
Flag Coverage Δ
functional-tests 27.68% <100.00%> (-0.04%) ⬇️
unit-tests 68.89% <0.00%> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
...services/ui/floatingbutton/views/FloatingButton.kt 75.81% <100.00%> (ø)
...mobile/services/ui/message/views/MessageContent.kt 92.59% <100.00%> (-0.51%) ⬇️
...g/mobile/services/ui/message/views/MessageFrame.kt 94.20% <100.00%> (-0.32%) ⬇️

... and 1 file with indirect coverage changes

@@ -58,8 +58,8 @@ internal fun FloatingButton(
onDragFinished: (Offset) -> Unit
) {
// Floating button draggable area dimensions
val heightDp = with(LocalConfiguration.current) { remember { mutableStateOf(screenHeightDp.dp) } }
val widthDp = with(LocalConfiguration.current) { remember { mutableStateOf(screenWidthDp.dp) } }
Copy link
Contributor

@spoorthipujariadobe spoorthipujariadobe May 9, 2024

Choose a reason for hiding this comment

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

Do we also need to recompute the offset for the floating button when the orientation is changed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Offset is provided from the FloatingButtonScreen which is not remembered so that should be sufficient.

@spoorthipujariadobe
Copy link
Contributor

There's an instrumentation test for orientation change. Is it possible to modify it to verify the width and height are recalculated?

@prudrabhat
Copy link
Contributor Author

prudrabhat commented May 10, 2024

There's an instrumentation test for orientation change. Is it possible to modify it to verify the width and height are recalculated?

The current instrumentation test for rotation is misleading as it not really testing rotation. It just simulates reconstruction. I am working on modifying it to actually test rotation.

@spoorthipujariadobe
Copy link
Contributor

spoorthipujariadobe commented May 10, 2024

There's an instrumentation test for orientation change. Is it possible to modify it to verify the width and height are recalculated?

I looked at the current instrumentation test for rotation is misleading as it not really testing rotation. It just simulates reconstruction. I am working on modifying it to actually test rotation.

I couldn't find a reliable way to simulate device rotation using compose test rule :/ Closest I found was emulateSavedInstanceStateRestore which disposes the current composable and composes again the content passed to setContent. If we find no other way, using this would clear the state stored via remember(), maybe we can test that way?

@prudrabhat prudrabhat merged commit ab819d9 into adobe:dev May 13, 2024
8 checks passed
@prudrabhat prudrabhat deleted the iam_resize branch June 17, 2024 21:53
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.

None yet

3 participants