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

fix: borders on fullscreen Reviewer #9278

Merged
merged 4 commits into from Jul 16, 2021

Conversation

david-allison
Copy link
Member

When going fullscreen, the reviewer had white/blue borders rather than
filling the content

Cause:

d0d1edd moved to a single xml file for
the navigation drawer, rather than copy/pasting it into each file

Some layouts excluded android:fitsSystemWindows="true", but the new
xml file always included this.

This caused the layout problems

Fixes

Fixes #9267

Approach

  • add method: fitsSystemWindows and layout without the variable

How Has This Been Tested?

My Android 11

Learning (optional, can help others)

  • I wasted a lot of time learning about PhoneWindow on Android Code Search

Checklist

  • You have not changed whitespace unnecessarily (it makes diffs hard to read)
  • You have a descriptive commit message with a short title (first line, max 50 chars).
  • Your code follows the style of the project (e.g. never omit braces in if statements)
  • You have commented your code, particularly in hard-to-understand areas
  • You have performed a self-review of your own code
  • UI changes: include screenshots of all affected screens (in particular showing any new or changed strings)
  • UI Changes: You have tested your change using the Google Accessibility Scanner

Copy link
Member

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

One wish for a strongly typed set of layout ints, but really that's a wish as it's clearly not a regression, just that you might know them off-hand at this point and know how to make the intdef or enum really quickly at this point.

If not, just hit the merge button - works for me

@mikehardy mikehardy added Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) Needs Author Reply Waiting for a reply from the original author and removed Needs Review labels Jul 16, 2021
@mikehardy mikehardy added this to the 2.15.7 release milestone Jul 16, 2021
@david-allison
Copy link
Member Author

Fair comment - I was considering it anyway, held off as the only other usage was directly above

@david-allison david-allison removed the Pending Merge Things with approval that are waiting future merge (e.g. targets a future release, CI wait, etc) label Jul 16, 2021
@david-allison
Copy link
Member Author

More work than I thought, but it caught a bug (not in this code), so all worth it

@mikehardy
Copy link
Member

Will this fix #9279 as well? it might...

@david-allison david-allison added Needs Review and removed Needs Author Reply Waiting for a reply from the original author labels Jul 16, 2021
@david-allison
Copy link
Member Author

No it doesn't, there's something else going on there.

Force pushed & updated code

@mikehardy
Copy link
Member

unit test fail is legit - looks like a symbol mixup during re-shape-basing

@mikehardy mikehardy added Needs Author Reply Waiting for a reply from the original author and removed Needs Review labels Jul 16, 2021
Refactoring requested in a review for clarity
When going fullscreen, the reviewer had white/blue borders rather than
filling the content

Cause:

d0d1edd moved to a single xml file for
the navigation drawer, rather than copy/pasting it into each file

Some layouts excluded `android:fitsSystemWindows="true"`, but the new
xml file always included this.

This caused the layout problems

fix: add method: fitsSystemWindows and layout without the variable

Fixes 9267
@david-allison david-allison added Needs Review and removed Needs Author Reply Waiting for a reply from the original author labels Jul 16, 2021
Copy link
Member

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

Wow, our full screen mode proliferation is kind of crazy.
This is now a real advance in the state of things I think, the reshape is a significant positive step to me
...and you got a bug :-)

@mikehardy mikehardy merged commit 1bcd4c1 into ankidroid:master Jul 16, 2021
@david-allison david-allison deleted the 9267-full-screen branch July 16, 2021 17:42
@david-allison david-allison mentioned this pull request Jul 16, 2021
7 tasks
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.

fullscreen mode with hide the system bars (after v2.15.5)
2 participants