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

Improve image placeholders #8530

Merged
merged 9 commits into from
Jul 14, 2022
Merged

Conversation

krlvm
Copy link
Contributor

@krlvm krlvm commented Jun 21, 2022

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

Currently, placeholders are shown instead of avatars or video previews only in case of an error during loading, including a timeout. This is not a good solution for a slow or unstable internet connection, or if the government blocks the domain with user avatars (yt3.ggpht.com) - the application waits for a very long time to load and in the end it will display the placeholder, but all this time on this place is empty, which makes it difficult to read comments and use the application as a whole.

Also, the placeholders didn't display very nicely on high resolution screens, so I replaced them with vector ones based on the SVG originals from the assets directory. I adapted them to the light theme, as they don't look very consistent and don't fit very well.

  • Show placeholders until the image is loaded because timeout can be very long and missing profile pictures and video thumbnails make app inconvenient to use
  • Adapt profile picture and video thumbnail placeholders to light theme
  • Replace profile picture and video thumbnail placeholders with vector graphics

Before/After Screenshots/Screen Record

Before After
before after
before2 after2

+2 thousand views per minute

APK testing

The APK can be found by going to the "Checks" tab below the title. On the left pane, click on "CI", scroll down to "artifacts" and click "app" to download the zip file which contains the debug APK of this PR.

Due diligence

@litetex litetex added the niche Only relevant to a very small amount of people label Jun 21, 2022
@Stypox
Copy link
Member

Stypox commented Jul 6, 2022

What about using this material icon instead of creating a new SVG icon? This way we surely don't have styling problems, we don't have to maintain one more icon and we have a more standard look.

@krlvm
Copy link
Contributor Author

krlvm commented Jul 7, 2022

I thought using of custom icon was intentional, this look better indeed

@krlvm krlvm marked this pull request as draft July 7, 2022 19:57
@krlvm krlvm marked this pull request as ready for review July 7, 2022 20:07
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.

  • Is the file dummy_thumbnail_dark.png still used? Remove it otherwise
  • I would rename buddy.xml to dummy_person.xml
  • I think you have colors swapped for the video thumbnail and the playlist thumbnail: image
  • Even though you created different colors in values-night and then you used them in all placeholders, it doesn't seem to have worked, as the same image is shown regardless of the theme (light, dark or black)

@krlvm krlvm force-pushed the improve_placeholder_images branch from 405d23c to 32af043 Compare July 13, 2022 20:26
@krlvm
Copy link
Contributor Author

krlvm commented Jul 13, 2022

Can't reproduce the last one, it works like #7518 does

dummy_thumbnail_dark.png is still used here, but I've resized to 1x1px because it is just filled with black color.

@krlvm krlvm requested a review from Stypox July 13, 2022 20:46
@Stypox
Copy link
Member

Stypox commented Jul 14, 2022

Can't reproduce the last one, it works like #7518 does

These are the same colors, I tested with a color picker, but the theme is "black" in the first image and "light" in the second.
image
image
I tried to tackle this, but unfortunately it seems to be a problem in Picasso, see square/picasso#1275. I therefore removed the differences between themes for now, leaving the colors as they have always been in NewPipe.


Also, the play arrow's left line does not seem to be completely straight:
image
I pushed a commit improving the svg paths (also for the playlist), tell me if they look good to you ;-)


dummy_thumbnail_dark.png is still used here, but I've resized to 1x1px because it is just filled with black color.

I would remove dummy_thumbnail_dark.png completely and set the overlay thumbnail to a null image when nothing is to be shown. I pushed a commit for this, too, tell me if it looks good to you.


I also renamed all of the placeholder images (also e.g. the channel banner) to placeholder_* in another commit.

@krlvm
Copy link
Contributor Author

krlvm commented Jul 14, 2022

LGTM, night mode colors worked for me though, at least on Android 10+.
I think there should be a light version of placeholder_person - the dark version is too contrasting with the background.

@Stypox
Copy link
Member

Stypox commented Jul 14, 2022

LGTM, night mode colors worked for me though, at least on Android 10+.

Yes, they do follow the device's night/day theme. But if you set the theme manually in NewPipe's settings, then it doesn't work anymore.

krlvm and others added 8 commits July 14, 2022 14:14
- Show placeholders until the image is loaded because timeout can be very long and missing profile pictures and video thumbnails make app inconvenient to use

- Adapt profile picture and video thumbnail placeholders to light theme

- Replace profile picture and video thumbnail placeholders with vector graphics
Theme customization does not seem to work well with Picasso: square/picasso#1275
@krlvm
Copy link
Contributor Author

krlvm commented Jul 14, 2022

Did not think about it.
Added person placeholder for light theme with hardcoded colors.

After making the playlist and video thumbnails' scaleType fitCenter, the 24dp*24dp thumbnails would appear as a square, which would be strange, since the image view is 16:9.
@Stypox Stypox force-pushed the improve_placeholder_images branch from 55a83f4 to 9f993e0 Compare July 14, 2022 12:48
@Stypox
Copy link
Member

Stypox commented Jul 14, 2022

Mmmh, I think the background and foreground colors should be the same across all placeholders. For example, I think the first image below better fits the app style when thumbnails are disabled.
image
image

Maybe we should just increase the darkness of the foreground color for each placeholder; would something like this do, in your opinion?
image

Btw, I rebased on dev and then resized SVGs to 16:9 so that with the new scaleType:fitCenter they appear at the correct size in thumbnails.

@krlvm
Copy link
Contributor Author

krlvm commented Jul 14, 2022

I think the cover thumbnails darkness is perfect and suits the light theme too, so it can be universal.

Unfortunately, proper drawable resource can't be selected too due to Picasso bug, so I think we will have to use the same placeholders for both themes.

Official YouTube app person thumbnails:
image

@krlvm krlvm force-pushed the improve_placeholder_images branch from 0bf12be to 9f993e0 Compare July 14, 2022 13:24
@Stypox Stypox force-pushed the improve_placeholder_images branch from 555b40a to 9f993e0 Compare July 14, 2022 13:41
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.

Ok, then I am good with these changes. Thank you for the PR and the feedback :-D

Unfortunately, proper drawable resource can't be selected too due to Picasso bug, so I think we will have to use the same placeholders for both themes.

Maybe by looking more into it it is possible to find a solution, but I don't think it is so important as of now. Though, if you want to give it a try in a new PR, it would be accepted.

@sonarcloud
Copy link

sonarcloud bot commented Jul 14, 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

@Stypox Stypox merged commit 33e2076 into TeamNewPipe:dev Jul 14, 2022
@Stypox
Copy link
Member

Stypox commented Aug 4, 2022

Currently, placeholders are shown instead of avatars or video previews only in case of an error during loading, including a timeout. This is not a good solution ...

For reference, this behavior was initially requested in #5928

@opusforlife2
Copy link
Collaborator

You mean this #5928 (comment)?

@Stypox
Copy link
Member

Stypox commented Aug 11, 2022

Yes

@Stypox Stypox mentioned this pull request Aug 27, 2022
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
niche Only relevant to a very small amount of people
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants