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

Add ability to mark an item as played #6773

Merged
merged 7 commits into from
Aug 1, 2021

Conversation

nschulzke
Copy link
Contributor

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

  • Add a method to HistoryRecordManager that adds a record to the history and sets a video to 100% watched
  • Add an item to the stream dialog labeled "Mark as played" that calls the above method

Before/After Screenshots/Screen Record

  • Before:
    Screenshot_1627354761
  • After:
    Screenshot_1627354961

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

@Stypox
Copy link
Member

Stypox commented Jul 27, 2021

Thank you! Have you tested this with the fast-loading feed (press on the (?))?

@AudricV AudricV added feature request Issue is related to a feature in the app feed Issue is related to the feed labels Jul 27, 2021
@Fs00
Copy link
Contributor

Fs00 commented Jul 27, 2021

That seems a nice addition! I was thinking that maybe "Mark as watched" (or viewed) would sound a bit clearer than played?

@nschulzke
Copy link
Contributor Author

Thank you! Have you tested this with the fast-loading feed (press on the (?))?

No, I didn't know about that feature! I probably won't get to it today, but I'll test it tomorrow.

That seems a nice addition! I was thinking that maybe "Mark as watched" (or viewed) would sound a bit clearer than played?

Yes, I'd agree. I chose "played" because that's the word that's already used in the "Show played items" button, which this is intended to synergize with. @Stypox, would it be okay if I changed both locations to "watched", or should we leave the wording as is?

@Stypox
Copy link
Member

Stypox commented Jul 27, 2021

Yes, change the wording

@nschulzke
Copy link
Contributor Author

Okay, I've changed the wording for both the Show Watched Items button text and the new dialog item.

I also tested it with the fast-loading feed for both YouTube and FramaTube, and it works the same as with the regular feed.

@Stypox
Copy link
Member

Stypox commented Jul 27, 2021

@nschulzke actually, the "Mark as watched" entry does nothing on videos loaded by the fast-feed mode that have never been opened. That happens since items loaded with the fast-feed mode do not come with a duration.

@nschulzke
Copy link
Contributor Author

@nschulzke actually, the "Mark as watched" entry does nothing on videos loaded by the fast-feed mode that have never been opened. That happens since items loaded with the fast-feed mode do not come with a duration.

I expected that behavior based on the help text, but that's not what I'm seeing in running it. I'm probably somehow accidentally caching the durations at some other point in the process.

I've also tried refreshing the feed while fast mode is on and then testing, it still works. It also works after wiping all the caches and history via the settings. Is there a way to force the app to drop cached duration data for testing?


Regardless, I pushed up a change that, when the duration is missing, loads it via ExtractorHelper. I tested it by first making it use ExtractorHelper all the time, verified that it works, and then I made it only happen if it doesn't have a duration.

@Stypox
Copy link
Member

Stypox commented Jul 28, 2021

Is there a way to force the app to drop cached duration data for testing?

Try to import an old database, then turn on fast mode and finally reload the feed. That's what I did. Otherwise turn on fast mode, subscribe to one of those channels putting out a new video every minute, wait a minute and reload the feed ;-)

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 to me, thank you! Before merging could you add some comments to the markAsWatched method?

@nschulzke
Copy link
Contributor Author

Code looks good to me, thank you! Before merging could you add some comments to the markAsWatched method?

Done.

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.

After testing a little bit it seems like your code does the correct thing when saving the stream state for items w/o duration, but the watched bar below the item never shows up (not even after reloading the app). All of this is solved as soon as the video is opened by tapping on it: the bar shows up correctly. Is the stream info loaded in markAsWatched saved into the known videos of the database?

@nschulzke
Copy link
Contributor Author

nschulzke commented Jul 30, 2021

Ah, yep, you're correct. Sorry for not testing it myself more thoroughly the first time! I was able to replicate your findings using the method you suggested (loading an old database). I pushed up a commit that switches the order of steps to ensure that the fetched data gets saved.

I've tested it now and confirmed that it works as expected in both fast and slow mode. If no duration has yet been loaded, it takes a second or two to fetch the duration. It then saves both the duration and the watched state to the database.

@nschulzke nschulzke requested a review from Stypox July 30, 2021 03:00
@nschulzke nschulzke requested a review from Stypox July 31, 2021 15:53
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.

Thank you! I tested again and it works perfectly, and code looks good :-)

@Stypox Stypox merged commit 551e8df into TeamNewPipe:dev Aug 1, 2021
This was referenced Aug 4, 2021
@Pentaphon
Copy link

Pentaphon commented Aug 26, 2021

@Stypox @nschulzke Can we extend this feature to ALL videos? It only works on "what's new". I would like it to work on channel pages as well.

@Stypox
Copy link
Member

Stypox commented Aug 26, 2021

Upgrade to 0.21.9 @Pentaphon

@Pentaphon
Copy link

Pentaphon commented Aug 26, 2021

Upgrade to 0.21.9 @Pentaphon

I'm using that and "mark as watched" only shows up in my "what's new" results. I would like it to show up on all results, whether they are search results, channel pages, etc. Just because I am not subscribed to a channel doesn't mean I shouldn't be able to "mark as watched" videos from that channel. Sometimes, I just need to watch a few videos from a channel and NOT have to subscribe to it, like for example, videos on "how to" do something that I just need to watch once.

@Stypox
Copy link
Member

Stypox commented Aug 30, 2021

Ok, I see. It should be simple, if you want to try to implement it @nschulzke

@nschulzke
Copy link
Contributor Author

nschulzke commented Aug 30, 2021 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issue is related to a feature in the app feed Issue is related to the feed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature request] manually mark videos as watched
5 participants