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

Migrate to Android DayNight Theme, fix Light Theme, minor UI improvements #5927

Merged
merged 40 commits into from
Apr 3, 2021

Conversation

krlvm
Copy link
Contributor

@krlvm krlvm commented Mar 27, 2021

What is it?

  • Bugfix (user facing)
  • Feature (user facing)
  • Codebase improvement (dev facing)
  • Meta improvement to the project (dev facing)

Description of the changes in your PR

  • Fixed Dark Text & Dark Icons in Light Theme
  • Fixed Light Navigation Bar Icons in Light Theme
  • Navigation Bar in Splash Screen is red like its background now
  • All styles relies on Android's DayNight Theme now
  • Light Theme is now looks properly, so I changed default theme from Dark to Automatic
  • All _white and _black assets have been separated in drawable and drawable-night, ThemeHelper.resolveResourceIdFromAttr is no longer used - it should improve the overall performance, but I did not run benchmarks
  • Styles, attributes XML code cleanup
  • Slightly reduced installed app size (installed v0.21.0-release: 17MB -> 14,68MB debug version)

Fixes the following issue(s)

APK testing

On the website the APK can be found by going to the "Checks" tab below the title and then on "artifacts" on the right.

Due diligence

@krlvm krlvm changed the title Daynight Fix Light Theme, Migrate to Android DayNight Theme Mar 27, 2021
@krlvm krlvm marked this pull request as draft March 27, 2021 15:25
@krlvm krlvm marked this pull request as ready for review March 27, 2021 15:27
@AudricV AudricV added bug Issue is related to a bug codequality Improvements to the codebase to improve the code quality feature request Issue is related to a feature in the app GUI Issue is related to the graphical user interface labels Mar 27, 2021
@krlvm
Copy link
Contributor Author

krlvm commented Mar 27, 2021

I just finished checking everything down to the smallest detail on all topics, so I think I'm done.

Some screenshots compared to v0.21.0

@Stypox
Copy link
Member

Stypox commented Mar 27, 2021

@krlvm thank you for this huge improvement! It does have many benefits. :-D
Could you better explain how this new theming strategy works? Since there are ~300 changed files I don't understand which ones contain the important changes.
Also, did you test on API 19?

@TobiGr I'd give priority to this PR, is would be painful to rebase 300 files ;-)

@krlvm
Copy link
Contributor Author

krlvm commented Mar 27, 2021

@krlvm thank you for this huge improvement! It does have many benefits. :-D
Could you better explain how this new theming strategy works? Since there are ~300 changed files I don't understand which ones contain the important changes.
Also, did you test on API 19?

@TobiGr I'd give priority to this PR, is would be painful to rebase 300 files ;-)

In fact, this number is not scary, I just renamed all files like ic_x_white_24dp to ic_x, as well as all references to it. Likewise with dark versions moved to drawable-night. That's where the 300 changes come from, though
image
The theming strategy has not been changed much for that. To make the resource source automatically change from drawable to drawable-night, we use AppCompatDelegate.setDefaultNightMode(), which is available by using DayNight as the base theme.
To force invert the colors of the drawables (for example, the control buttons in the player are always white), we give the layout the WhiteTint theme.

Important changes are in:

  • ThemeHelper
  • styles.xml files
  • android:theme attribute for Toolbars and Player's container (WhiteTintTheme)
  • TabLayout icons colors changed via XML attributes too

Also got rid of icons in attrs.xml and ThemeHelper.resolveResourceIdFromAttr as I said before.

Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

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

Looks mostly good to me, I also tested a little bit and everything seems fine. Thank you! :-D

@krlvm krlvm requested a review from Stypox March 28, 2021 09:25
@krlvm
Copy link
Contributor Author

krlvm commented Mar 28, 2021

Also, did you test on API 19?

Screenshots

There also some problems with tab on-tap-highlight color (it is 100% opaque) on KitKat.

@Stypox
Copy link
Member

Stypox commented Mar 28, 2021

@krlvm Under white theme:
  • the icons in "Settings -> Content -> Peertube instances" and "Settings -> Content -> Content of main page" are black.
  • the file chooser has a black title (e.g. try to import database)
  • the error activity has white text on white background (to test this click "Settings -> Debug -> Crash the app" twice)
  • the search button when adding subscriptions to a subscription group is white, and the "v" overlay above selected subscriptions is black-on-black
  • the in-player queue and the in-player chapter list has white-on-white text (to test this enable auto enqueue and search for a video with chapters, e.g. the first result for "youtube chapters")
  • the in-player "Captions" and "Quality" selectors are dark even on white theme (maybe this is ok, open to discussion). This is the case for the selectors in the popup player
  • the queue accessible via a background/popup notification has an app bar with black icons and a strange shade at the bottom

@Stypox
Copy link
Member

Stypox commented Mar 31, 2021

The icons of the items in "Settings -> Content -> Content of main page -> + -> Kiosk page" are white under white theme, even though they should be black. This is not caused by your PR, but if it's not an issue please fix it while you are at it ;-)

@krlvm
Copy link
Contributor Author

krlvm commented Mar 31, 2021

Should be fixed now

@Stypox
Copy link
Member

Stypox commented Mar 31, 2021

  • This happens on API 19 and API 30, but not on my phone with API 29:

  • This happens only on API 19:

  • Some icons in the notification on API 19 are invisible (I noticed the buffering icon and the close icon in particular, but maybe even others, idk if this was caused by this PR)

@krlvm
Copy link
Contributor Author

krlvm commented Mar 31, 2021

Can't reproduce the last issue, two others were fixed

Copy link
Member

@Stypox Stypox left a comment

Choose a reason for hiding this comment

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

The player issue is still not fixed on API 19 (emulator):

app/src/main/res/values/styles.xml Outdated Show resolved Hide resolved
krlvm added 3 commits April 2, 2021 20:25
AppCompatImageButton ignores "tint" theme attribute on API 19, therefore, they had to be specified directly, these attributes can be removed after the KitKat support is dropped.
@krlvm krlvm requested a review from Stypox April 2, 2021 21:00
@TacoTheDank
Copy link
Member

TacoTheDank commented Apr 3, 2021

I'm actually really excited for this PR (DayNight usage is something I've wanted for quite a while, plus no more themed drawables). Hoping as many bugs as possible will be ironed out before merge-time :)

@Redirion
Copy link
Member

Redirion commented Apr 3, 2021

actually it appears this is already ironed out alot? Why not merge it and let it test with the RC for the next release?

@krlvm
Copy link
Contributor Author

krlvm commented Apr 3, 2021

I cannot find any more problems on the last two versions of Android, which I use, at least in the most used activities, but additional flaws on API 19 may be revealed. Anyway, thanks for taking the time to find those problems that I could not discover before opening the PR.

@TobiGr
Copy link
Contributor

TobiGr commented Apr 3, 2021

Did anyone test this on a TV?

@Stypox
Copy link
Member

Stypox commented Apr 3, 2021

@TobiGr I didn't think of that, I'm going to test now on an emulator

@Stypox
Copy link
Member

Stypox commented Apr 3, 2021

I tested again, even on an emulated TV (API 29), and couldn't find anything wrong, both in white and dark/black theme. I think this can be merged, since potential problems would anyway only be color problems, not usability issues.

@krlvm krlvm changed the title Fix Light Theme, Migrate to Android DayNight Theme Migrate to Android DayNight Theme, fix Light Theme, minor UI improvements Apr 3, 2021
@TobiGr TobiGr merged commit a3d74ea into TeamNewPipe:dev Apr 3, 2021
@krlvm krlvm deleted the daynight branch April 3, 2021 21:17
@undeadfox
Copy link
Contributor

Release build failed:
app/src/release/res/xml/main_settings.xml:43: AAPT: error: resource drawable/ic_cloud_download_black (aka org.schabi.newpipe:drawable/ic_cloud_download_black) not found.

@krlvm
Copy link
Contributor Author

krlvm commented Apr 3, 2021

Release build failed:
app/src/release/res/xml/main_settings.xml:43: AAPT: error: resource drawable/ic_cloud_download_black (aka org.schabi.newpipe:drawable/ic_cloud_download_black) not found.

Try replacing ic_cloud_download_black with ic_cloud_download

This was referenced Apr 11, 2021
tossj pushed a commit to tossj/NewPipe-legacy that referenced this pull request Apr 23, 2021
Migrate to Android DayNight Theme, fix Light Theme, minor UI improvements
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issue is related to a bug codequality Improvements to the codebase to improve the code quality feature request Issue is related to a feature in the app GUI Issue is related to the graphical user interface
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Polish white theme: make app bar text and icons white
7 participants