search

kodi-pvr / pvr.nextpvr

Kodi's NextPVR client addon
GNU General Public License v2.0
22 stars 33 forks source link

Matrix 4.4 feature enhancements #121

Closed emveepee closed 4 years ago

emveepee commented 4 years ago

This PR will include multiple commits for a new 4.4 release of the addon. New Matrix settings, new startup logic, new options & features implementing improvements on the backend.

emveepee commented 4 years ago

I'd like to get this out before the next API change, it is pain to build new versions of kodi on windows and ubuntu to test and resolve. Are there any additional comments or concerns?

I do have an unrelated minor bug fix (problem is in Leia too) but should I add it here or in a new PR?

phunkyfish commented 4 years ago

I will review tomorrow, please add the bug too, ping me once it ready for review.

emveepee commented 4 years ago

@phunkyfish thank you for the review I didn't merge it with code as it impacted too many individual commits. I hope that is acceptable and makes it easier to review the resolved issues.

phunkyfish commented 4 years ago

A fix up commit like you have done is ok. Give me some time to review.

emveepee commented 4 years ago

I am running into crashing problems with the Code Review commit I think I have a bad pointer with the move to String::Format for artworkPath.

Also @phunkyfish what is the purpose of GetRecordingSize() polling when no recording is in progress? I have been simply returning PVR_ERROR_NOT_IMPLEMENTED and this wasn't a problem but I feel that Kodi might not be liking it when pCapabilities->bSupportsRecordingSize is true.

phunkyfish commented 4 years ago

Where exactly does it crash?

Have you run it through a debugger?

On 27 Apr 2020, at 23:10, emveepee notifications@github.com wrote:

 I am running into crashing problems with the Code Review commit I think I have a bad pointer with the move to String::Format for artworkPath.

Also @phunkyfish what is the purpose of GetRecordingSize() polling when no recording is in progress? I have been simply returning PVR_ERROR_NOT_IMPLEMENTED and this wasn't a problem but I feel that Kodi might not be liking it when pCapabilities->bSupportsRecordingSize is true.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

phunkyfish commented 4 years ago

The GetRecordingSizeCalls are fine, that's normal.

phunkyfish commented 4 years ago

Should m_sid be m_sid.c_str()?

emveepee commented 4 years ago

The GetRecordingSizeCalls are fine, that's normal.

Thanks,

Should m_sid be m_sid.c_str()?

no m_sid is char[]

Where exactly does it crash? Have you run it through a debugger?

No one spot and I haven't duplicated it today so I maybe moving artworkPath https://github.com/kodi-pvr/pvr.nextpvr/pull/121/commits/d50b84cd55935adf299fb155f2978013ab518b9e#diff-d502357537ac071b04080ad67ea5625fR575 before the if was necessary

In the debugger it was in a map related function in xtree.cpp on Visual Studio so I need to check to see if my maps are working. Since I can't duplicate it this morning it I am not sure

I do have regular crashes in Matrix on clear pvr database on what seems to be the same error which may be a bug related to the serious issue that needs addressing. Before clearing the PVR manage, Kodi has to persist new data and that can can a long minutes. I haven't got around to looking into this. I know that my destructor is not being called after the clear which I think is an issue but I don't know how PVR notifies me of the restart.

I decided to investigated this and now I see the issue, it was a stupid typo forgot to reset the itr here https://github.com/kodi-pvr/pvr.nextpvr/pull/121/commits/d50b84cd55935adf299fb155f2978013ab518b9e#diff-d502357537ac071b04080ad67ea5625fR804

I have made these two changes so I think the crashing issues were resolved

It would be nice if after a reset Kodi could notify the client a reset was happening.

phunkyfish commented 4 years ago

Did you find the issue?

emveepee commented 4 years ago

Did you find the issue?

Yes thanks, moving the artworkPath out of an "if" to keep it from being de-referenced probably solved one issue and iterating properly helped another .

Debugging this I noticed that Kodi doesn't reset the client when a database reset occurs so the preexisting logic of keeping a global variable for GetNumChannels() was wrong. I will rework https://github.com/kodi-pvr/pvr.nextpvr/pull/121/commits/7a6d1cdcf8cd368637b9b93d0682fb4550d18506 and resubmit it with the Code Review commit unless you can share a way to modify a previous commit..

phunkyfish commented 4 years ago

You can modify any previous commit by rebasing.

For instance if the commit you want to edit is 7 commits back then run git rebase -i HEAD~7. Then change the commit in question from pick to edit. Next make your changes, save them and then run git add -u and then git commit --amend. Finish it off with git rebase --continue.

In english, you are rewinding to the commit you want to edit, changing it, recommitting it and applying all the later commits on top of it. Just note that if any of changes cause conflicts in a later commit you will need to fix those as you go.

emveepee commented 4 years ago

You can modify any previous commit by rebasing.

For instance if the commit you want to edit is 7 commits back then run git rebase -i HEAD~7. Then change the commit in question from pick to edit. Next make your changes, save them and then run git add -u and then git commit --amend. Finish it off with git rebase --continue.

In english, you are rewinding to the commit you want to edit, changing it, recommitting it and applying all the later commits on top of it. Just note that if any of changes cause conflicts in a later commit you will need to fix those as you go.

Thanks that is so helpful. It looks like the GetNumChannel commit is what I want, but the build system did't like it. Missed one in a later commit which you probably suspected. Cherry picking is not for the faint of heart and all good now.

emveepee commented 4 years ago

@AlwinEsch I have to add XBMC->CreateDirectory() because without settings.xml (which does work) I don't have the user folder which holds channel icons

However XBMC->CreateDirectory("special://userdata/addon_data/pvr.nextpvr/"); won't compile on Windows because of the Windows define

define CreateDirectory CreateDirectoryA

If I #undef CreateDirectory this works Is that how this is supposed to work?

@phunkyfish if you have an alternate solution for creating the user folder let me know.

phunkyfish commented 4 years ago

You should always pass by reference if you don't need a copy of an object. Generally const objects should be passed by reference.

emveepee commented 4 years ago

Well done ;)

Thanks for all your input. Now just need to know about CreateDirectory for the last commit on this.

You should always pass by reference if you don't need a copy of an object. Generally const objects should be passed by reference.

Thanks for that tip.

phunkyfish commented 4 years ago

If I remember correctly that I needed to do something like that in iptv simple. Maybe have a look there.

emveepee commented 4 years ago

If I remember correctly that I needed to do something like that in iptv simple. Maybe have a look there.

Basically I can put it it in modules where windows.h is not included. I didn't notice anything specific in iptv simple that needs windows.h

Martin

phunkyfish commented 4 years ago

I’m not sure now but I remember needing it. Maybe around Stats. Or something.

emveepee commented 4 years ago

Hi I have sent 3 new commit's since the last review. I read about menuhooks in a recent PR and they were new to me but after reading more they seemed like the best way to do the reset of a channels items. I saw this in mythv and from there figured out how to present a list of backend IP's to the user on a new install.

phunkyfish commented 4 years ago

Can I suggest maybe we move the newer commits out from this PR? A PR should be a completed collection of work you are interested in merging. It may never end if you keep adding commits ;)

emveepee commented 4 years ago

Can I suggest maybe we move the newer commits out from this PR? A PR should be a completed collection of work you are interested in merging. It may never end if you keep adding commits ;)

I understand what you are saying but Settings.cpp Settings.h and (effectively) Settings.xml are new files and I feel they should be be merged in final form which is what this PR is. There is no real value in a merge followed by immediate new PR and then the inevitable delay that sub3 imposes waiting for additional comments after yours. During that delay there could be a translation PR which could only complicate things. It would certainly have helped if I had known about menuhooks when I started this PR.

I could remove the Request class but I didn't really like this code https://github.com/kodi-pvr/pvr.nextpvr/pull/121/commits/a7f4ba62f0cba72b19323d16c62d335f41edf167#diff-34f48ad91ac6c202ac60b0348ae90e30R76 that is in this code and it makes for a cleaner implementation.

I am not expecting another change to this module and I am hoping that it can be merged and tagged fairly quickly.

phunkyfish commented 4 years ago

If we are drawing a line after this then ok for this PR.

What I'm trying to get to is that in the future let's try and have completed PRs to review, as there is a duplicate of effort on my part and time is finite ;)

emveepee commented 4 years ago

@phunkyfish I understand your concern about time, but there does seem to be a long delay at the review stage that leads to these. When I first released this there was a week of no comments when I hoped for a merge and then a few rounds like this with days without comment. Half of the 21 days for this PR are simply waiting for @sub3 and you to review. I would have hoped that this could have been merged and tagged by now so alpha test could be done in earnest.

I already have started on my next big change which I have mentioned (separating timers, recordings and channels into classes with many clean ups ) but with everything jamming up at the review phase the commits might appear to stack up.

phunkyfish commented 4 years ago

This is the reality though of this open source project (and many others). I'm a volunteer and do this in my spare time. That means PRs can take time to get through.

Just finishing the next review of this PR.

phunkyfish commented 4 years ago

I was just chatting to @AlwinEsch and it looks like the C++ migration will happen in about 2 weeks. It’s pretty extensive so we will need to co-ordinate PRs around that time.

Do you think your next PR will be in by then?

sub3 commented 4 years ago

@phunkyfish I understand your concern about time, but there does seem to be a long delay at the review stage that leads to these. When I first released this there was a week of no comments when I hoped for a merge and then a few rounds like this with days without comment. Half of the 21 days for this PR are simply waiting for @sub3 and you to review. I would have hoped that this could have been merged and tagged by now so alpha test could be done in earnest.

I usually hold off merging it for a bit to give others a chance to comment, but I'm always happy to be rushed, particularly for smaller less controversial changes. Just post a 'any comments?' reminder, and give me a bump if you'd like to get things moving.

emveepee commented 4 years ago

I was just chatting to @AlwinEsch and it looks like the C++ migration will happen in about 2 weeks. It’s pretty extensive so we will need to co-ordinate PRs around that time.

Do you think your next PR will be in by then?

I have TImers done and that was the biggest one, I expect after this PR is merged and tagged I can focus on it and get it submitted in a few days. I am sending all the modules through clang-format so that should make your review easier. Typically it is changing native tinyxml to XMLUtils, getting rid of PVR_STRCPY where I can and changing some things to C++. I might do some snprintf changes too when it makes sense.

phunkyfish commented 4 years ago

Cool, sounds like a plan. I will let Alwin know not to do the nextpvr PR till closer to the merge date.

emveepee commented 4 years ago

I usually hold off merging it for a bit to give others a chance to comment, but I'm always happy to be rushed, particularly for smaller less controversial changes. Just post a 'any comments?' reminder, and give me a bump if you'd like to get things moving.

I am not sure if simply parking a PR for days after @phunkyfish approved it serves much purpose unless you actually call on other reviewers. Basically I am just waiting for the merge and tag.