Local Playlist and Database Normalization #1004
Conversation
|
Nice work! Awesome PR. Waiting until it gets merged with your future improvements. Are the timestamps in git log real? I mean, did you really write all this code in 3-4 days? Anyway here is what i think about improvements:
Also app crashes with this: |
|
Awsome PR !!! |
|
Thanks guys. @avently It actually took me a week to get the first commit in since I needed to think through the design. But the rest are just CRUD work and UI, which I mostly just piggybacked on the existing InfoItem UIs. Funny though, I thought about the same when you were able to merge and refactor the popup player so quickly =D
Yep, updated the todos.
These are probably stretch goals though, since I want to minimize the impact on VideoDetailFragment before your PR is merged.
I've thought about adding an option to add to playlist on every stream info item as well, but in #810 there was an agreement that the dialog controls do not include functionality that is easily accessible after the fragment is opened. @theScrabi what do you think about this? Exception should be fixed, thanks again.
Yep, currently each playlist just keep track of the items on the streams table, which contains of the service id. In the future, we can even do queries to filter playlist by service ids, if someone fancies that. |
In some places of the world an internet connection is slow or costs much money. And if user searches, for example, he maybe want to watch videos later from playlist/bookmarks. That's a use case. There is an option in settings which named autoplay. So every load of fragment will trigger video downloading. Also landscape orientation will trigger to do it too. |
Not arguing with you here, I want to be able to do that as well, especially this feature is available on the official Youtube app. I'll will add this at the last stages when the PR is feature complete and we are just working on the UI. The code to do this is already there, we just need to add an item to the dialog.
I think this would better be done under a separate PR after yours is merged though, since this is a low hanging fruit on UI upgrade and is not really related to the core functions of both your PR and mine. |
|
I'm also about to write my exams, so I just took a quick look and it looks great!
|
|
Haha, yeah =P The UI are a mess right now as I'm just aiming for feature completion at the moment. The icon for add to doesn't even line up with the icons to its right. Please feel free to suggest a new UI design and bookmark icon though, I'll implement it towards the end of the PR. |
|
I tought about this problem. Maybe we could add a dropdown menu when there is not enogh space, but Im not yet convinced of that idea. |
karyogamy
force-pushed
the
local_playlist
branch
from
0312344
to
2a6a5f6
Compare
|
@theScrabi What about swapping the line for uploader with the addTo+background+popup buttons. This way we can pull another button from the top and make a row of 4 buttons similar to the Youtube app. |
|
Yea I think it was good. |
|
In Android Studio you could just add one of the Material Design Icons and
I'd suggest the 'bookmark' icon or maybe the 'star' icon.
|
|
Yea I'd prefer the bookmark icon. |
karyogamy
force-pushed
the
local_playlist
branch
from
0cc4d58
to
376c4f7
Compare
|
@karyogamy as a suggestion for the future. If user opens a playlist (not just single video) from a channel or a search the "plus" button in the fragment should save the whole playlist as a local playlist. Maybe with a switch in the UI. Something like that: |
|
I was thinking about that too, though I'm still trying to choose between saving the immediate partial playlist or downloading the full playlist (which might be a problem when the whole thing has few thousand videos) before saving. |
karyogamy
force-pushed
the
local_playlist
branch
from
26aea7b
to
40b7d74
Compare
karyogamy
force-pushed
the
local_playlist
branch
2 times, most recently
from
86b52c6
to
35e7b10
Compare
|
Local playlist is now feature complete and below are some updated screenshots:
Some things to note:
Some cleaning up and bug hunting have already been done but there are likely some areas I have missed. So please feel free to take a look at the code, provide suggestions or ask questions. |
|
Thank you so much for this amazing work!! |
|
I am very anxious to see this work in newpipe! many thanks!!! later next could add a fourth button, along with add to the playlist, background and popup, the button to see the comments |
|
Great work! |
|
I agree, the old history panel should really be part of the fragments rather than a new management activity. The problem is that the new history uses aggregate entries and the old history shows entries like a broswer does (finer control over which entries to delete), and a switch would show two different list on the same fragment. IMHO, this should probably be done in another PR to simplify history, since it can easily introduce bugs and other unexpected behaviors. However, deleting aggregate entries straight from the new history fragment can be easily done, as the same db queries are already used by the old history management. This is not what I intend how the aggregate history to be used, but such feature should be easy to add in this PR. What do you think? |
|
@karyogamy what about an option which the user can "save" an online playlist (just url, name and thumbnail), similar to what YouTube does? I think this option would be better suited in the place of the current "Add to playlist". It would be extremely useful for me, as I have some musics playlist which would be saved and it'd be in-sync when I want to play it (I imagine this is a very common use case). So the current alternatives to this option ("Add to playlist"):
Also, I'll try to see this PR more in-depth soon. |
|
The first approach might be more feasible. We can add another table which only keeps remote playlists. The only problem I see is that the video count or thumbnails will be outdated until every time the user accesses the playlist. On such remote playlist, the user would open playlist fragment where everything is unmodifiable, including the name, thumbnail and video list. My problem with the second approach (adding all to playlist) is how we deal with the UI. Currently, you can manually load the playlist to its bottom and save it as a local playlist. Since the video list can be very long, meaning that when saving the playlist, we need to send out a bunch of request in sequence before we can save it. During this time, we either need a blocking UI, which is discouraged. Or we can either keep the request going in the background, which may lag the app and potentially the database. Or we kill the request when user switches out, which would also be undesirable. Let me know what you think about the options. Right now I'm trying to get a proof of concept out for subtitles and aspect ratio support on the video player, and will get back to this soon. |
karyogamy
force-pushed
the
local_playlist
branch
from
e6ada0f
to
159bcce
Compare
|
Hi. Thank you for your work! By the way, it throws NPE on the latest commit(actually on some recent commits), only when I try to open this video. Other videos seem working right except for this video. [Edit] It happens when playing videos of the specific channel MBCkpop. Is it because of a regional restriction problem in getting available subtitle information?
|
|
Yikes, didn't even notice this one. Fixed, thanks. |
-Fixed local playlist header margins.
|
I've decided to drop the dependency on LeakCanary in this PR, since it is hogging the app way too much. I'll create another PR to solely introduce LeakCanary as well as adding leak detection of fragments to it. |
…ist fragment is created by external share.
|
@karyogamy I don't think having two menu items is a good idea, because is inefficient and as you may have noticed, there's a blinking when the buttons are repositioning (appearing/disappearing). Instead, use the private void updateBookmarkButtonsVisibility() {
if (playlistBookmarkButton == null) return;
int iconAttr = playlistEntity == null ? R.attr.ic_playlist_add : R.attr.ic_playlist_check;
int titleRes = playlistEntity == null ? R.string.bookmark_playlist : R.string.unbookmark_playlist;
playlistBookmarkButton.setIcon(ThemeHelper.resolveResourceIdFromAttr(activity, iconAttr));
playlistBookmarkButton.setTitle(titleRes);
} |
| /** Determines if the fragment is part of the main fragment view pager. | ||
| * If so, then this method must be overriden to return true | ||
| * in order to show the hamburger menu. */ | ||
| protected boolean isPartOfFrontPager() { |
There was a problem hiding this comment.
I think it's better to just drop this function and this call to setDisplayHomeAsUpEnabled, and just let the already implemented function in the activity handle it.
This check should be used for other things such as: don't inflate menus or change the toolbar too much.
-Fixed bookmarking disposable not part of playlist fragment lifecycle. -Rearranged local fragment directory structure.
-Fixed bookmark button flickering on visibility toggling. -Removed toolbar up button control from local fragments, delegating functionality back to main fragment. -Updated extractor to latest.
Totally forgot about the new
Ahh, I see what you saying. Fixed/removed. |
…rom search page. -Fixed NPE when playlist fragment is destroyed while renaming. -Fixed remote playlist thumbnail to use uploader avatar when thumbnail url is unavailable. -Added dispose on exit to all database requests in local playlist fragment.
|
Found a quirk in the Soundcloud playlist extractor. Where the url for playlist (info item) in search result uses This discrepency initially causes the bookmark button to stop working properly on Soundcloud when the playlist fragment is opened from a search page, but is now fixed. However, this makes me think that we should store links in the database using URN (excluding the http and https protocols) rather than URL. What do you guys think about this, please let me know. |
|
I just saw, in the new menu where we put popup playlist, etc we could also put the download icon into. Like the real yt app. |
|
Can you clarify which popup playlist you are referring to? I don't have Youtube Red so no download button anywhere here. Edit: Though I think playlist downloading would probably be best if done in another PR, after we introduce a better downloader. |
|
Basicly I mean this: |
Yea why not. Maybe we could embed a method somewhere that uprise the protocol from http to https. |
Done.
Sorry, I didn't think this through yesterday and have just realized that it is possible for future streaming services to be |
|
Yikes, few seconds too late =P |
I did do it as well now xD |
|
@theScrabi what this line do? With it openPopupPlayer and openBackgroundPlayer called 4 times. Without it everything is fine |
|
@karyogamy it is possible to add the same video into one playlist multiple times. P.S. I merged your changes. Let me know if I broke something. And thanks for this PR |
|
@avently you are right, i fixed the line. |
Yes, though I haven't added the ability to see duplicate entries when the add dialog is open (which can get annoying), this functionality should be fairly easy to implement now if anyone wants to give it a shot =D |
ghost
mentioned this pull request
IMPORTANT: Please test this on an emulator since this PR includes a database migration script (that normalizes the watch history table) which is currently irreversible.
Here are some screenshots, which is probably better at explaining the new features. These UI are not final, please feel free provide ideas:
Here's an overview of the new database schemas:
Todos (not in order):
Future PR:
Please feel free to give recommendations and report bugs, though please don't test this on a real device yet.