Skip to content

Conversation

@Stypox
Copy link
Member

@Stypox Stypox commented Jul 1, 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

#8336 did not really fix the problem, it just made so that each item in history has 1 less view. This meant that, even though the bug appeared to be fixed the first time a video is opened (since that video's views are correctly set to 1), the second time the same video is opened the views are incorrectly set to 3. And #8336 actually introduced a regression, since e.g. enqueued videos were registered as "No views" as reported in #8336 (comment).

The underlying problem was an incorrect usage of Java's == (same) operator: it was used in the player's onEvents function to check if the media item tag in the ExoPlayer was equal to the one currently set in our Player. The problem with this is, when the player is started from scratch, the media item tag is first updated with a tag with no LoadedMediaSource attached (since the player is still loading the media source), and then updated again when the media source was loaded. The second update created a new instance of the media item tag (see StreamInfoTag.withExtras() and its usage in LoadedMediaSource and FailedMediaSource), so the == operator is not suitable for comparing them. When the player is not started from scratch, but rather just switches to the next item in the queue, only 1 view was (correctly) registered, since the media source is usually already loaded because of the mechanism of pre-fetching the next item in the queue.

This PR solves the issue by comparing the stream info url associated with the media item tag before calling updateMetadataWith() with the new stream info.

This PR also makes it so that marking a video as watched does not add one view to it, and also does not update its last access time. Now a new history entry with 0 views is added, and this happens only if one did not exist before. Tell me if I should revert this.

Fixes the following issue(s)

APK testing

app-debug.zip

Due diligence

@Stypox Stypox added bug Issue is related to a bug ASAP Issue needs to be fixed as soon as possible player Issues related to any player (main, popup and background) labels Jul 1, 2022
@litetex litetex self-requested a review July 2, 2022 18:32
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

Copy link
Member

@AudricV AudricV left a comment

Choose a reason for hiding this comment

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

I didn't really reviewed the whole code but found an optimization in it that you may use.

I trust @litetex for the rest of the code :)

Comment on lines +2490 to +2491
final StreamInfo previousInfo = Optional.ofNullable(currentMetadata)
.flatMap(MediaItemTag::getMaybeStreamInfo).orElse(null);
Copy link
Member

Choose a reason for hiding this comment

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

You can use getCurrentStreamInfo() method to do so, unless you want to avoid confusion between the previous info and the current one :)

Suggested change
final StreamInfo previousInfo = Optional.ofNullable(currentMetadata)
.flatMap(MediaItemTag::getMaybeStreamInfo).orElse(null);
final StreamInfo previousInfo = getCurrentStreamInfo().orElse(null);

Reminder of getCurrentStreamInfo() method
    private Optional<StreamInfo> getCurrentStreamInfo() {
        return Optional.ofNullable(currentMetadata).flatMap(MediaItemTag::getMaybeStreamInfo);
    }

Copy link
Member Author

Choose a reason for hiding this comment

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

Even tough to my eye your suggestion looks better, I'd prefer to keep the current code as it shows where the stream info comes from

@Stypox Stypox merged commit 1608915 into TeamNewPipe:release-0.23.1 Jul 4, 2022
@Stypox Stypox deleted the fix-view-count branch August 4, 2022 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ASAP Issue needs to be fixed as soon as possible bug Issue is related to a bug player Issues related to any player (main, popup and background)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants