[pvr] fix: last watched timestamps won't update after channel switch #5400
Conversation
|
You should probably factor that out to a separate method, currently there's a lot of duplication going on. |
xhaggi
force-pushed
the
pvr-fix-last-watched
branch
from
89d81b9
to
01e5257
Compare
|
sure makes sense, but c/p was a little bit easier ;) |
|
@Jalle19 done in the fixup commit |
| * @brief Updates the last watched timestamps of the channel and group which are currently playing. | ||
| * @param channel The channel which is updated | ||
| */ | ||
| void UpdateLastWatched(CPVRChannel channel); |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
|
@Jalle19 done in the new fixup |
| channel->Persist(); | ||
|
|
||
| // update last watched timestamp for group | ||
| CPVRChannelGroupPtr group = g_PVRManager.GetPlayingGroup(channel->IsRadio()); |
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
xhaggi
force-pushed
the
pvr-fix-last-watched
branch
from
f44899a
to
2b5f062
Compare
|
squashed |
|
@Jalle19 same here, any objections? |
|
Seems fine. Did you check if there are other places where the new `UpdateLastWatched´ method could be used? I remember seeing that code in a lot of places when doing some refactoring but I think we decided to skip it for now then since you were rewriting the window code anyway and it would have made rebasing harder. |
|
okay, jenkins build this please |
As we only update the last watched timestamp of the current playing channel before a successful channel switch is performed we sometimes continue the wrong last watched channel and group.
xhaggi
force-pushed
the
pvr-fix-last-watched
branch
from
2b5f062
to
956a736
Compare
|
rebased. any objections? |
|
Correct me if I'm wrong but with this change we store last watch before switch, after switch and on close of stream. In contrast to before were we stored after watch and at stream close? |
|
hmm, i'm going to take a look at this before merging this in edit: as always, in the evening or week end (tomorrow :)) |
|
@topfs2 UpdateLastWatched is called in Open, Close and PerformChannelSwitch, which sounds about right to me. Open is called only once (the first time you start a channel), PerformChannelSwitch is called all the other times and Close is called only once you actually press Stop. |
|
right, this PR only change one part: on a channel switch it updates the last watch after the switch is performed for the channel you switched to, not for the channel you already watched. see line 1297 etc. |
|
next time i don't mix a fix with a refactor which makes things more difficult to review |
from a logical view, shouldn't last watched indicate the time you actually "watched" the channel and not switched to it? So let's say if I switch to channel A on 13:00 and have it running for like 2h and then switch to channel B, shouldn't last watched be 15:00 for channel A instead of 13:00? From your descriptions it's more a "last_tuned" then a "last_watched" edit: fixed double negation in grammar :) |
|
How would the program know that you actually "watched" it? The only thing that makes sense is that the last channel that was successfully tuned is the "last watched" channel. |
|
well, it's just the naming that's a bit misleading. Let's call it bikeshedding and ignore my comment :) I suppose we're doing the same on video playback (set lastWatched on playback start and not on playback end or both) |
|
When it comes to normal videos there's actually some fancy algorithm that determines when it is watched and not. I think it's something that >80% watched means watched, otherwise it is "in progress", unless it's less than say 10% watched, then it's "unwatched". I don't think we need something similar for live TV channels though. |
|
the only reason we need this timestamps are to continue the last watched channel/group on startup. totally different from what we do for movies |
|
@opdenkamp could this go in? |
|
It looks good to me, the changes in CloseStream looks like they could belong in another PR? Those are memleak fixes no? Anyways I want to approve this, it seems like a valid fix. @opdenkamp or @FernetMenta you have any time to check this? |
|
Yeah meant to check the open PRs last week end, but a failing hdd thought otherwise ;-) Got most things up an running again, will check in the evening. |
|
You can remove the approved flag if you find something unsetteling |
| @@ -1338,6 +1280,9 @@ bool CPVRManager::PerformChannelSwitch(const CPVRChannel &channel, bool bPreview | |||
| // set channel as selected item | |||
| CGUIWindowPVRBase::SetSelectedItemPath(channel.IsRadio(), channel.Path()); | |||
|
|
|||
| CPVRChannelPtr channelPtr(new CPVRChannel(channel)); | |||
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
xhaggi
force-pushed
the
pvr-fix-last-watched
branch
from
c35860e
to
b4f95ad
Compare
|
dropped the const from |
|
Looks good to me. Does this fix the bug where if you change the channel group in the channel list and quit XBMC, that group will forever be the default group on startup no matter what group you switch to later on? If not then let's get this in either way so we can work on fixing that. |
|
it definitely update the last watch of the selected group after the channel switch which is currently not the case. i think this could fix what you described |
|
@opdenkamp ping |
|
+1 |
[pvr] fix: last watched timestamps won't update after channel switch
As we only update the last watched timestamp of the current playing channel before a successful channel switch is performed we sometimes continue the wrong last watched channel and group.
@opdenkamp or @Jalle19 for a second look.