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

Support displaying bookmark state and watched progress in video manager #742

Merged
merged 7 commits into from
Feb 5, 2024

Conversation

hambre
Copy link
Contributor

@hambre hambre commented Apr 4, 2023

This adds support of displaying the bookmark state and the watched progress
within the video manager.
I have added preliminary support to display bookmark state and watched progress to the Monochrome theme
as seen in the image below:
Bookmark_and_WatchedProgress_in_VideoManager_annotated

Checklist

@rcrdnalor
Copy link
Contributor

The terminus 'Plugin' has a special semantic within MythTV.
See
https://www.mythtv.org/wiki/User_Manual:Plugins_and_Addons
https://www.mythtv.org/wiki/Category:Plugins
I see a 'MythMediaPlugin' in your proposed changeset.
IMHO, this change will confuse many of the developer and user.
If it is not a 'plugin' in the sense of MythTV, please consider a different naming.

@hambre
Copy link
Contributor Author

hambre commented Apr 5, 2023

The terminus 'Plugin' has a special semantic within MythTV. See https://www.mythtv.org/wiki/User_Manual:Plugins_and_Addons https://www.mythtv.org/wiki/Category:Plugins I see a 'MythMediaPlugin' in your proposed changeset. IMHO, this change will confuse many of the developer and user. If it is not a 'plugin' in the sense of MythTV, please consider a different naming.

Yes, no problem. I just followed the naming of the function to register the formerly used MediaCallback, which is called 'RegisterMediaPlugin'. I could rename 'MythMediaPlugin' to 'MythMediaHandler', but then the register function should be also renamed to avoid confusion.
I just wanted to first only do minimal changes to implement the feature and before getting feedback.

@hambre hambre force-pushed the hambre/video-bookmarks branch from 9e44f4f to 9e150e2 Compare April 9, 2023 16:28
@hambre hambre force-pushed the hambre/video-bookmarks branch from 62e21c7 to dbdbc67 Compare May 10, 2023 18:24
@hambre hambre changed the base branch from fixes/33 to master May 10, 2023 18:26
@hambre hambre marked this pull request as ready for review May 10, 2023 18:26
@hambre hambre closed this May 18, 2023
@hambre hambre reopened this May 18, 2023
@warpme
Copy link
Contributor

warpme commented Oct 5, 2023

hambre
Nice work!

One Q: is this code allows to have also watched progressbars on video lists entries?
(the same like in tv recordings)?
I'm asking as i'm not seeing progress bars on my recordings.
I applied hambre/Monochrome@d485478 and i' not sure what else i'm missing ?

@hambre hambre closed this Oct 6, 2023
@hambre hambre reopened this Oct 6, 2023
@hambre
Copy link
Contributor Author

hambre commented Oct 6, 2023

hambre Nice work!

One Q: is this code allows to have also watched progressbars on video lists entries? (the same like in tv recordings)? I'm asking as i'm not seeing progress bars on my recordings. I applied hambre/Monochrome@d485478 and i' not sure what else i'm missing ?

Hi Warpme, yes it allows progress bars for video list entries. If you look at the left red mark on the screenshot, you can see a green progressbar under the video title. However for that to work the video length entry within the videomarkup table needs to be present.

@warpme
Copy link
Contributor

warpme commented Oct 7, 2023

Ah - i missed green bar on screenshot :-)

Also - isn't be good idea also to add new field in filemarkup with total frames for movie?
With this - progress will be simple mark/total frames.
Maybe this can be fallback path - when user not run yet mythcommflag to rebuild seektable for a video file?
Benefit is that showing progress feature will be available also without any extra user actions (like rebuild seektable).

@hambre
Copy link
Contributor Author

hambre commented Oct 7, 2023

I think the total duration mark is given in frames. The problem is where to get this value from without relying on mythcommflag. I would love to add that, so I would not need a script to actually create this using mythcommflag. Any ideas?

@warpme
Copy link
Contributor

warpme commented Oct 7, 2023

Well - simple idea might be to do (or estimate) total_frames in mythplayer.
When user watching video and press for osd to see progress - there is progress shown on osd - so we have here required info.
So for example we can use this i.e. to estimate total_frames to store it filemarkup table ?

@warpme
Copy link
Contributor

warpme commented Oct 7, 2023

btw: i love your work with Monochrome!
imho: most beautiful theme with perfect form-beauty balance!

@hambre
Copy link
Contributor Author

hambre commented Oct 7, 2023

Hmm ok, I can have a look to come up with something. Thanks for the hint!

@hambre
Copy link
Contributor Author

hambre commented Oct 8, 2023

@warpme Thanks to your hint I added saving the total frame count for video files automatically without needing to use external tooling. You might want to check if that works for you.

@warpme
Copy link
Contributor

warpme commented Oct 8, 2023

@hambre
2f86ddb works perfectly :-)

One Q: isn't worth to have behavior of video progress as much as possible similar to recordings progress behavior?
I mean: user see progress bar on recording when: status is "not watched" & progress is > 0.

Also - i see small issue when user enters videolist: there is no progress-bar on list entries unless user gives active focus on list entry.
(This is different compared to progress bar behavior in recordings: entering recordings already shows progress bars on all recordings with status "not watched" & progress > 0)

@hambre
Copy link
Contributor Author

hambre commented Oct 14, 2023

@warpme
For me that is a little different. I typically watch recorded programs once. If I want to watch them again later I move them to the movies. So for movies which might be watched multiple times, I think having a progress bar not just for the first viewing is a feature I want to have.
For the progress bar on all items of the list, there is more work to do. That would need pulling all of the watched progress for all movies at once and caching that data to avoid too many queries to the database. So I did not do that first. I might look into that if time permits.

@warpme
Copy link
Contributor

warpme commented Oct 17, 2023

@hambre
Indeed - showing progress on all videos is "non-trivial" from perf. perspective.
I recall - for this feature in recordings area - David implemented threaded loading where top 10..20 rec are loaded in main thread and rest in backgroud thread (with slowdown to save cpu cycles & db bw).
Maybe it is worth to reuse this approach for video list progress bars?
At this might be non-trivial - indeed maybe it is worth to give this aspect lower priority...

But i still think idea that: user see progress bar on video only when: status is "not watched" & progress is > 0 is worth to do?

@warpme
Copy link
Contributor

warpme commented Jan 4, 2024

@hambre

You mention:

For me that is a little different. I typically watch recorded programs once. If I want to watch them again later I move them to the movies. So for movies which might be watched multiple times, I think having a progress bar not just for the first viewing is a feature I want to have.

I'm reading this as alternative use-case to current mythtv offers:

mythv:
"watched" means "is done" - so no progress make sense for something which is done. By this - when "watched" is ON, progress disappears (as not make sense anymore)

your needs:
Even is movie was already watched - you want to see progress of your's last watching - despite first watching is done

So ideally will be to add setting like: "always show watching progress" which functionally will be opposite to "show watching progress only for unwatched" (like in current mythtv).
Such setting should be mythtv wide (i mean it should work the same for recordings and for movies)

With this we can cover: current presentation model & your alternative preferred model....

What you think?

@hambre
Copy link
Contributor Author

hambre commented Jan 4, 2024

@warpme

Yes this might work to support the different use cases. I will look into it.

Extend ProgramInfo to load, delete and insert
MARK_TOTAL_FRAMES entries in filemarkup table.
This markup type can now be written by mythcommflag
when rebuilding the seektable for a video file.
This is in preparation to support displaying watched
progress within the video manager.
When exiting the media player, check if the current
video file alreadey has a total frame count saved
within the filemarkup table. If it is not yet present,
then save the total frame count from the player.
This way the watched progress does not need extra
tooling to provide the total frame count of a video file.
Query and cache playback state related data from
database so the watched progress can be shown for
multiple video list items.
Add support for displaying a bookmark state within
the video manager. This uses the playback state class
for caching the playback state for all videos.
Also adds support for displaying watched progress
state to video manager. This needs the last playing
position (already supported) as well as a total
frames markup datum within the filemarkup table.
The watched progress is then calculated and can be
displayed via a progress in the currently selected list
item.
Signal removal of last played position and bookmark
to video dialog, so it can update the respective
button list item.
When watching a video to the end, the player can mark
(if enabled by a setting) the video as watched. This
was not reflected in the GUI state until now.
So after receiving the playback stopped event, we load
the video metadata of the current video and sync it with
the metadata from the cache.
Adds a setting to always show watched progress even
if the recording or video is marked as watched.
Make behaviour of displaying the watched progress
the same in recordings and video lists.
@hambre hambre force-pushed the hambre/video-bookmarks branch from 2f86ddb to 21dd2aa Compare January 31, 2024 23:21
@hambre
Copy link
Contributor Author

hambre commented Jan 31, 2024

@warpme
I reworked this PR completely to have the changes more contained and to support displaying bookmark and watched progress for all visible items. Also added a setting to enable displaying the watched progress even when a video/recording is already marked as watched (default: off).

@warpme
Copy link
Contributor

warpme commented Feb 1, 2024

@hambre
Works perfectly!
Really nice work.
I hope it will be merged asap :-)

@hambre
Copy link
Contributor Author

hambre commented Feb 3, 2024

It seems not. String freeze for Myth 34 is already in effect.

@kmdewaal
Copy link
Contributor

kmdewaal commented Feb 3, 2024

It seems not. String freeze for Myth 34 is already in effect.

Can still be added to v34 as far as I know. It will be then only in English and not translated.

@hambre
Copy link
Contributor Author

hambre commented Feb 3, 2024

It seems not. String freeze for Myth 34 is already in effect.

Can still be added to v34 as far as I know. It will be then only in English and not translated.

Good to know. It still needs one of the developers to look at the PR and hopefully merge it.

@kmdewaal kmdewaal self-requested a review February 4, 2024 18:20
Copy link
Contributor

@kmdewaal kmdewaal left a comment

Choose a reason for hiding this comment

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

Looks OK to me. It does not appear to break anything. To make it useful there must be changes in the themes but I expect that then to be added later. There is one translatable string added but I expect at least the German translation can still be done before v34 is really frozen.
I am OK with merging this now.

@hambre
Copy link
Contributor Author

hambre commented Feb 4, 2024

Looks OK to me. It does not appear to break anything. To make it useful there must be changes in the themes but I expect that then to be added later. There is one translatable string added but I expect at least the German translation can still be done before v34 is really frozen. I am OK with merging this now.

Thanks for your approval! I can do the German translation for the new string as soon as this is merged, no problem. However I do not have the permissions to merge myself, so you or one of the other devs need to do that.

@kmdewaal kmdewaal merged commit 53d32e0 into MythTV:master Feb 5, 2024
8 checks passed
@hambre hambre deleted the hambre/video-bookmarks branch February 5, 2024 11:41
@hambre
Copy link
Contributor Author

hambre commented Feb 5, 2024

@kmdewaal Thanks for merging!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants