Fix toolbar icon and text color in white theme by IAmSSH · Pull Request #3482 · TeamNewPipe/NewPipe

IAmSSH · 2020-04-25T09:47:40Z

What is it?

Bug fix (user facing)
Feature (user facing)
Code base improvement (dev facing)
Meta improvement to the project (dev facing)

Description of the changes in your PR

Create a new theme ToolbarTheme in styles.xml to set it on the toolbar to get the desired color and icons.
Modify Tab.java and KioskTranslator.java to always return icons having white color.

Fixes the following issue(s)

Fixes Polish white theme: make app bar text and icons white #3042

Testing apk

NewPipe.zip

Agreement

I carefully read the contribution guidelines and agree to them.

IAmSSH · 2020-04-25T09:59:34Z

@Stypox
Request your review please.
Thanks.

Stypox

What you did seems correct, I only pointed out one thing that could be improved. Thank you :-D

app/src/main/res/values/styles.xml

IAmSSH · 2020-05-01T11:30:58Z

I know its not good practice, I tried doing that, but there's a problem.
If we inherit DarkTheme, we essentially also inherit its parent the Theme.AppCompat.NoActionBar. There are some styles in Theme.AppCompat.NoActionBar that make the text in the spinner that is used to select the quality of a video (in video detail fragment) appear black along with its background, so it becomes unreadable.
I tried overriding some styles but it didn't work out.
I'll try some more and let you know.

IAmSSH · 2020-05-04T06:51:59Z

I changed the spinner items to always be of red colour, irrespective of the theme selected.

Stypox · 2020-05-04T19:17:34Z

I just realized one thing: maybe there is no need to create a new style in styles.xml just for icons, but it is enough to force the white drawables (instead of using ?attr/) in all menus, since there is only one global style for toolbars. Anyway, the new code looks better, and if my above assumption can't be implemented it is good to go after some testing ;-)
Thank you for the patience ;-D

IAmSSH · 2020-05-06T03:50:04Z

I think the hamburger icon's colour cannot be changed without using the DrawerArrowStyle style, since it does not belong to our set of style icons (defined in Dark and Light themes), but is preset. So, it might be necessary to keep the style.

Stypox · 2020-05-06T19:42:11Z

Ok, then this is correct approach.

I found some issues (tested on an emulator with Android 4.4 API 19):

The background color of the toolbar does not change according to the service (i.e. try with soundcloud). It should have the same background color as the tab selector in the main page.
The background color of the stream selector in the video detail activity should not be red, but should follow the chosen app theme (i.e. dark if dark theme, white if light theme, etc.), just like three-dot menus do.
The color of the ______ line under the currently selected tab in the main page should always be white, but instead it changes color when changing theme.
In download activity the color of the "List" toolbar button is black, while it should be white.
The toolbar of the about activity still has black icons and text when using white theme.

IAmSSH · 2020-05-07T09:20:31Z

Okay, I'll look into it

IAmSSH · 2020-05-08T09:26:08Z

Regarding the last comment:

The background color of the stream selector in the video detail activity should not be red, but should follow the chosen app theme (i.e. dark if dark theme, white if light theme, etc.), just like three-dot menus do.

You mean

The menu to select stream quality is red but it should be like the colour of dot menu popup, i.e

The dot menu popup is always of the colour as shoown in the image irrespective of the stream or the theme, so should I make it similar to that (gray background and white text), or you meant something else?

Stypox · 2020-05-08T13:54:49Z

Oh, that's another issue. Both menus should have dark background with the dark theme and light background with the light theme. I guess you need to do something similar to what you did with DrawerArrowStyle

IAmSSH · 2020-05-10T04:54:36Z

The app is working fine now. All issues are resolved.
The only thing remaining is the colour of the spinner in video detail fragment. It does not change according to Light and Dark themes, but is consistent with the colours of different services.
I think it appears fine like this. If you want it to be changed do let me know.

Stypox · 2020-05-10T19:48:23Z

Thank you for fixing the issues! I have found some more things to improve ;-)

The "save playlist" toolbar button in the playlist fragment is black in white theme, while it should always be white
The download dialog (opened from the video detail fragment) has two issues: the stream selector has a red background color, while its background should follow the theme, and the back button (top left) is black in white theme, while it should always be white
The toolbar in background/popup player activities still has black icons and title in white theme, while they should be white

These two issues you posted screenshots above are still not solved:

The three-dot menu has a dark background independently of the theme, while it should follow the theme
The stream selector in video detail fragment has a red background, while it should follow the theme

IAmSSH · 2020-05-12T06:25:28Z

There were some problems with the current approach. Since the Toolbar had the DarkTheme as the parent, so the styles of DarkTheme were getting applied to it statically and they were not changing with the Themes or the Services and it created problems everywhere.

So I had to revert back to the earlier approach that involved copying icons in the ToolbarTheme, it solved all the problems.

IAmSSH · 2020-05-25T03:36:45Z

Hey,
I removed the dead code.
Is there anything else that is remaining?

wb9688 · 2020-05-25T08:45:52Z

@IAmSSH: #3271 changed the names of some icons, so you have to change your PR accordingly.

PS: Please stay longer on IRC until we've seen your question and answered, or use e.g. Matrix.

app/src/main/res/values/styles.xml

IAmSSH · 2020-05-28T06:11:39Z

The required changes are done.

PS: @wb9688 Sorry about the IRC. I was really having a hard time figuring out how to work with it. I have set up Matrix now.

Stypox

All of these problems refer to the app when the white theme is active.

The subtitle in subscription groups "What's New" toolbar is dark grey, while it should be light gray.
The search bar description/hint is dark grey, while it should be light gray. The flashing indicator is black instead of white.

Other than this, everything seems good. I tested on Android 4.4 API 19 and on Android 7.0

Stypox · 2020-05-28T06:54:09Z

app/src/main/java/org/schabi/newpipe/MainActivity.java

        }

-        setSupportActionBar(findViewById(R.id.toolbar));
+        Toolbar toolbar = findViewById(R.id.toolbar);


Add final here, or revert this change

Stypox · 2020-05-28T06:58:05Z

app/src/main/java/us/shandian/giga/ui/fragment/MissionsFragment.java

+            boolean isLight = ThemeHelper.isLightThemeSelected(mContext);
+            int icon;
+
+            if (mLinear)
+                icon = R.drawable.ic_apps_white_24dp;
+            else
+                icon = R.drawable.ic_list_white_24dp;
+
+            mSwitch.setIcon(icon);


I prefer the ?: syntax, also I think isLight is a leftover from the merge, because it is not needed

Suggested change

boolean isLight = ThemeHelper.isLightThemeSelected(mContext);

int icon;

if (mLinear)

icon = R.drawable.ic_apps_white_24dp;

else

icon = R.drawable.ic_list_white_24dp;

mSwitch.setIcon(icon);

mSwitch.setIcon(mLinear ? R.drawable.ic_apps_white_24dp : R.drawable.ic_list_white_24dp);

Stypox · 2020-05-28T06:59:52Z

app/src/main/res/layout/error_retry.xml

        android:textAlignment="center"
        android:textAllCaps="true"
        android:textAppearance="@style/TextAppearance.AppCompat.Body1"
+        android:background="?attr/colorPrimary"


Why was this added? Isn't the @style/ServiceColoredButton added by @B0pol enough?

Stypox · 2020-05-28T07:27:22Z

app/src/main/java/org/schabi/newpipe/MainActivity.java

+        toolbar.setPopupTheme(
+                ThemeHelper.isLightThemeSelected(this)
+                ? R.style.PopupThemeLight
+                : R.style.PopupThemeDark
+        );


Why not use R.style.ToolbarTheme here, too?

Stypox · 2020-05-28T07:30:03Z

app/src/main/java/org/schabi/newpipe/fragments/detail/VideoDetailFragment.java

+                RelativeLayout r = (RelativeLayout) parent.getChildAt(0);
+                ((TextView) r.getChildAt(1)).setTextColor(getResources().getColor(R.color.white));
+                ((TextView) r.getChildAt(2)).setTextColor(getResources().getColor(R.color.white));


Isn't there any other way to do this? It seems to be a workaround to me, though if there is no other way then ok, I guess the toolbar inflated layout is always the same.

I couldn't find any direct way of styling just the first element of a spinner, so had to use this approach.

Stypox · 2020-05-28T07:32:14Z

app/src/main/java/org/schabi/newpipe/fragments/list/playlist/PlaylistFragment.java

+        playlistBookmarkButton.setIcon(iconAttr);
        playlistBookmarkButton.setTitle(titleRes);


While you are at it, you could move the variables above directly here:

Suggested change

playlistBookmarkButton.setIcon(iconAttr);

playlistBookmarkButton.setTitle(titleRes);

playlistBookmarkButton.setIcon(playlistEntity == null

? R.drawable.ic_playlist_add_white_24dp

: R.drawable.ic_playlist_add_check_white_24dp);

playlistBookmarkButton.setTitle(playlistEntity == null

? R.string.bookmark_playlist

: R.string.unbookmark_playlist);

Stypox · 2020-05-28T07:33:57Z

app/src/main/java/us/shandian/giga/ui/fragment/MissionsFragment.java

+            boolean isLight = ThemeHelper.isLightThemeSelected(mContext);
+            int icon;
+
+            if (mLinear)
+                icon = R.drawable.ic_grid_white_24dp;
+            else
+                icon = R.drawable.ic_list_white_24dp;
+
+            mSwitch.setIcon(icon);


Here, too. Also, isLight is unused.

Suggested change

boolean isLight = ThemeHelper.isLightThemeSelected(mContext);

int icon;

if (mLinear)

icon = R.drawable.ic_grid_white_24dp;

else

icon = R.drawable.ic_list_white_24dp;

mSwitch.setIcon(icon);

mSwitch.setIcon(mLinear

? R.drawable.ic_grid_white_24dp

: R.drawable.ic_list_white_24dp);

app/src/main/res/layout/error_retry.xml

IAmSSH · 2020-06-01T17:22:46Z

Done @Stypox

Stypox

Code looks good! Thank you for the patience. I ran a thorough test this time, hopefully I haven't missed anything, so these are the problematic things I could find while using white theme:

Black title of "Settings -> Download -> Video/Audio download folder"
Black "Restore defaults" toolbar icon, item icons (also in the "Choose tab" dialog) and + FAB of "Settings -> Content -> Content of main page"
Black "Restore defaults" toolbar icon and + FAB of "Settings -> Content -> PeertTube instances"
Black title of "Settings -> Content -> Import/Export database"
Black cursor pointer in the search bar (it appears when moving the cursor along the text, or when selecting something)
Black selection in the search bar (dark grey instead of light grey)
White unselected item circles in the three-dot menu to choose the search filters (they should be dark dark gray, like in e.g. the download dialog)
The spinner arrow-down icon flashes black when the spinner is clicked on (then turns back to white in the flash of an eye). Even though the flashing behaviour apparently exists in old versions of android (4.4, but not on my 7.0 phone), I think it should just stay white even when clicked on, as this is also what happens when the dark theme is active.
When the device is in landscape mode popup/background queue toolbars have black title and icons
Black title of "Subscriptions -> Import from -> Previous export"
Black title of "Subscriptions -> Export to -> File"

wb9688 · 2020-06-27T17:32:19Z

@TobiGr: Should we move this to v0.20.0 seeing how @IAmSSH hasn't replied for 25 days and it not being a bug fix (so not really worth it fixing it ourself imho)?

AudricV · 2021-03-28T16:59:50Z

Closing in favor of #5927.

fix toolbar UI

e8fa58a

Stypox requested changes Apr 30, 2020

View reviewed changes

app/src/main/res/values/styles.xml Show resolved Hide resolved

wb9688 added this to the 0.19.4 milestone May 1, 2020

refactor Toolbar UI improvement code

34c4cd4

Stypox self-assigned this May 4, 2020

change toolbar color according to service, minor bug fixes

9c6ca66

Stypox mentioned this pull request May 10, 2020

Change retry & subscribe buttons background color based on service color #3565

Merged

5 tasks

copy icons into ToolbarTheme, create PopupTheme

c8f1403

Stypox self-requested a review May 13, 2020 08:33

B0pol added the bug Issue is related to a bug label May 17, 2020

IAmSSH and others added 2 commits May 25, 2020 08:51

remove dead code

a017c54

Merge branch 'dev' into improve-toolbar-design

e23648e

Stypox requested changes May 25, 2020

View reviewed changes

app/src/main/res/values/styles.xml Outdated Show resolved Hide resolved

IAmSSH added 3 commits May 25, 2020 16:40

fix toolbar UI

7c84d29

refactor Toolbar UI improvement code

60e51e9

change toolbar color according to service, minor bug fixes

ac34141

IAmSSH added 4 commits May 25, 2020 16:53

copy icons into ToolbarTheme, create PopupTheme

91638b3

remove dead code

4a84aa0

code compliant to latest icon changes

f09238e

resolve merge conflicts

b11759d

Stypox requested changes May 28, 2020

View reviewed changes

Stypox modified the milestones: 0.19.4, 0.19.5 May 28, 2020

code refactor as per suggested changes

06c1856

Stypox requested changes Jun 2, 2020

View reviewed changes

TobiGr modified the milestones: 0.19.6, 0.20.0 Jun 27, 2020

wb9688 modified the milestones: 0.20.0, 0.20.1 Jul 29, 2020

Stypox mentioned this pull request Aug 11, 2020

A few thoughts concerning newpipe's design #4088

Closed

Stypox changed the title ~~fix toolbar UI~~ Fix toolbar icon and text color in white theme Oct 4, 2020

Stypox modified the milestones: 0.20.1, 0.20.2 Oct 4, 2020

TobiGr force-pushed the dev branch 2 times, most recently from 679bc75 to 2aeccc0 Compare March 16, 2021 08:24

krlvm mentioned this pull request Mar 27, 2021

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

Merged

5 tasks

AudricV closed this Mar 28, 2021

AudricV unassigned Stypox Mar 28, 2021

		playlistBookmarkButton.setIcon(iconAttr);
		playlistBookmarkButton.setTitle(titleRes);

-        playlistBookmarkButton.setIcon(iconAttr);
-        playlistBookmarkButton.setTitle(titleRes);
+        playlistBookmarkButton.setIcon(playlistEntity == null
+                ? R.drawable.ic_playlist_add_white_24dp
+                : R.drawable.ic_playlist_add_check_white_24dp);
+        playlistBookmarkButton.setTitle(playlistEntity == null
+                ? R.string.bookmark_playlist
+                : R.string.unbookmark_playlist);

Uh oh!

Conversation

IAmSSH commented Apr 25, 2020 • edited by opusforlife2 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is it?

Description of the changes in your PR

Fixes the following issue(s)

Testing apk

Agreement

Uh oh!

IAmSSH commented Apr 25, 2020

Uh oh!

Stypox left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

IAmSSH commented May 1, 2020

Uh oh!

IAmSSH commented May 4, 2020

Uh oh!

Stypox commented May 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

IAmSSH commented May 6, 2020

Uh oh!

Stypox commented May 6, 2020

Uh oh!

IAmSSH commented May 7, 2020

Uh oh!

IAmSSH commented May 8, 2020

Uh oh!

Stypox commented May 8, 2020

Uh oh!

IAmSSH commented May 10, 2020

Uh oh!

Stypox commented May 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

IAmSSH commented May 12, 2020

Uh oh!

IAmSSH commented May 25, 2020

Uh oh!

wb9688 commented May 25, 2020

Uh oh!

Uh oh!

IAmSSH commented May 28, 2020

Uh oh!

Stypox left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

IAmSSH commented Jun 1, 2020

Uh oh!

Stypox left a comment

Choose a reason for hiding this comment

Uh oh!

wb9688 commented Jun 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AudricV commented Mar 28, 2021

Uh oh!

Reviewers

Assignees

Labels

IAmSSH commented Apr 25, 2020 •

edited by opusforlife2

Loading

Stypox commented May 4, 2020 •

edited

Loading

Stypox commented May 10, 2020 •

edited

Loading

wb9688 commented Jun 27, 2020 •

edited

Loading