-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
FR: Full width thumbnails aka card view mode #9310
Conversation
Can someone tell me how to take over the world with Newpipe? i really need this feature Also creating clones will be a nice addition to the app |
Updated the PR description |
Add YT channel's icon left to the video titles For more feedback: #6087 (comment) |
@tvnmguy This is not implemented for two reasons
This can be a separate FR and based on votes/requirements it can be taken up. |
Latest NewPipe release; PR Debug; {---------}, which is misbehaving in PR Debug. •Completely missing both sides of the thumbnails. •There is a gap in {---------} between each items (In latest release, there is no gap between them and also which was a two layer combination). •In Newpipe release {---------} aligned next below to the refresh bar line. Openion; I think this card layout with proper arrangement of [{---------} this] would be better. For more feedback, please check this comment: #6087 (comment) |
This comment was marked as off-topic.
This comment was marked as off-topic.
@SameenAhnaf Above two can be done in a separate PR. Including them will make code review harder. @tvnmguy I'll look into the double line issue |
@tvnmguy Fixed the gap between dashed lines and increased the gap on right/left in feed section. In other screens, the same edge to edge rendering is used.
|
d4f28f1
to
87c462d
Compare
Kudos, SonarCloud Quality Gate passed! |
First of all a millions of Thanks🎉 for all your hard work. 1: If nothing new, remove the contraction of the thumbnail after feed updated. 2: (A): Make the gap between sides of the thumbnail only required for the dashed lines (Let's say 1mm for the dashed line, then 1mm for the gap). Benefit: The thumbnail doesn't feel too much contracted (Feels non contracted in normal view). (B): Make only new feed items contracted for dashed line and rest of them in normal state. Other improvements:- 1: Adjust gaps above and below of the titles and other details texts between thumbnails like this👇. 2: Adjust position of the duration card like 2nd one. 3: Remove upper bar of the thumbnails (when press and hold). 4: Add white gradient layer (when press and hold) coverage over the thumbnails too. |
Other problems: 1: when press and hold; (A) For Playlists: The alignment not correct in that area. (B) For Albums: The album split into two parts. 2: I don't think this problem is associated with card view because NP stable has the same problem. Anyway I mention that one as well. In search segment, the album doesn't show total number of videos on the cover. But it show in bookmarked segment. |
Making more customization specific to card layout will make the code-base prone to cosmetic errors and will break the existing generic UI paradigm. Also, it is not ideal to raise broader PR since code review won't have particular thing to focus. |
What's your thoughts about this like card view ? |
Hello, I would like to ask why the thumbnail quality is low in Card view? Also the last proposed design by @tvnmguy looks nice. |
Thumbnail quality is being addressed by a different PR in extractor. Once it is merged, we can start utilizing the HQ ones. See comment: #6087 (comment). Vote this PR : TeamNewPipe/NewPipeExtractor#889. As far as we don't make much customization per view type, it's good. Please let us decide on following card view variants before start with changes. Please consider these aspects and come up with mocks.
|
So local playlists will preserve the list mode UI, right? Is that also the case for other playlists? A unified layout would make more sense imo. |
@PassionateG1t Here you can see the screenshot. #9310 (comment) I made changes, but it doesn't make sense for playlist to have larger thumbnails. It just reduces the number of visible items. Streams inside playlist, has a full width variant. You can find screenshot in first or second comment. |
Mmmh, I think for consistency it would be better not to do per-screen customizations. If the user wants big thumbnails, then he will have big thumbnails everywhere. If he doesn't like big thumbnails, he can choose intermediate-sized ones (grid) or small ones (list). This also makes the code and the decision-making simpler. If in the future we find out this is a bad choice we can always change the type of some screen without issues.
You might say so, but maybe somebody else would disagree. If someone chooses big thumbnails because the channels they are subscribed to have more information in thumbnails, then the history will most likely contain videos from those channels and they would want to see big thumbnails there, too. |
@Stypox Except the history screen, others have full width cards now. I'll update the history screen sometime this week. |
@mahendranv @Stypox I think, #8412 needs to be implemented in a separate PR as some users may want to access |
87c462d
to
4c2ae28
Compare
@Stypox - Updated history screen to use full width card now. @SameenAhnaf - Yeah.. for TV UX, #8412 and #9635 (comment) should cover most usecases. In this PR, we have increased the thumbnail size - this should slightly improve usability. |
This comment was marked as off-topic.
This comment was marked as off-topic.
@tvnmguy This is not specifically relevant to |
@SameenAhnaf / @Stypox - Can you start the workflow? |
@mahendranv One more modification is required. #9555 is expected to add support for big thumbnails for channels in But channels in search results should be shown this way. Thumbnail should be in the middle of left side and description should auto-fit as required. |
app/src/main/java/org/schabi/newpipe/info_list/InfoListAdapter.java
Outdated
Show resolved
Hide resolved
app/src/main/java/org/schabi/newpipe/info_list/holder/StreamCardInfoItemHolder.java
Outdated
Show resolved
Hide resolved
app/src/main/java/org/schabi/newpipe/local/LocalItemListAdapter.java
Outdated
Show resolved
Hide resolved
app/src/main/java/org/schabi/newpipe/info_list/InfoListAdapter.java
Outdated
Show resolved
Hide resolved
app/src/main/java/org/schabi/newpipe/local/feed/FeedFragment.kt
Outdated
Show resolved
Hide resolved
|
||
<dimen name="video_item_grid_thumbnail_image_width">280dp</dimen> | ||
<dimen name="video_item_grid_thumbnail_image_height">160dp</dimen> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are these for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For tablets and TV, this increases the thumbnail size. #8412 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe these are a bit too big, but let's see how user perceive it, no need for you to make changes.
(leaving here some other plausible values I found)
<dimen name="video_item_grid_thumbnail_image_width">208dp</dimen>
<dimen name="video_item_grid_thumbnail_image_height">117dp</dimen>
41e948d
to
7924bb5
Compare
…tates full width thumbnails and dubbed as card mode.
@SameenAhnaf I'll raise a separate PR. And provide with mocks.
|
@Stypox - Addressed review comments - pls start the workflow |
Kudos, SonarCloud Quality Gate passed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good to me. I tested on various devices with different sizes and APIs and even Android TV and it works well. Thank you!
Found this comment on reddit:
So apparently some users perceive it this way. Maybe it's better to revert the part of this PR that makes thumbnails bigger, or choose some different values? |
Don't we have a separate XML for TV dimensions? |
Well, considering the issue was originally raised because of small thumbnails on TVs, it wouldn't make much sense to make thumbnails on TVs smaller and tablet ones bigger |
Maybe it's navigation that needs to change, then? Instead of going row by row, it could go page by page? |
I think remote control can only go right, left, up, down. So if we mapped the "down" to "go down one page" we would have no way to "go down one row". |
How about going off the edge (pressing Right on the last video in the row, or pressing Left on the first video) makes it go into 'page navigation mode', which you can exit again by pressing Left or Right? After which it goes back to highlighting videos in rows? |
That would conflict with going to the next main page tab, wouldn't it? |
Then, when in "row navigation mode", having the cursor/selector go off-screen (when pressing up or down) shouldn't scroll, but instead highlight the tab header again. So, scrolling would be done in "page mode" only. |
For TV, the normal layout and focusables won't help. Basic requirement is DPad accelerated smooth scrolling. DPad recyclerview is a third party lib optimised for TV. You can see demo videos in the readMe. https://github.com/rubensousa/DpadRecyclerView It's still in Alpha. But worth try. |
This comment was marked as duplicate.
This comment was marked as duplicate.
@Angelk90 Card layout is targeted for Phone-Portrait mode where thumbnails will be too small. Ideally to give better UX, the grid layout should have customizable column count. |
@Angelk90 Since you've already opened an issue, you don't need to repeat it here. |
Reverts part of TeamNewPipe#9310, which introduced bigger grid thumbnail sizes on big screens, because some users reported not being happy about having too few grid columns. See TeamNewPipe#9310 (comment) .
Rationale behind the feature:
When user go through videos, it's not possible to get enough info from title as content creators prefer to include details in thumbnails as such.
PR for #6087
What is it?
Description of the changes in your PR
This PR adds new List view mode called
Card
. Behavior follows,Which screens include the new layout?
Minor code improvement:
Currently we have flags for
gridLayout
,miniLayout
. Replaced the same with enum. Unified logic to identify the list mode resides in ThemeHelper.Minor improvement for w800 devices
#8412
Thumbnail size in grid is small for TV(and tablets). This PR includes a small tweak where size has been increased for w800 qualified devices. As opposed to seeing 6 small thumbnails in TV, user will see 3-4 of em now.
Before/After Screenshots/Screen Record
N/A Grid & List have smaller thumbnails
Uploaded in follow-up comment.
Fixes the following issue(s)
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