Material theming

[proletarius101]

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 and For e-ink devices needs dark themes? Yes
• Do Solarized and For e-ink devices needs a "Follow system" option? Let them be by default

[proletarius101]

Ready for a review

[di72nn]

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?

[proletarius101]

Can you explain why you use this. in MainActivity.java everywhere?

Sorry I forgot to turn off the hook which was set to hardening my programs. Apparently it pollutes the changelog. Reverted.

[proletarius101]

@di72nn Any updates?

[proletarius101]

Will it be more attractive if I post some screenshots?

[di72nn]

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 @Overrides:

• 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.

[proletarius101]

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.

[di72nn]

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 in Activity (cast is possible, but may not be desirable).
• In a method signature when you expect someone to pass AppCompatActivity and not some other type of Activity.
• I guess there may be overloaded compat-methods different versions of which accept AppCompatActivity and Activity. 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.

[proletarius101]

OK. So then the diff becomes incredibly small... which is great! @di72nn

[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 from applyTheme(Activity, boolean) rather than applyTheme(Activity) (because MainActivity calls applyTheme(Activity, boolean) directly).
• applyDarkTheme() should probably be also called from applyDialogTheme(Activity).
• 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.
• There are some unused imports added.

Is shape.xml used at all?

Any particular reason "splash" with the logo was removed?

[proletarius101]

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.

[proletarius101]

BTW, "it works fine" means it doesn't increase glitches.

[proletarius101]

And I'm planing to enhance the transition, which can only be done on top of this

[di72nn]

Just tested on Android 8.1 emulator - glitches are there, you can see for yourself.

[proletarius101]

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.

[di72nn]

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?

[proletarius101]

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.