[2 pane layout] Tablet mode detection logic change #13228

AnirudhBhat · 2024-12-31T10:54:09Z

Description

This PR refactors the tablet detection logic to account for both width and height dimensions when determining if a device qualifies as a tablet. The updated logic improves compatibility with larger phones especially in landscape mode.

Changes Introduced:
New Size Class Determination Method:

Introduced determineWindowSizeClassByDimensions(widthDp: Int, heightDp: Int).
Evaluates both shortSize and longSize (smallest and largest dimensions, respectively) to classify devices as Compact, Medium, or ExpandedAndBigger.

Steps to reproduce

Open WooCommerce app
Navigate to orders screen
Rotate your device
Observe the Behaviour:
On smaller devices: The screen remains in single-pane mode and does not split into tablet mode.
On larger phones and tablets: The screen transitions to split-pane (tablet) mode as expected.

The tests that have been performed

Verified behaviour on below emulators:
Tablets (portrait and landscape)
Large phones in landscape mode

Images/gif

tablet_detection_logic.mp4

I have considered if this change warrants release notes and have added them to RELEASE-NOTES.txt if necessary. Use the "[Internal]" label for non-user-facing changes.

Reviewer (or Author, in the case of optional code reviews):

Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement:

The PR is small and has a clear, single focus, or a valid explanation is provided in the description. If needed, please request to split it into smaller PRs.
Ensure Adequate Unit Test Coverage: The changes are reasonably covered by unit tests or an explanation is provided in the PR description.
Manual Testing: The author listed all the tests they ran, including smoke tests when needed (e.g., for refactorings). The reviewer confirmed that the PR works as expected on big (tablet) and small (phone) in case of UI changes, and no regressions are added.

… and height for split mode.

wpmobilebot · 2024-12-31T10:59:00Z

📲 You can test the changes from this Pull Request in WooCommerce-Wear Android by scanning the QR code below to install the corresponding build.

	App Name	WooCommerce-Wear Android
	Platform	⌚️ Wear OS
	Flavor	Jalapeno
	Build Type	Debug
	Commit	`7e2c9bc`
	Direct Download	`woocommerce-wear-prototype-build-pr13228-7e2c9bc.apk`

wpmobilebot · 2024-12-31T11:13:06Z

📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.

	App Name	WooCommerce Android
	Platform	📱 Mobile
	Flavor	Jalapeno
	Build Type	Debug
	Commit	`7e2c9bc`
	Direct Download	`woocommerce-prototype-build-pr13228-7e2c9bc.apk`

codecov-commenter · 2024-12-31T11:21:59Z

Codecov Report

Attention: Patch coverage is 56.33803% with 31 lines in your changes missing coverage. Please review.

Project coverage is 41.22%. Comparing base (ca38ed6) to head (7e2c9bc).
Report is 20 commits behind head on trunk.

Files with missing lines	Patch %	Lines
...n/com/woocommerce/android/extensions/ContextExt.kt	0.00%	9 Missing ⚠️
...oid/ui/orders/creation/OrderCreateEditViewModel.kt	80.00%	6 Missing and 2 partials ⚠️
...oocommerce/android/util/TabletLayoutSetupHelper.kt	0.00%	8 Missing ⚠️
...ers/creation/totals/OrderCreateEditTotalsHelper.kt	0.00%	0 Missing and 2 partials ⚠️
...merce/android/ui/orders/list/OrderListViewModel.kt	0.00%	2 Missing ⚠️
.../com/woocommerce/android/analytics/AnalyticsExt.kt	75.00%	1 Missing ⚠️
...n/kotlin/com/woocommerce/android/util/UiHelpers.kt	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##              trunk   #13228      +/-   ##
============================================
- Coverage     41.23%   41.22%   -0.01%     
+ Complexity     6548     6547       -1     
============================================
  Files          1326     1326              
  Lines         77731    77730       -1     
  Branches      10706    10701       -5     
============================================
- Hits          32049    32046       -3     
- Misses        42836    42839       +3     
+ Partials       2846     2845       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

kidinov · 2025-01-06T09:54:39Z

RELEASE-NOTES.txt

@@ -7,6 +7,7 @@
 - [*] Fixed overlap issue in Settings > WooCommerce Version  [https://github.com/woocommerce/woocommerce-android/pull/13183]
 - [*] Fixed a crash on the order details [https://github.com/woocommerce/woocommerce-android/pull/13191]
 - [**] Fixed a crash when a shop manager was trying to install or activate plugin in the POS onboarding [https://github.com/woocommerce/woocommerce-android/pull/13203]
+- [*] Refactored tablet detection logic to consider both width and height dimensions, enhancing compatibility with larger phones and tablets, particularly in landscape mode [https://github.com/woocommerce/woocommerce-android/pull/13228]


np: I think this is not a refactoring, but change of the logic

Updated here: dd61dda

kidinov · 2025-01-06T10:33:51Z

WooCommerce/src/main/kotlin/com/woocommerce/android/extensions/ContextExt.kt


 val Context.windowSizeClass: WindowSizeClass
-    get() = determineWindowWidthSizeClassByGivenSize(resources.configuration.screenWidthDp)
+    get() = determineWindowSizeClassByDimensions(


@AnirudhBhat I think we can not use the same semantics here if we change the logic. Window classes we use here are from https://m3.material.io/foundations/layout/applying-layout/window-size-classes and those are determined based on the width of a device. Reusing the same concept but with different logic may bring confusion and errors

I am thinking that:

We should either simplify all of this and end up with something "isTwoPaneShouldBeUsed" with logic similar/the same as WooPosIsScreenSizeAllowed. I'd say this is preferred, as I think we learned that the use of those size classes doesn't work well at all

Or at least refactor all of this and call it in not attaching to the material design size classes name (remove comments etc) to make sure that we don't mix the concepts

wdyt?

Sorry for the late response.

I agree with your opinion that mixing Window classes and our custom detection logic would create confusion. I've updated the logic to remove Window class reference. Please take a look.

f43bda8

wpmobilebot · 2025-01-10T11:35:35Z

Version 21.4 has now entered code-freeze, so the milestone of this PR has been updated to 21.5.

wpmobilebot · 2025-01-24T20:51:54Z

Version 21.5 has now entered code-freeze, so the milestone of this PR has been updated to 21.6.

dangermattic · 2025-01-28T05:37:53Z

	2 Warnings
⚠️	This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.
⚠️	This PR is assigned to the milestone 21.6. This milestone is due in less than 2 days. Please make sure to get it merged by then or assign it to a milestone with a later deadline.

Generated by 🚫 Danger

…-logic-change # Conflicts: # WooCommerce/src/main/kotlin/com/woocommerce/android/ui/orders/details/OrderDetailFragment.kt # WooCommerce/src/main/kotlin/com/woocommerce/android/ui/orders/list/OrderListFragment.kt # WooCommerce/src/main/kotlin/com/woocommerce/android/ui/orders/list/OrderListViewModel.kt

kidinov · 2025-01-29T08:49:52Z

WooCommerce/src/main/kotlin/com/woocommerce/android/analytics/AnalyticsExt.kt


 val Context.deviceTypeToAnalyticsString: String
    get() = buildAnalyticsDeviceTypeValue(
-        IsScreenLargerThanCompactValue(value = windowSizeClass != WindowSizeClass.Compact)
+        IsScreenLargerThanCompactValue(value = isTwoPanesShouldBeUsed)


IsScreenLargerThanCompactValue should probably be renamed? I see that we track:

const val VALUE_DEVICE_TYPE_REGULAR = "regular" const val VALUE_DEVICE_TYPE_COMPACT = "compact"

But is this the same as 2 panes and 1 pane?

Logically it seems that what we want to track -> either 2 panes or 1 pane used, but naming is old so maybe confusing. Wdyt regarding this?

I've renamed variables used for analytics here: 91c4324

I've commented for more clarity on why we had kept the values for analytics as it is.

kidinov · 2025-01-29T09:13:19Z

...merce/src/main/kotlin/com/woocommerce/android/ui/orders/creation/OrderCreateEditViewModel.kt

@@ -568,8 +567,8 @@ class OrderCreateEditViewModel @Inject constructor(
        handleCouponEditResult(couponEditResult)
    }

-    private fun getScreenSizeClassNameForAnalytics(windowSize: WindowSizeClass) =
-        IsScreenLargerThanCompactValue(windowSize != WindowSizeClass.Compact).deviceTypeToAnalyticsString
+    private fun getScreenSizeClassNameForAnalytics(isTwoPane: Boolean) =


Renaming is needed

Renamed: 91c4324

kidinov · 2025-01-29T09:19:06Z

...merce/src/main/kotlin/com/woocommerce/android/ui/orders/creation/OrderCreateEditViewModel.kt

@@ -580,7 +579,7 @@ class OrderCreateEditViewModel @Inject constructor(
                    KEY_STATUS to order.status,
                    KEY_TYPE to CUSTOMER,
                    KEY_FLOW to flow,
-                    KEY_HORIZONTAL_SIZE_CLASS to getScreenSizeClassNameForAnalytics(viewState.windowSizeClass)
+                    KEY_HORIZONTAL_SIZE_CLASS to getScreenSizeClassNameForAnalytics(viewState.isTwoPaneLayout)


Hmm... I am thinking about how we could keep the naming that we send to Tracks (for backward compatibility) but at the same time make it clear what we track in the code 🤔 I think maybe just renaming the variables to something that make it clear that we track 1/2 panes, but the values keep the same?

I've commented for more clarity on why we had kept the values for analytics as it is -91c4324

kidinov · 2025-01-29T09:27:12Z

WooCommerce/src/main/kotlin/com/woocommerce/android/util/UiHelpers.kt

-
-class IsWindowClassExpandedAndBigger @Inject constructor(val context: Context) {
-    operator fun invoke() = context.windowSizeClass >= WindowSizeClass.ExpandedAndBigger
+    operator fun invoke() = context.isTwoPanesShouldBeUsed


Naming here could be changed too

Renamed: 91c4324

kidinov

LGTM! I'd just recommend to improve the naming of the variables, to make it clear what are we doing, especially in tracking

…-logic-change

AnirudhBhat added 2 commits December 31, 2024 13:08

Change the window size detection logic so that we consider both width…

b45041b

… and height for split mode.

Remove needless blank line

62428af

AnirudhBhat added type: bug A confirmed bug. category: tablet Specific to tablet devices such as a Galaxy Tab or an iPad. labels Dec 31, 2024

AnirudhBhat added this to the 21.4 milestone Dec 31, 2024

Add release notes

e0b9d62

AnirudhBhat requested a review from samiuelson January 2, 2025 04:10

AnirudhBhat marked this pull request as ready for review January 2, 2025 04:10

AnirudhBhat changed the title ~~[Woo POS][Non-Simple Products] Tablet mode detection logic change~~ [2 pane layout] Tablet mode detection logic change Jan 3, 2025

kidinov requested review from kidinov and removed request for samiuelson January 6, 2025 09:40

kidinov reviewed Jan 6, 2025

View reviewed changes

Base automatically changed from issue/13221-remove-variation-quantity to trunk January 10, 2025 04:14

wpmobilebot modified the milestones: 21.4, 21.5 Jan 10, 2025

kidinov self-assigned this Jan 13, 2025

wpmobilebot modified the milestones: 21.5, 21.6 Jan 24, 2025

AnirudhBhat added 2 commits January 28, 2025 10:57

Remove mixing of window size logic and our custom two pane layout logic

f43bda8

Fix detekt errors

f4e100c

AnirudhBhat added 3 commits January 28, 2025 11:15

Remove unused imports and add necessary import

3f8497f

Update release notes

dd61dda

AnirudhBhat requested a review from kidinov January 28, 2025 09:59

kidinov reviewed Jan 29, 2025

View reviewed changes

kidinov approved these changes Jan 29, 2025

View reviewed changes

AnirudhBhat added 2 commits January 30, 2025 13:03

Rename variable names used in analytics for better understanding.

91c4324

Merge branch 'refs/heads/trunk' into issue/12861-windowsize-detection…

7e2c9bc

…-logic-change

AnirudhBhat merged commit 32dd18c into trunk Jan 30, 2025
15 checks passed

AnirudhBhat deleted the issue/12861-windowsize-detection-logic-change branch January 30, 2025 11:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[2 pane layout] Tablet mode detection logic change #13228

[2 pane layout] Tablet mode detection logic change #13228

AnirudhBhat commented Dec 31, 2024 •

edited by kidinov

Loading

wpmobilebot commented Dec 31, 2024 •

edited

Loading

wpmobilebot commented Dec 31, 2024 •

edited

Loading

codecov-commenter commented Dec 31, 2024 •

edited

Loading

kidinov Jan 6, 2025

AnirudhBhat Jan 28, 2025

kidinov Jan 6, 2025

AnirudhBhat Jan 28, 2025

wpmobilebot commented Jan 10, 2025

wpmobilebot commented Jan 24, 2025

dangermattic commented Jan 28, 2025 •

edited

Loading

kidinov Jan 29, 2025

kidinov Jan 29, 2025

AnirudhBhat Jan 30, 2025

kidinov Jan 29, 2025

AnirudhBhat Jan 30, 2025

kidinov Jan 29, 2025 •

edited

Loading

AnirudhBhat Jan 30, 2025 •

edited

Loading

kidinov Jan 29, 2025

AnirudhBhat Jan 30, 2025

kidinov left a comment

[2 pane layout] Tablet mode detection logic change #13228

[2 pane layout] Tablet mode detection logic change #13228

Conversation

AnirudhBhat commented Dec 31, 2024 • edited by kidinov Loading

Description

Steps to reproduce

The tests that have been performed

Images/gif

Reviewer (or Author, in the case of optional code reviews):

wpmobilebot commented Dec 31, 2024 • edited Loading

wpmobilebot commented Dec 31, 2024 • edited Loading

codecov-commenter commented Dec 31, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wpmobilebot commented Jan 10, 2025

wpmobilebot commented Jan 24, 2025

dangermattic commented Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kidinov Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

AnirudhBhat Jan 30, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kidinov left a comment

Choose a reason for hiding this comment

AnirudhBhat commented Dec 31, 2024 •

edited by kidinov

Loading

wpmobilebot commented Dec 31, 2024 •

edited

Loading

wpmobilebot commented Dec 31, 2024 •

edited

Loading

codecov-commenter commented Dec 31, 2024 •

edited

Loading

dangermattic commented Jan 28, 2025 •

edited

Loading

kidinov Jan 29, 2025 •

edited

Loading

AnirudhBhat Jan 30, 2025 •

edited

Loading