android-app
android-app copied to clipboard
Material theming
Scope
- Rebase styles onto
Theme.MaterialComponents
family - Options to follow system dark theme
- Borrow color scheme and shape scheme from fortnightly
Out of scope
- Change components to the modern material components
- Better visual design such as study from fortnightly
- Article layout (implemented in WebView)
Questions
- ~Do
Solarized
andFor e-ink devices
needs dark themes?~ Yes - ~Do
Solarized
andFor e-ink devices
needs a "Follow system" option?~ Let them be by default
Ready for a review
Thanks for your contribution! I'm not very good with UI, so the review may take some time.
Can you explain why you use this.
in MainActivity.java
everywhere?
Can you explain why you use
this.
inMainActivity.java
everywhere?
Sorry I forgot to turn off the hook which was set to hardening my programs. Apparently it pollutes the changelog. Reverted.
@di72nn Any updates?
Will it be more attractive if I post some screenshots?
Sorry for the delay and thanks again for your work. I have a few issues with this PR.
I generally don't mind cleaning the code up, but only if it doesn't mess with the commit history. That is, cosmetic/cleanup changes go into a separate commit. That commit generally shouldn't be in the middle of a PR. Preferably it should go into a separate PR, but that's not a requirement.
Activity
being replaced with AppCompatActivity
. I have no problem with using a more specific class when it is needed, but I don't see any reason to replace it everywhere.
Braceless one-line ifs. Braceless multiline ifs are bad, but one-line ifs are a matter of taste. I don't mind them in the current code base. I also don't mind expanding them, but only in the code you touch directly (you edit lines around an if like that - you may rewrite it; the if is in the different method of the same class - don't touch it).
These classes have only AppCompatActivity
replacements:
-
ContextMenuItemHandler
, -
LoggingUtils
, -
WallabagFileProvider
.
These have only AppCompatActivity
and ifs replacements:
-
ListAdapter
, -
ConfigurationTestHelper
, -
SettingsActivity
, -
ArticleActionsHelper
, -
ArticleListFragment
, -
ArticleListsFragment
.
And these have only AppCompatActivity
, ifs and missing @Override
s:
-
TtsFragment
, -
ConnectionWizardActivity
.
So all of the above changes may be considered in a separate PR, but I already gave my opinion on these. Most of these changes are in a separate commit, but the commit also contains some "misc fixes" and it is in the middle of the PR.
Some of the undesirable changes are also present in these classes:
-
MainActivity
, -
ReadArticleActivity
, -
Themes
.
At this point I've only briefly looked over theme-related changes.
Thanks for taking your time on reviewing this!
That is, cosmetic/cleanup changes go into a separate commit.
Yeah. Actually I don't really intend to change coding styles. Those will just be reverted in this PR.
Braceless one-line ifs. Braceless multiline ifs are bad, but one-line ifs are a matter of taste. I don't mind them in the current code base.
Including this change. It was introduced by the formatter but shouldn't be just done in such a way.
Activity being replaced with AppCompatActivity. I have no problem with using a more specific class when it is needed, but I don't see any reason to replace it everywhere.
The reason: material components explicitly requires so. I'm not sure if the interpolation is done automatically, nor there will be any implications if we don't replace them so.
Using AppCompatActivity will ensure that all the components work correctly. If you are unable to extend from AppCompatActivity, update your activities to use AppCompatDelegate. This will enable the AppCompat versions of components to be inflated among other important things.
Including this change. It was introduced by the formatter but shouldn't be just done in such a way.
I'm not sure I understand what you're saying.
The reason: material components explicitly requires so. I'm not sure if the interpolation is done automatically, nor there will be any implications if we don't replace them so.
It means that AppCompatActivity
should be used as a parent class (directly or indirectly) for activities.
Instances of AppCompatActivity
can be handled as Activity
perfectly fine thanks to polymorphism.
You have to use AppCompatActivity
only under these circumstances:
- As a parent class for an activity (mentioned above).
- When you need to access methods that are available in
AppCompatActivity
, but not inActivity
(cast is possible, but may not be desirable). - In a method signature when you expect someone to pass
AppCompatActivity
and not some other type ofActivity
. - I guess there may be overloaded compat-methods different versions of which accept
AppCompatActivity
andActivity
. Not sure if there are many methods like this.
There may be some other cases, but I don't think they have anything to do with this PR.
OK. So then the diff becomes incredibly small... which is great! @di72nn
Sorry for the delay again.
I'll start with the fact that I tested this PR as it is on an Android 9 device and things are very broken: most of the time important parts of the screen are simply black, there are some other animation-related glitches.
I noticed that your last commit reverts some stuff in article.xml
and ReadArticleActivity
that may be needed.
It also reverts the default theme in Settings
.
Themes
:
-
applyDarkTheme()
should probably be called fromapplyTheme(Activity, boolean)
rather thanapplyTheme(Activity)
(becauseMainActivity
callsapplyTheme(Activity, boolean)
directly). -
applyDarkTheme()
should probably be also called fromapplyDialogTheme(Activity)
. - I think
theme.name().toLowerCase().contains("light")
should be replaced with a flag or enum field inThemes.Theme
, so lightness or darkness is not tied to the name. - There are some unused imports added.
Is shape.xml
used at all?
Any particular reason "splash" with the logo was removed?
Eh... I've tested on Android 11 for months, and it works well.
most of the time important parts of the screen are simply black
Do you mean light/black are mixed?
I noticed that your last commit reverts some stuff in article.xml and ReadArticleActivity that may be needed.
No, as long as you see the app bar.
It also reverts the default theme in Settings.
Yes, because it's not needed. The default theme is set application-wise, and covers the Settings.
Is shape.xml used at all?
Yes. https://github.com/wallabag/android-app/pull/1132/files#diff-683326448134b180d2575e28185ff2552667470a4c81d10271c59f0de0a781ffR47
I think theme.name().toLowerCase().contains("light") should be replaced with a flag or enum field in Themes.Theme, so lightness or darkness is not tied to the name.
Every theme should have their light/dark variant. And because of the current theme listing, it should be fine to list all themes in an enum. Further improvements could be addressed in another PR, and is large enough to be another PR.
Other things are fixed.
BTW, "it works fine" means it doesn't increase glitches.
And I'm planing to enhance the transition, which can only be done on top of this
Just tested on Android 8.1 emulator - glitches are there, you can see for yourself.
Just tested on Android 8.1 emulator - glitches are there, you can see for yourself.
Eh, so in older system we have to specify the opacity for android:colorBackground
. Otherwise, it's transparent by default. Thanks for pointing this out. Fixed.
Thanks!
Can you make the tab bar on the main screen not stand out as much in the dark theme? Currently it has a lighter background color than both title bar and list background.
The accent color in settings may differ in normal screens and in popup screens (like checkboxes in "Settings -> UI -> Math delimiters") depending on the selected theme.
I noticed that some icons (the checkmark, the star) in the title bar have slightly inconsistent color in some themes.
The overflow menu in ReadArticleActivity
is not properly themed when the Solarized theme is selected.
So also are popup windows ("Add new entry", settings popups), but these are broken in the current implementation too.
I haven't tested it on older Android versions yet.
And I'm planing to enhance the transition
What do you mean?
Can you make the tab bar on the main screen not stand out as much in the dark theme? Currently it has a lighter background color than both title bar and list background.
I make them both have 4dp elevation to fix the issue. The default elevation of a top app bar is 4dp in Material Design.
The accent color in settings may differ in normal screens and in popup screens (like checkboxes in "Settings -> UI -> Math delimiters") depending on the selected theme.
The MultiSelectPreference
pop-ups a legacy Dialog, which doesn't read the materialDialogOverlay. I fix this by injecting the material dialog theme to the old parameters anyway.
I noticed that some icons (the checkmark, the star) in the title bar have slightly inconsistent color in some themes.
The theme with a legacy ActionBar doesn't read the material parameters correctly. I fix this by draw the top app bar directly (which is a big change)
The overflow menu in
ReadArticleActivity
is not properly themed when the Solarized theme is selected. So also are popup windows ("Add new entry", settings popups), but these are broken in the current implementation too.
Ditto.
And I'm planing to enhance the transition
What do you mean?
The navigation transitions. I thought you mean the transition is flickered by glitches
. But that seems to be a misunderstanding.