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

Remove icon duplicates and fix some theming issues #7518

Merged
merged 5 commits into from
Mar 15, 2022

Conversation

mauriciocolli
Copy link
Contributor

@mauriciocolli mauriciocolli commented Dec 8, 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

Some icons are not consistently themed (it affects the other pull request #7516):

Drawing Drawing

  • Remove unnecessary duplicates of some icons, as it is a bad practice that leads to mistakes. As vectors are used, color resources are available.
  • Use menu resources instead of manually creating it, let the system do its job and get the correct resources.
    • Some menu items were not themed properly because the toolbar itself had a custom theme overriding some values.

Now only some drawables are different in behavior than the rest of the ic_ prefixed icons:

  • The heart icon.
  • Headphone with shadow.
  • Play with shadow.

Do you think they should be renamed to something else to make it consistent?

Due diligence

@mauriciocolli mauriciocolli added bug Issue is related to a bug feature request Issue is related to a feature in the app GUI Issue is related to the graphical user interface labels Dec 8, 2021
@Stypox
Copy link
Member

Stypox commented Dec 8, 2021

Woah, the APK size is reduced by 300kb :-D
Great work, I will review and give feedback soon. I tried other times, too, but never found out how to properly style icons on Android without creating duplicates everywhere or having to write Java code for each icon. Just one thing: does this work well in all situations on API 19 too?

@mauriciocolli
Copy link
Contributor Author

Just one thing: does this work well in all situations on API 19 too?

I think so, the app is currently using the support library for vectors and the proper build configuration is already set.

Also, the test icon preview in the original post was working in every API I tested (19,23,26,28,30,31,32-preview). Both using it in layouts (normal layouts, menu, drawer) and getting the drawable with AppCompatResources.getDrawable.

I think that should cover all uses, right?

@litetex
Copy link
Member

litetex commented Dec 12, 2021

@mauriciocolli
How did you get the view with all icons? (in the PR-description)

Copy link
Member

@litetex litetex left a comment

Choose a reason for hiding this comment

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

LGTM
Tested it in my emulator (Android 11) and didn't saw any specific problems.

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.

Code looks good. I could only find one issue after testing anything that came to my mind in both white and dark theme on API 19:

  • on white theme the icon on top of local and remote playlist items is black instead of white
    image

@TacoTheDank
Copy link
Member

This is fantastic work. Thanks so much! 💟

@litetex
Copy link
Member

litetex commented Dec 27, 2021

Note: This PR might be affected by #7577 and vice versa.

@litetex litetex added the waiting for author If the author doesn't respond, the issue will be auto-closed. Otherwise the label will be removed. label Jan 21, 2022
Doing this programmatically is just a no-go when themes are being set
in some other places (the toolbar is using a custom theme, in this
case), so, instead of hunting down the proper theme, just let the
system do its work.
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.

I rebased and pushed a commit that fixes the issue I reported above. I tested again various miscellaneous things on API 19, on API 22, and on my API 29 phone, and I found no issue. Ready for merge in my opinion. @litetex

I think the only possible regressions this PR could introduce are in places that retain the same color under all themes (the playlist icon is an example of this). Therefore I tested well the player (which behaves that way), but could not think of other places where the color of an icon should be independent of theme (except for the already mentioned&fixed playlist icon). So I think we are safe to merge this PR as there should be no regressions, and even if there are, they will not be much impacting.

@sonarcloud
Copy link

sonarcloud bot commented Mar 2, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

@litetex litetex removed the waiting for author If the author doesn't respond, the issue will be auto-closed. Otherwise the label will be removed. label Mar 15, 2022
@litetex litetex merged commit 0158f13 into TeamNewPipe:dev Mar 15, 2022
Stypox added a commit to Stypox/NewPipe that referenced this pull request Mar 20, 2022
They were added by accident in PRs not properly rebased on top of TeamNewPipe#7518, they can be removed safely.
@Stypox Stypox mentioned this pull request Apr 16, 2022
12 tasks
@krlvm krlvm mentioned this pull request Jul 13, 2022
5 tasks
@AudricV AudricV mentioned this pull request Aug 20, 2023
5 tasks
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 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.

4 participants