-
-
Notifications
You must be signed in to change notification settings - Fork 90
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
Material Refresh #480
Material Refresh #480
Conversation
@t895 Have you tried using target API 35 with this? |
a8cb3e4
to
cb4a1e7
Compare
I just tested and everything works fine |
Hi @t895, Thanks a lot for making this PR! We appreciate and value all the time and efforts you have put towards making this PR. A few things that I feel should be fixed in the PR:
|
Sorry I made this a little opinionated, but let me elaborate
|
cb4a1e7
to
af11488
Compare
HI @t895, Thanks for addressing the feedback!
I had initially used a specific gray background so that it would be easy to recognize the differentiate the bounds of the image from the background in case of a dark image with little details. I personally feel it's better to keep it that way as have been used to seeing it that way for a long time. We could consider the opinion of other users before we are able to decide on this |
6584154
to
7e0991d
Compare
Hi @t895, Thanks for retaining the original background transition in your latest set of commits! Although the UI looks a bit off right now primarily because the action bar color has been set to transparent (unlike the original translucent color), and there's no to little padding to center the image/thumbnail preview correctly, which is more visible as we view images of different aspect ratios. The padding issue could have occurred because of introducing edge to edge, or because the padding that was previously being applied might have been accidentally removed. The latest release correctly centers the image as expected (as a reference for the expected behavior) Adding the original translucent action bar color should fix the first issue pertaining to the action bar. For the second issue, please make sure the padding is applied on the correct view to ensure that zooming in the preview utilizes the entire screen and the newly applied padding doesn't cause a fixed gap to occur at the top of the preview. |
I'll check those out soon Also sorry if I gave the impression that I was done with working over your suggestions. I'm just going to be busy until this weekend and then I'll be able to finish things up. |
Ah, there’s no need to apologize at all. We greatly appreciate and value the time and effort you have put into contributing to GrapheneOS. Thank you so much for your contributions! |
7e0991d
to
cb076c0
Compare
I just fixed up the weird insets for the camera previews and the bottom tab layout I still need to fix the issues with the settings dialog. The flags for ignoring system bars are really messy. I also wanted to get your opinion on this change. I know that you wanted to get a translucent top app bar, but since we're edge-to-edge now, I thought we could get something a little cleaner looking. If you don't like the look, I could just change it to the previous solid color. demo.mp4 |
cb076c0
to
37d7a31
Compare
Hi @t895, The primary reason to go with a translucent action bar was because it looks off with the gray background. The image preview in the video seems to be spanning the entire screen causing the background to remain hidden at the moment. Ideally the gray background would be visible at the default/minimum zoom level making it look a bit off. I feel we'll be better off going with gray background and a translucent bar as before |
1750bcf
to
bb997c6
Compare
Understood, I went back to the solid color from before |
I'm unsure about what you'd like from the indicator color, so I put together a few combinations for you to browse. This shows a pretty standard blue theme and what I would consider to be "worst case scenario" as a monochrome theme. Let me know what you think |
5d66719
to
9dba479
Compare
Everything except for the indicator has been addressed now. I've also updated the images in the opening comment to match as well. Let me know if you spot any other oddities. |
Also, please refrain from making new changes to the PR now. With each iteration, it is primarily introducing new regressions, causing more harm than help. If possible, only keep the changes related to Material Theming of the app in this PR and separate off the extra changes as a different set of PRs. This would make testing and accepting changes a lot easier for us |
ba71f09
to
b66ac8c
Compare
9f5debd
to
afc77df
Compare
Oh as in the version before the bottom insets issue was addressed? Does adding bottom padding to the main container and keep the original changes help resolve the issue without adding new regressions? |
Sorry, in trying to respond to you quickly, I missed something. |
I just committed the fix separately so you could take a look at it in isolation before I squash |
Yes, the solution resolves the final issue with the edge-to-edge implementation. I think the PR is ready to be merged. Thank you for all your time, patience and efforts. Appreciate and value it all! |
Thank you and I appreciate the same from you. I love your and the rest of the team's work so I'm more than happy to contribute. |
You're always welcome, glad to have your kind words, support and valuable contributions too mate! |
@t895 "Copy to clipboard" button in ImageCapturer error dialog no longer works after these changes. Tapping on it simply dismisses the dialog. Interestingly, tapping above that button does activate it. |
Switching between camera modes looks wrong now sometimes because mode name (CAMERA/VIDEO/etc) color changes too late or too early. |
Huge switches for "Include Audio" and "Enable EIS" items in video quick settings menu look wrong. Their large size also makes the "Focus Timeout" setting invisible, it's easy to miss that it even exists. The only hint of that it's there is a faint scrollbar on the right of quick settings popup that quickly disappears. |
Shrink material switch and always show scrollbar In order to shrink the switch and retain the ability to press the negative space between a label and a switch to toggle, I had to pass touch events to the switch from its container.
I couldn't get a camera error to happen but I tried just running the dialog in the main activity and I couldn't get the issue to happen on my devices. Could you explain a bit more about what you did to trigger that? I know that these dialogs can be really finicky with shortEdges so I don't doubt that it happened.
I don't believe this is a problem? I didn't make a change to the timing of the text color animation when switching between tabs. This is an issue with the tab layout that we can't change.
I pushed a commit to try and address this. I shrunk the switch and always show the scrollbar when the list is scrollable now. |
One of the ways to trigger it is by switching modes while clicking a photo (easier to reproduce in night mode) |
Got it, thank you |
The MaterialAlertDialog appears broken when large, the status bar is hidden, and short edges mode used in the parent window. This tells the dialog to use the default mode to prevent weird layout issues.
I went with a more involved fix since this can potentially effect any material alert dialog that appears in the camera preview screen |
The dialog button in case of long texts and scrollbar of settings dialog seem to work as expected now |
It's true that the text color animation issue is present in the release build too, but it's noticeable there only in one mode that is disabled by default. These changes make this issue noticeable in all modes.
Maybe it's better to not use a switch at all? Pixel Camera doesn't use them in its roughly equivalent UI element. |
Given that this is an issue with the component itself, I could just revert this color change. Making an entirely new component is way out of the scope of this PR.
I feel like we could argue similarly about the right component to use for every part of the settings dialog since it has pretty tight size constraints. If we went with what the Pixel Camera has, we would run into the same problem. |
Will be doing a new release with these changes soon. Ideally we can figure out a fix for the text animation issue or a replacement for that part of the UI. |
Personally, I really don't like working with Android Views, but when Compose is introduced into this project with #468, I'd be interested in rewriting the camera UI with Compose. |
We likely want to move all our apps to Compose but not right away because it will interfere with pending work, etc. |
Well if y'all need help with Compose or anything else with your apps, ping me and I'd be happy to lend a hand |
Hi @t895, Thanks a lot for showing interest in contributing to GrapheneOS, appreciate and value it all. We are already working on porting our camera app to Jetpack Compose, will surely let you know if there's any help needed for testing the app or anything related |
I updated a few parts of the app to make things a little more modern
Images
Apologies for the seemingly large PR. It's primarily from reformatting xml layouts which don't change much.