emveepee commented 4 years ago

Change pvrclient from monolithic code model. C++ from C, remove PVR_STRCPY, sprintf when feasible, remove raw tinyxml. Remove dead and debug code. Processed with clang-format

emveepee commented 4 years ago

This commit is going to serve as the baseline for me to begin to implement PR 16485. @phunkyfish or @sub3 I am not sure if you want to review it or not since it will be changed almost completely by the end of this PR.

phunkyfish commented 4 years ago

@emveepee is this PR ready for review right now? Any substantial change in code structure should be done and merged before 16485.

Let’s ask @alwinus but it may be easier for him to do the C++ API PR. Not that he wouldn’t mind the help but he’ll be working across many PVR addons. @alwinus, thoughts? Don’t get me wrong I’m easy either way but it’s not me doing the work.

phunkyfish commented 4 years ago

Just some high level points for future work. Firstly LOG_API_CALL and LOG_API_IRET could probably be moved out to a logger class and remove the need to have these in numerous classes. Everywhere you have NULL can be nulltpr I'd say.

There is this in a lot of places:

#if defined(TARGET_WINDOWS)
  #define atoll(S) _atoi64(S)   
#else
  #define MAXINT64 ULONG_MAX
#endif

This really should not be needed (I hope). Maybe @AlwinEsch has an idea on how to avoid it.

emveepee commented 4 years ago

@emveepee is this PR ready for review right now? Any substantial change in code structure should be done and merged before 16485.

I wasn't sure if I wanted to go the the full mapping and break the code down further to add these classes. The first three are pretty static so it might be good. Do you think it is worth it?

include "pvr/ChannelGroups.h"

include "pvr/EDL.h"

include "pvr/MenuHook.h"

include "pvr/Stream.h"

Let’s ask @AlwinEsch ut it may be easier for him to do the C++ API PR. Not that he wouldn’t mind the help but he’ll be working across many PVR addons. @AlwinEsch , thoughts? Don’t get me wrong I’m easy either way but it’s not me doing the work.

If I don't do it now, it will probably take just as long or longer to understand the code after the conversion.

alwinus commented 4 years ago

Hi,

Please stop mailing me. I am not the @alwinus you think I am.

Regards, Alwin

On 12 May 2020, at 16:17, emveepee notifications@github.com wrote:

@emveepee https://github.com/emveepee is this PR ready for review right now? Any substantial change in code structure should be done and merged before 16485.

I wasn't sure if I wanted to go the the full mapping and break the code down further to add these classes. The first three are pretty static so it might be good. Do you think it is worth it?

include "pvr/ChannelGroups.h"

include "pvr/EDL.h"

include "pvr/MenuHook.h"

include "pvr/Stream.h"

Let’s ask @alwinus https://github.com/alwinus but it may be easier for him to do the C++ API PR. Not that he wouldn’t mind the help but he’ll be working across many PVR addons. @alwinus https://github.com/alwinus, thoughts? Don’t get me wrong I’m easy either way but it’s not me doing the work.

If I don't do it now, it will probably take just as long or longer to understand the code after the conversion.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/kodi-pvr/pvr.nextpvr/pull/124#issuecomment-627374519, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJGBAHQTO3WY2YCHQDVECTRRFLA3ANCNFSM4M6H4TRQ.

emveepee commented 4 years ago

Just some high level points for future work. Firstly LOG_API_CALL and LOG_API_IRET could probably be moved out to a logger class and remove the need to have these in numerous classes.

I don't want to create a logger class, it would make more sense to support spd_trace.

Everywhere you have NULL can be nulltpr I'd say.

That's fair I will do that globally now.

There is this in a lot of places:
#if defined(TARGET_WINDOWS)
  #define atoll(S) _atoi64(S) 
#else
  #define MAXINT64 ULONG_MAX
#endif
This really should not be needed (I hope). Maybe @AlwinEsch has an idea on how to avoid it.

I just took that from the existing pvrclient-nextpvr code and because of testing on Windows and ubuntu I wasn't sure where it was needed so I copied it to all classes.

phunkyfish commented 4 years ago

Apologies. My mistake.

On 12 May 2020, at 15:19, alwinus notifications@github.com wrote:

Hi,

Please stop mailing me. I am not the @alwinus you think I am.

Regards, Alwin

On 12 May 2020, at 16:17, emveepee notifications@github.com wrote:

@emveepee https://github.com/emveepee is this PR ready for review right now? Any substantial change in code structure should be done and merged before 16485.

I wasn't sure if I wanted to go the the full mapping and break the code down further to add these classes. The first three are pretty static so it might be good. Do you think it is worth it?

include "pvr/ChannelGroups.h"

include "pvr/EDL.h"

include "pvr/MenuHook.h"

include "pvr/Stream.h"

Let’s ask @alwinus https://github.com/alwinus but it may be easier for him to do the C++ API PR. Not that he wouldn’t mind the help but he’ll be working across many PVR addons. @alwinus https://github.com/alwinus, thoughts? Don’t get me wrong I’m easy either way but it’s not me doing the work.

If I don't do it now, it will probably take just as long or longer to understand the code after the conversion.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/kodi-pvr/pvr.nextpvr/pull/124#issuecomment-627374519, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJGBAHQTO3WY2YCHQDVECTRRFLA3ANCNFSM4M6H4TRQ.

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

phunkyfish commented 4 years ago

@emveepee this is really good stuff.

The only remaining point I would make is that you have #define in namespaces. But you can't namespace preprocessor directives. So instead of these you should use static const int VAR = val; etc if you want to limit scope.

If you want to keep the defines move them up to below the includes. I generally try to avoid the use of defines if at all possible in C++ code ;)

emveepee commented 4 years ago

If you want to keep the defines move them up to below the includes. I generally try to avoid the use of defines if at all possible in C++ code ;)

Thanks, I was thinking of moving them up but figured I'd try something new. Should I use constexpr instead? That seems to be what the Kodi style guide wants.

phunkyfish commented 4 years ago

If you want to keep the defines move them up to below the includes. I generally try to avoid the use of defines if at all possible in C++ code ;)

Thanks, I was thinking of moving them up but figured I'd try something new. Should I use constexpr instead? That seems to be what the Kodi style guide wants.

Ya, if you can can use constexpr you should. I generally havn't but that's just habit ;)

emveepee commented 4 years ago

@phunkyfish I think I have addressed your comments (thanks again), now I need to see if @AlwinEsch wants me to continue to convert this to PR16485 on my own (at least as far as I can)

@AlwinEsch I am confused about instance handling for PR16485 since I don't see how that addresses multiple settings.xml Is instance handling for future PIP which doesn't seem to be addressed elsewhere.

phunkyfish commented 4 years ago

I’ll will review later but we should get this merged before any work on PR16485 IMO.

Note that any prep work for PIP is not part of C++ PR. That will come in a later phase.

emveepee commented 4 years ago

I’ll will review later but we should get this merged before any work on PR16485 IMO.

Definitely I agree to this. Other then waiting for @sub3 to comment on your request about LOG_API_CALL(FUNCTION) since I don't need them, those are there for his debugging, I feel you can merge this once you are happy with the review.

Note that any prep work for PIP is not part of C++ PR. That will come in a later phase.

Fair enough but multi instance seems to be too early too, as I said I don't know how to code for this with discovery, settings defaults etc in this scenario. Clearly I don't want local host twice. I thought maybe it was for PIP.

sub3 commented 4 years ago

I’ll will review later but we should get this merged before any work on PR16485 IMO.

Definitely I agree to this. Other then waiting for @sub3 to comment on your request about LOG_API_CALL(FUNCTION) since I don't need them, those are there for his debugging, I feel you can merge this once you are happy with the review.

I'm not attached to them and don't remind if they're removed/replaced - as long as you're comfortable that the logs still contain enough info to support users.

emveepee commented 4 years ago

I'm not attached to them and don't remind if they're removed/replaced - as long as you're comfortable that the logs still contain enough info to support users.

These are compile time #define logs messages which aren't in the release builds so they are developer use only. I will take them out.

phunkyfish commented 4 years ago

Also, @emveepee I spoke to @AlwinEsch. If you want to implement PR 16485 he would be glad of the help.

You would just need to get it all working and tested before before PR16485 get's merged. This means you only get the benefit of CI for a very small window but that's unavoidable.

emveepee commented 4 years ago

Also, @emveepee I spoke to @AlwinEsch. If you want to implement PR 16485 he would be glad of the help.

You would just need to get it all working and tested before before PR16485 get's merged. This means you only get the benefit of CI for a very small window but that's unavoidable.

Ok I will go as far as I can. @sub3 when you are ready please merge. I don't see any reason to tag and release this but I create a changelog and version bump if you really want.

phunkyfish commented 4 years ago

@AlwinEsch had a few addons done. Zattoo, iptvsimple and demo should all be good reference points to start with.

There is still a filesystem API change to go in which should further simplify things. Once that is in iptvsimple will be finished off and vuplus will get done.

emveepee commented 4 years ago

@AlwinEsch had a few addons done. Zattoo, iptvsimple and demo should all be good reference points to start with.

Yes I've been building and reviewing demo for the last couple of weeks. I have an issue right now where Kodi is requesting GetNumChannels() continuously when I go into recordings so I will need to adjust something to reduce the calls to the backend before I move on.

AlwinEsch commented 4 years ago

@AlwinEsch I am confused about instance handling for PR16485 since I don't see how that addresses multiple settings.xml Is instance handling for future PIP which doesn't seem to be addressed elsewhere.

Currently is it not supported, only prepared about. We must look how the best way is to handle them. My ideas was to make several ways, where then the addon.xml define how.

Addon bring amount on his settings.xml, e.g. 5 pages and support then five instances.
- With backends_controller="addon" shows then that he handle it
- kodi::GetSettingsString("01.username");
- kodi::GetSettingsString("02.username");
- There becomes then 5 times the CreateInstance called and addon can say yes or no.
- like this then on xml:
```
<extension point="kodi.pvrclient" 
    own_handled_backends="5"
    backends_controller="addon"
    library_@PLATFORM@="@LIBRARY_FILENAME@"/>`
```
Addon support big amount of backend, but have only one setting
- Kodi clone then the settings and addon must then ask setting with his instance id.
- kodi::GetSettingsString(m_strInstanceId + ".username");
- There must then a extra button on Kodi's addon window to duplicate settings and use for another place
- like this then on xml:
```
<extension point="kodi.pvrclient" 
    own_handled_backends="-1" 
    backends_controller="kodi"
    library_@PLATFORM@="@LIBRARY_FILENAME@"/>`
```
If addon.xml like now without any extra parts, then show that only one time is supported.

emveepee commented 4 years ago

Currently is it not supported, only prepared about. We must look how the best way is to handle them. My ideas was to make several ways, where then the addon.xml define how.
* Addon bring amount on his settings.xml, e.g. 5 pages and support then five instances.

Sounds like I don't need to worry too much now. Can the logs indicate the instance automatically or is that something that I should be considering while updating log message now? I am assuming I will need to track which backend might be failing.

phunkyfish commented 4 years ago

I think at the time we solve this we should be leveraging spdlog and handling it there. I would hate to think every addon would need to change its logging.

emveepee commented 4 years ago

There is still a filesystem API change to go in which should further simplify things. Once that is in iptvsimple will be finished off and vuplus will get done.

@AlwinEsch @phunkyfish I am having trouble with live tv and recordings immediately not starting with the c++ change. Would this be related to the fileysystem API change? The logs are here https://paste.kodi.tv/cazuduveda.kodi and the live attempt is at 2020-05-18 07:24:32.336 and I do get a response from the server

The existing code is here https://github.com/kodi-pvr/pvr.nextpvr/blob/Matrix/src/buffers/Buffer.cpp#L40 and for now I am using the raw url with various flags

kodi::vfs::CFile m_inputHandle;
m_inputHandle.OpenFile(inputUrl, optFlag);

I also tried CURLCreate with no success

phunkyfish commented 4 years ago

They will be the same. Can you open a PR and reference the new code?

emveepee commented 4 years ago

They will be the same. Can you open a PR and reference the new code?

OK but I will note the PR C++ testing is still work in progress . Notably many QueueNotifications will be broken I need to make a va_args flavour.

phunkyfish commented 4 years ago

Just put WIP in the title.

kodi-pvr / pvr.nextpvr

Code Restructuring #124

include "pvr/EDL.h"

include "pvr/MenuHook.h"

include "pvr/Stream.h"

include "pvr/EDL.h"

include "pvr/MenuHook.h"

include "pvr/Stream.h"

include "pvr/EDL.h"

include "pvr/MenuHook.h"

include "pvr/Stream.h"