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

[PVR][Estuary] Add support for ACTION_(PREV|NEXT)_ITEM for timeshift buffer navigation #14387

Merged
merged 5 commits into from Sep 5, 2018

Conversation

@ksooo
Copy link
Member

commented Sep 3, 2018

Found some time to add another missing feature to PVR OSD: prev and next button (working similar to music player implementation) to navigate timeshift buffer / the epg events in the buffer. Ofc, timeshift must be supported by the respective pvr/inputstream addons. Otherwise theses buttons will not be available.

screenshot001

Runtime-tested on macOS, latest Kodi master.

@Jalle19 might want to take a look at the code.
@Paxxi are you fine with the appmessenger changes?
@ronie are the skin changes okay?

@ksooo ksooo added this to the Leia 18.0-beta2 milestone Sep 3, 2018

@ksooo ksooo requested review from ronie, Paxxi and Jalle19 Sep 3, 2018

@ksooo ksooo force-pushed the ksooo:pvr-action-prev-next branch from 80fb5aa to 5d90ba5 Sep 3, 2018

@ksooo ksooo force-pushed the ksooo:pvr-action-prev-next branch from 5d90ba5 to 1d25538 Sep 3, 2018

@@ -76,7 +76,7 @@
<param name="texture" value="osd/fullscreen/buttons/next.png"/>
</include>
<onclick>PlayerControl(Next)</onclick>
<visible>MusicPlayer.HasNext + !MusicPlayer.Content(livetv)</visible>
<visible>Player.SeekEnabled | MusicPlayer.HasNext</visible>

This comment has been minimized.

Copy link
@ronie

ronie Sep 3, 2018

Member

during music playback, the next button will now be visible even if there is no next song to play.

This comment has been minimized.

Copy link
@ksooo

ksooo Sep 4, 2018

Author Member

Right. I will fix that.

@@ -81,7 +81,7 @@
<param name="texture" value="osd/fullscreen/buttons/next.png"/>
</include>
<onclick>PlayerControl(Next)</onclick>
<visible>!VideoPlayer.Content(livetv) + [Player.ChapterCount | Integer.IsGreater(Playlist.Length(video),1)]</visible>
<visible>Player.SeekEnabled | Player.ChapterCount | Integer.IsGreater(Playlist.Length(video),1)</visible>

This comment has been minimized.

Copy link
@ronie

ronie Sep 3, 2018

Member

similar issue here.

This comment has been minimized.

Copy link
@ksooo

ksooo Sep 4, 2018

Author Member

Yep, I will fix that.

@Paxxi

This comment has been minimized.

Copy link
Member

commented Sep 4, 2018

The param1/2 for appmessenger methods are mostly for backwards compat and guimessages where they're part of the routing for a message.
In general anything message specific should go into the lpvoid parameter. I brought over the string and fileitem parameters for backwards compat and I regret that now. Nothing in appmessenger cares what's in there so it's up to the sender and receiver to agree on a contract. I've gone for raw pointers where the receiver is responsible for deleting them.
I'm not against adding more parameters as such but I think we should try to keep them low to minimize complexity.
Having an int64_t is general enough that it could be useful for other messages in the future so I think it's fine to add it.
Can the parameter be added to all the methods without too much work to keep them consistent?
I noticed the comment said nonblocking, sendmessage will block and wait for a response, postmessage is nonblocking fire and forget.

In closing. Adding the parameter is fine but try to add it to all the relevant overloads to keep things consistent.

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Sep 4, 2018

In the first place I actually used lpvoid, but thought it is not guaranteed that the message gets actually delivered or delivered to only one receiver. Memory leaks or multi frees could be the result, no?

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Sep 4, 2018

If we stick with the new parameter I would like not to extend the other methods now, because atm nobody needs this. If it gets needed we can extend stuff on demand. But you have the last saying on this.

@Paxxi

This comment has been minimized.

Copy link
Member

commented Sep 4, 2018

There's currently no support for delivery to multiple recipients so that's not an issue.
Failed delivery should only happen in three scenarios, possibly during startup/shutdown or some catastrophic failure as runtime removal of receivers isn't implemented yet.

OK, but can you at least add a matching postmsg method so they're consistent that way?

}
else if (Size() > 0)
{
/* return the first event that is in the past */

This comment has been minimized.

Copy link
@Jalle19

Jalle19 Sep 4, 2018

Member

Why can't this code path be used all the time?

This comment has been minimized.

Copy link
@ksooo

ksooo Sep 4, 2018

Author Member

I mainly borrowed the implementation from CPVREpg::GetTagNext(). I guess it's for performance reasons? First code path is faster, because no sequential seek involved.

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Sep 4, 2018

Not sure what you mean with the missing postmessage method. I added a new postmessage method already, which internally calls sendmessage with last parameter - named wait - set to false. I thought that would do a "post".

@Paxxi

This comment has been minimized.

Copy link
Member

commented Sep 4, 2018

Sorry. I'm on my phone so I guess I missed it.
Carry on then 😀

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Sep 4, 2018

Sorry. I'm on my phone so I guess I missed it.
Carry on then 😀

Will do, thanks.

@ksooo ksooo force-pushed the ksooo:pvr-action-prev-next branch from 1d25538 to 0f50b14 Sep 4, 2018

@ksooo

This comment has been minimized.

Copy link
Member Author

commented Sep 4, 2018

@ronie I updated the skin changes. Good to go now?

@ksooo ksooo force-pushed the ksooo:pvr-action-prev-next branch from 0f50b14 to f4aecb8 Sep 4, 2018

@ronie
ronie approved these changes Sep 4, 2018
Copy link
Member

left a comment

yup, go ahead

@ksooo ksooo force-pushed the ksooo:pvr-action-prev-next branch 2 times, most recently from 80253c1 to c3b5d30 Sep 4, 2018

@ksooo ksooo force-pushed the ksooo:pvr-action-prev-next branch from c3b5d30 to f1bb4ac Sep 4, 2018

@ksooo ksooo merged commit b04ed6f into xbmc:master Sep 5, 2018

1 check passed

default You're awesome. Have a cookie
Details

@ksooo ksooo deleted the ksooo:pvr-action-prev-next branch Sep 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.