[Approved] Show Comskip Markers in Timeline (1 Viewer)

[offbyone]

Hi Matt,
we found some time to review your patch, here are our findings:

1. in TVHome.cs you iterate the Chapters and JumpPoints of the g_Player - both might be null, so it might be wise to check that as well
2. the changes to GUITVProgressControl.cs are in conflict with the changes that were done due to a different patch - can you update your patch/branch to include those changes so the merge can be done without conflicts
3. arion_p pointer to another possible issue, i'd like to quote him here:
   

   On a quick code review there is one potential issue. While it is designed to only handle static markers (the number of markers cannot change once the progress control is initialized) it rereads and reparses the markers' positons on every render. This has 2 pitfalls: 1s is speed, the 2nd is the number of markers may be different than the number initially allocated causing an "index out of bounds" exception in rendering. Once this happens rendering will most probably stall.
   Edit: I fact I think it would be possible to use a single GUIAnimation control instead of an array, which would make it able to handle dynamic changes to markers.

 

[mattjcurry]

Hey Offbyone,

Thanks for the feedback, here are my responses:
1.) I agree and am happy to make that edit.
2.) Is this patch merged into master? If not, can you please tell me which branch it is in so that I can sync up the changes? Otherwise, please let me know if there is an easier way.
3.) I completely agree with this statement. In fact I had mentioned earlier that I thought that all the static components of the progress bar should only be rendered once (background, left, right, background texture, etc...). I wanted to refactor all of that, but was worried that it would create conflicts with other branches. I will break out the processing of the parsing of the positions for now and only call it once.

The one thing I find really interesting here is the edit. I think what arion_p is saying is that you would go through the loop of determining the positions and just call render after each change in the position, thus rendering the same element multiple times. The question I would have is, what if I want to move the determination of the positions out to another method that only gets called once. Would it be cleaner and have better performance if there were multiple instances in a list, or would it be better to re-determine each position every time. The thought is that if I leave it the way it is, then I can run the logic that determines the position one time and then just call render on each item in the array during the render method since its position has already been determined. It seems like that would be lower overhead than determining the position everytime, which I only did to maintain consistency with how the logic was already working of other GUIAnimation objects on the control, but would be required to use a single GUIAnimation control multiple times. I suppose the alternative would be to store the positions and widths of the elements in an object(s) and cache it once it is read in. Then when you call render, you have the one instance and you just read each position and length from the cached object.

My final question is, since this is my first contribution, I was curious...Should I be still working off my fork and issue another pull request, or can I checkin changes to the branch you created?

Thanks again for your support in getting this feature into the codebase. Much respect to you and the rest of the MediaPortal devs.

Thanks,
Matt

 

[Sebastiii]

Hey,
I surely not answer all your question, but you can work on 1.3.0alpha branch to make the new path because there is change in it so better to work on latest code and submit your patch here.

I let other to post a better answer
Thanks.

 

[offbyone]

The branch's name is : 1.3.0Alpha_testing. This will get merged to master, we simply merged all branches that will make up 1.3 alpha into a new single branch to build a test installer. AFAIK, you cannot push changes to the (https://github.com/MediaPortal/MediaPortal-1/tree/FEAT-3853-Show_Comskip_Markers_in_Timeline) branch due to missing permissions (yet ). But I'd say yes use that branch to commit your changes.
For answers to question 3 I will point arion_p to this thread, maybe he can do a quick comment.

 

[mattjcurry]

Hey offbyone,

Thanks for the info. I will make the edits and then push them up to my repo.

I was hoping that since I am a dev team member now that I would have github access to push directly to that branch, but that does not appear to be the case yet. I will just work like this for now.

Thanks,
Matt

 

[offbyone]

Hi and welcome to the team!
You should actually have those rights - maybe contact high or infinite.loop and ask to set the permissions for git.

 

[mattjcurry]

Hey guys,

I went ahead and implemented suggestions #1 and #3.

As for the conflict with another feature, that is something that I have to tackle either tomorrow or after the weekend. I talked to Arion_p about it and I know what I need to do, it is just a matter of getting it done.

Thanks,
Matt

 

[mattjcurry]

I completed the merge of BUG_3849_GUITVProgressControl_scaling into my feature, which allows for a clean merge into 1.3.0Alpha_testing.

All requested updates have been made. Please code reivew.

Thanks,
Matt

 

[Holzi]

So will this be merged?

 

[offbyone]

Well the deadline for Alpha is over if I'm correct? And as it is a new feature it would bot be allowed for a beta. But maybe we can make an exception, as we reviewed it already earlier and all changes are actually now done prior to an Alpha actual release. Unfortunately I think I can't make that decision on my own - need approval of a MP1 manager like arion or chemelli. From my point: I'd like to see it in 1.3 Alpha.