Skip to content

Fixed paddings on horizontal mode #1353

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

Merged

Conversation

ArnyminerZ
Copy link
Member

@ArnyminerZ ArnyminerZ commented Mar 20, 2025

Purpose

Horizontal mode's paddings are not correct, causing some parts of the UI to be overlaid by the system UI. See #1346 for examples.

Short description

  • Fixed padding for the side menu on landscape mode.
  • Fixed padding for FAB on landscape mode and 3-button navigation enabled.
  • Fixed text overlap with display cutout in horizontal mode.
Landscape mode accounts screen

davx5-fix-horizontal

Sidebar fix

davx5-fix-sidebar

Further information

https://developer.android.com/codelabs/edge-to-edge#4

Checklist

  • The PR has a proper title, description and label.
  • I have self-reviewed the PR.
  • I have added documentation to complex functions and functions that can be used by other modules.
  • I have added reasonable tests or consciously decided to not add tests.

@ArnyminerZ ArnyminerZ linked an issue Mar 20, 2025 that may be closed by this pull request
2 tasks
@ArnyminerZ
Copy link
Member Author

I really think that those are simply Android bugs, but we should fix them, at least until Google does something.

Edge-to-edge is really buggy and strange, and for some reason they are enforcing it

@ArnyminerZ ArnyminerZ requested a review from sunkup March 20, 2025 08:44
@ArnyminerZ ArnyminerZ marked this pull request as ready for review March 20, 2025 08:44
@sunkup
Copy link
Member

sunkup commented Mar 24, 2025

I really think that those are simply Android bugs, but we should fix them, at least until Google does something.

What makes you think that? In the docs they say we got to handle it manually ourselves. https://developer.android.com/develop/ui/compose/system/cutouts#handle-cutout

So I don't think google considers it an android bug ...

Copy link
Member

@sunkup sunkup left a comment

Choose a reason for hiding this comment

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

Works fine for accounts screen. But there are still some views that are missing, like the Webdav mounts (FAB). There may be more. Probably also a good idea to check the buy badges view in the gplay variant.

image

@sunkup
Copy link
Member

sunkup commented Mar 24, 2025

Actually I think that, when in landscape mode, we should not let the content area extend into the cutout areas at all.

image

@ArnyminerZ ArnyminerZ self-assigned this Mar 26, 2025
@ArnyminerZ ArnyminerZ added the bug Something isn't working label Mar 26, 2025
@mbiebl
Copy link
Contributor

mbiebl commented Mar 26, 2025

Actually I think that, when in landscape mode, we should not let the content area extend into the cutout areas at all.

I guess it get's even more complicated on Tablets and split-window mode.
Is there maybe a better way to handle this than simply basing this on whether we are in landscape mode or not?

@ArnyminerZ
Copy link
Member Author

I've simply added a bounding box to all views that makes sure nothing overlays anything:

Accounts screen

accounts

Side panel

sidepanel

Webdav mounts

mounts

Vertical works correctly as well. Only downside is that content cannot be under the navigation bar, but I guess it's just better to keep it this way.

@ArnyminerZ ArnyminerZ requested a review from sunkup March 26, 2025 17:46
Copy link
Member

@sunkup sunkup left a comment

Choose a reason for hiding this comment

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

When in portrait orientation in the accounts view:

  1. turn the device to landscape orientation
  2. open the account

Now the space left and right is missing. FABs are under the navigation again. This is probably true for other views as well.

image

@ArnyminerZ
Copy link
Member Author

ArnyminerZ commented Mar 31, 2025

Now the space left and right is missing. FABs are under the navigation again. This is probably true for other views as well.

Did you re-build it? I'm not having that issue 🤔

Edit: okay, I guess you used API 34, it's as you've said there.

Intro

api34-intro

Accounts

api34-accounts

@ArnyminerZ ArnyminerZ requested a review from sunkup April 4, 2025 08:31
@ArnyminerZ
Copy link
Member Author

I've now manually configured each activity that required it. I've checked all of them and seem to be fine.

I've also added that when opening the side menu on light mode, the title changes the text color to a lighter one, because otherwise it wasn't visible enough imo.

@mbiebl
Copy link
Contributor

mbiebl commented Apr 4, 2025

Holy shit, that's a lot of code trying to get this working. Unfortunately, I still find odd issues.

For example if you you turn your phone 90° clockwise (i.e. navigation bar is on the left side)

image

image

sunkup
sunkup previously requested changes Apr 7, 2025
Copy link
Member

@sunkup sunkup left a comment

Choose a reason for hiding this comment

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

I really hope we don't need to add that much code. Can't we turn off edge-to-edge in landscape mode and leave some unused extra whitespace left and right?

Maybe we can schedule a call with the three of us where Arnau explains the situation to us?

@rfc2822 @ArnyminerZ

@ArnyminerZ
Copy link
Member Author

As discussed in the meeting, we'll use safeDrawingPadding with safeDrawingPadding in the AppTheme. Removing all custom paddings.

We'll think about re-implementing edge-to-edge in the future, maybe after #1211, and after completely migrating to Compose Navigation.

@rfc2822 rfc2822 marked this pull request as draft April 8, 2025 13:18
@ArnyminerZ ArnyminerZ requested a review from sunkup April 14, 2025 06:21
@ArnyminerZ ArnyminerZ marked this pull request as ready for review April 14, 2025 06:21
@ArnyminerZ
Copy link
Member Author

ArnyminerZ commented Apr 14, 2025

check whether to add ime padding for all activities not just add webdav add mount

Turns out ime padding was not even necessary, it's already consumed on the theme

@ArnyminerZ ArnyminerZ removed the request for review from sunkup April 14, 2025 09:42
@ArnyminerZ ArnyminerZ marked this pull request as draft April 14, 2025 09:42
Signed-off-by: Arnau Mora <[email protected]>
Signed-off-by: Arnau Mora <[email protected]>
Signed-off-by: Arnau Mora <[email protected]>
@rfc2822 rfc2822 force-pushed the 1346-ui-elements-partially-obscured-in-landscape-mode branch from 2eaa83c to e0a490d Compare April 15, 2025 07:09
@rfc2822
Copy link
Member

rfc2822 commented Apr 15, 2025

FAB ist still under phone navigation in landscape orientation and now the app navigation drawer has some extra white space on the wrong side? Right side in the screenshot. What a hard issue 🦖

Can confirm, probably because of the windowInsets = WindowInsets(0.dp) in AccountsScreen.

@sunkup sunkup removed request for rfc2822 and sunkup April 15, 2025 09:48
@sunkup sunkup marked this pull request as draft April 15, 2025 09:48
Signed-off-by: Arnau Mora <[email protected]>
@ArnyminerZ
Copy link
Member Author

Okay, it should be good now. I've tried every orientation, in both phones and tablets, on API 34 and 35. The only limitation is that API 34 doesn't detect camera cutouts for some reason. But I think we can't do much about it.

@ArnyminerZ ArnyminerZ marked this pull request as ready for review April 16, 2025 15:29
@ArnyminerZ ArnyminerZ requested a review from sunkup April 16, 2025 15:29
@sunkup sunkup requested review from rfc2822 and removed request for sunkup April 17, 2025 06:26
Copy link
Member

@rfc2822 rfc2822 left a comment

Choose a reason for hiding this comment

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

Doesn't look correct in landscape with 3-button navigation when the navigation drawer is shown:

2

When nav drawer is not shown, it's OK:

1

@rfc2822
Copy link
Member

rfc2822 commented Apr 17, 2025

I'd say we remove the edge-to-edge from the Accounts drawer as well? So that it's only active for the IntroScreen

@rfc2822
Copy link
Member

rfc2822 commented Apr 17, 2025

I have removed edge-to-edge from AccountsScreen and navigation drawer also. Would you like to have a quick check? Otherwise I'd merge it and we can "happily" continue to play around with it later 😄

@ArnyminerZ
Copy link
Member Author

Looks good

@mbiebl
Copy link
Contributor

mbiebl commented Apr 17, 2025

still looks broken here

@mbiebl
Copy link
Contributor

mbiebl commented Apr 17, 2025

image

Notice how the drawer is behind the navigation bar

@rfc2822 rfc2822 merged commit 400318b into main-ose Apr 17, 2025
8 checks passed
@rfc2822 rfc2822 deleted the 1346-ui-elements-partially-obscured-in-landscape-mode branch April 17, 2025 10:48
@mbiebl
Copy link
Contributor

mbiebl commented Apr 17, 2025

image

@mbiebl
Copy link
Contributor

mbiebl commented Apr 17, 2025

On Android 12 device
image

Notice how the drawer is visible behind the navigation bar and on the the right side the status bar is not padded and extends all to the right.

@rfc2822
Copy link
Member

rfc2822 commented Apr 17, 2025

Notice how the drawer is visible behind the navigation bar and on the the right side the status bar is not padded and extends all to the right.

Could you provide a PR?

Otherwide I'd leave it for now. We don't have resources for that and I don't get it despite investing hours and hours

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UI elements partially obscured in landscape mode
4 participants