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 ListItemInfos: TimePlayed and IsResuming #5397

Closed

Conversation

relictMarauder
Copy link

Adding ListItem getter for retrieving resume time with carring of choise in resume dialog

@MartijnKaijser
Copy link
Member

please fix indenting (we use 2 spaces)

@relictMarauder
Copy link
Author

Done. Sorry

@xhaggi
Copy link
Member

xhaggi commented Sep 22, 2014

and squash it to one commit please

@FernetMenta
Copy link
Contributor

apart from the coding style which is still wrong (curly bracket starts next line) do you think it is a good idea to open/close a database each time gui wants to render this info?

@BigNoid
Copy link
Member

BigNoid commented Sep 22, 2014

is there another resume time as the current one?

@relictMarauder
Copy link
Author

@FernetMenta: I will fix the coding style, thank for your's feedback. About close of DB. To my regret i can't for this request i need last state of resume time from db. The corresponding value from VideoTag resume point is not actual, after user make choise in Resume Dialog
@BigNoid : all other resume times(m_resumeTime from videotag) has not actual value, a specially if user seleced resuming from Resume Dialog
The ResumePersent from listitem LabelInfo can't be used while:

  1. No actual (specially after using Resume Dialog)
  2. I need the resume time a not the percents, the total time is not readable too so i can't figure it out

@FernetMenta
Copy link
Contributor

If this variable differs in different parts of the code, this should be fixed. I don't think CVideoDatabase has a write through cache, right?

@BigNoid
Copy link
Member

BigNoid commented Sep 22, 2014

I meant that it should probably be named resumepoint instead of currentresumepoint as there are no other resume points. Or better yet resumetime.

@relictMarauder
Copy link
Author

@BigNoid: It name is in my patch is already InfoLabel("ListItem.CurrentResumeTime"). "Current" while info will be loaded direct from DB and don't use cached value
@FernetMenta:i have found only variable "precentplayed" ,i don't know exact mean of this info, but InfoLabel("ListItem.ResumePercent") return the resume presents without carring out of result from resume dialog.

@BigNoid
Copy link
Member

BigNoid commented Sep 23, 2014

If there's going to be a resume time infolabel it should have the same value as percentplayed presented in hh:mm:ss and should just be called listitem.resumetime. To have 2 infolabels that reflect the same but could have different values is not the way to go. So imo either use m_resumePoint value in hh:mm:ss or do it this way and make percentplayed use the same routine.

@relictMarauder
Copy link
Author

@BigNoid : the, value of m_resumePoint in video tag, has olways value of last resume point, but if user select start from beginning in resume dialog, i can't figure it only by value in resume point, The info wehether player should useresume time or not, stored in m_lStartOffset property if CFileItem.
Secondary, the value in m_resume_point is not olways equal this info in db.

@relictMarauder relictMarauder changed the title add listItem info: CurrentResumeTime add listItemInfos: TimePlayed and IsResuming Sep 25, 2014
@relictMarauder relictMarauder changed the title add listItemInfos: TimePlayed and IsResuming add ListItemInfos: TimePlayed and IsResuming Sep 25, 2014
@BigNoid
Copy link
Member

BigNoid commented Sep 26, 2014

Sounds good to me, thx.

@FernetMenta
Copy link
Contributor

jenkins build this please

@relictMarauder
Copy link
Author

@FernetMenta: please jenkins build one more times, the previous build was brocken(timeout by checkout)

@fritsch
Copy link
Member

fritsch commented Sep 28, 2014

jenkins build this please

@relictMarauder
Copy link
Author

For this pull i have the update: Meking neu Info isResuming avilable for Python addons

@BigNoid
Copy link
Member

BigNoid commented Sep 30, 2014

I'm going to need some time to look into this.

@topfs2
Copy link
Contributor

topfs2 commented Oct 2, 2014

I don't understand what isResuming refers to? With that naming I assume it means that the item is currently resuming, which sounds like a really unuseful info.

The timeplayed however seems valid, perhaps a timeprogressed also? @BigNoid useful?

@relictMarauder
Copy link
Author

The info about current resuming state of list item is usefull for plugins, i can't find other posibility to get info about user choise in resume dialog.

@BigNoid
Copy link
Member

BigNoid commented Oct 4, 2014

So ListItem.isResuming returns true when the resume dialog is active? You want to use that bool in a plugin to check if that dialog is active? There are other ways to do that, no need for two booleans which imply the same but have different meaning.
TimePlayed is useful information and that should go in IF it reflects percentplayed in a hh:mm:ss format.
@topfs2 I dont see how timeprogressed would be different from the already existing percentplayed

@Hitcher
Copy link
Contributor

Hitcher commented Oct 4, 2014

I think he means ListItem.IsResumable so scripts can find videos that are in progress but then they could just as easily use ListItem.PercentPlayed > 0.

@relictMarauder
Copy link
Author

I mean, if user choose in ResumeDialog that the video must be resumed, the ListItem.isResuming has true, otherwise false. This info i can't retrive from others ListItem properties, PresentPlayed and TimePlayed store info about last played position and not about user choose for resuming

@topfs2
Copy link
Contributor

topfs2 commented Oct 4, 2014

Could you give a case were that is relevent information? I cant come up
with any reason why skin would want to showcase that?

@BigNoid ah. Wasnt aware it already existed!
On 4 Oct 2014 10:22, "relictMarauder" notifications@github.com wrote:

I mean, if user choose in ResumeDialog that the video must be resumed, the
ListItem.isResuming has true, otherwise false. This info i can't retrive
from others ListItem properties, PresentPlayed and TimePlayed store info
about last played position and not about user choose for resuming


Reply to this email directly or view it on GitHub
#5397 (comment).

@Hitcher
Copy link
Contributor

Hitcher commented Oct 4, 2014

So if the user selects to resume a video it gets marked as IsResuming but then they'll probably watch it to the end and it will still be marked as IsResuming. Confused!

@MartijnKaijser
Copy link
Member

IsResuming seems not needed imo.

@topfs2
Copy link
Contributor

topfs2 commented Oct 4, 2014

@relictMarauder could you please split up the different additions to two PRs (so that we can discuss them seperately as the TimePlayed is very likely to be accepted while the other needs further discussion)

@relictMarauder
Copy link
Author

I have a this use case:

  1. the plugin create the ListItem with internet stream
  2. User start watching and stop them
  3. The time of resuming stored in db
  4. The user start this stream one more time
  5. The Resume dialog is showed
    6 User choose the resuming of the stream at ...time
  6. Plugin retrieve user action to play the item and must to know about user choise in resuming dialog.
    The TimePlayed and PercentPlayed properties contains the info about resuming time, but not about user choise in Resume Dialog, this info can plugin retriving from isResuming property .
    In my usecase the TimePlayed without isResuming has no sence.

@topfs2
Copy link
Contributor

topfs2 commented Oct 4, 2014

A resume from the VFS point of view is open followed by seeks. The player might not even decide to start the resume at precisely the time specified but rather the closest keyframe. So it seems very odd to me that the plugin should care when its seeked to.

If the plugin needs to know this it should be given the data on open and through that API and not through the listitem API (which is the viewmodel and the plugins way to talk to the skin)

@topfs2
Copy link
Contributor

topfs2 commented Oct 4, 2014

And consider if we add a "skip back X s from resume to catchup", then the plugin would be broken until it reacted on that setting too. Which feels like a terrible API :)

@relictMarauder
Copy link
Author

I have a use case with stream that a not seekable, but server can provide another streem url for other time position. So if plugin can recognize that start position is not begin, than he can retrieve from server new url. If player try to seek such stream the plugin can reacting on that via player callbacks(VirtualSeeking)

@topfs2
Copy link
Contributor

topfs2 commented Oct 5, 2014

That is a very valid usecase. However it still feels to me that the xbmcplugin is the place to extend instead of listitem interface, mostly because it is much more future proof. I'll see if I can hunt down some people with the plugin know how and see if we can find a good way forward.

Please split out the TimePlayed into a seperate PR though.

@topfs2
Copy link
Contributor

topfs2 commented Oct 5, 2014

Basically I think that https://github.com/xbmc/xbmc/blob/master/xbmc/windows/GUIMediaWindow.cpp#L1008 should send along the startoffset to the plugin (or perhaps even the entire fileitem somehow), so it can resolve the URL differently depending on if its resume or not.

@MartijnKaijser
Copy link
Member

Closing this. Please resubmit for comments when it's rebased

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.

None yet

8 participants