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

Jump to bottom

[addons][pvr] change addon system to C++ style #16485

Merged
merged 27 commits into from Jun 12, 2020
Merged

[addons][pvr] change addon system to C++ style #16485

merged 27 commits into from Jun 12, 2020

Conversation

AlwinEsch
Copy link
Member

@AlwinEsch AlwinEsch commented Aug 12, 2019 •
edited

Description

This converts the PVR addon system to the C ++ type.

The changes here are still in their infancy and not fully tested yet.

In addition, the Doxygen documentation is revised to have the presentation arranged.

The last commit with the WIP is not finished yet and for behaving to addon changes this has to be properly and thoroughly set up and tested.

Ready addons

Addon Request Changed Tested Finished
pvr.argustv kodi-pvr/pvr.argustv#102 ✔️ ✔️ ✔️
pvr.demo kodi-pvr/pvr.demo#65 ✔️ ✔️ ✔️
pvr.dvblink kodi-pvr/pvr.dvblink#135 ✔️ ✔️ ❔
pvr.dvbviewer kodi-pvr/pvr.dvbviewer#90 ✔️ ➖ ❔
pvr.filmon kodi-pvr/pvr.filmon#103 ✔️ ➖ ❔
pvr.freebox aassif/pvr.freebox#53 ✔️ ➖ ❌
pvr.hdhomerun kodi-pvr/pvr.hdhomerun#99 ✔️ ➖ ❌
pvr.hts kodi-pvr/pvr.hts#455 ✔️ ✔️ ❌
pvr.iptvsimple kodi-pvr/pvr.iptvsimple#365 ✔️ ✔️ ❌
pvr.mediaportal.tvserver kodi-pvr/pvr.mediaportal.tvserver#120 ✔️ ✔️ ❌
pvr.mythtv janbar/pvr.mythtv#152 ✔️ ✔️ ❌
pvr.nextpvr kodi-pvr/pvr.nextpvr#128 ✔️ ➖ ❌
pvr.njoy kodi-pvr/pvr.njoy#60 ✔️ ✔️ ❌
pvr.octonet DigitalDevices/pvr.octonet#43 ✔️ ➖ ❌
pvr.pctv kodi-pvr/pvr.pctv#79 ✔️ ➖ ❌
pvr.rtl.radiofm ✔️ ➖ ❌
pvr.sledovanitv.cz palinek/pvr.sledovanitv.cz#62 ✔️ ➖ ❌
pvr.stalker kodi-pvr/pvr.stalker#137 ✔️ ➖ ❌
pvr.teleboy rbuehlma/pvr.teleboy#54 ✔️ ➖ ❌
pvr.vbox kodi-pvr/pvr.vbox#239 ✔️ ➖ ❌
pvr.vdr.vnsi kodi-pvr/pvr.vdr.vnsi#124 ✔️ ✔️ ✔️
pvr.vuplus kodi-pvr/pvr.vuplus#282 ✔️ ➖ ❌
pvr.waipu flubshi/pvr.waipu#76 ✔️ ✔️ ❔
pvr.wmc kodi-pvr/pvr.wmc#85 ✔️ ➖ ❌
pvr.zattoo rbuehlma/pvr.zattoo#90 ✔️ ✔️ ✔️

Motivation and Context

How Has This Been Tested?

Currently only with pvr.demo from here:
kodi-pvr/pvr.demo#65

Screenshots (if appropriate):

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • Clean up (non-breaking change which removes non-working, unmaintained functionality)
  • Improvement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that will cause existing functionality to change)
  • Cosmetic change (non-breaking change that doesn't touch code)
  • None of the above (please explain below)

Checklist:

  • My code follows the Code Guidelines of this project
  • My change requires a change to the documentation, either Doxygen or wiki
  • I have updated the documentation accordingly
  • I have read the Contributing document
  • I have added tests to cover my change
  • All new and existing tests passed

@AlwinEsch AlwinEsch added Type: Feature non-breaking change which adds functionality Component: PVR WIP PR that is still being worked on Component: Binary add-ons API change: Binary add-ons v19 Matrix labels Aug 12, 2019
@AlwinEsch AlwinEsch added this to the Matrix 19.0-alpha 1 milestone Aug 12, 2019
@AlwinEsch AlwinEsch requested a review from ksooo August 12, 2019 19:22
@AlwinEsch AlwinEsch mentioned this pull request Aug 12, 2019
Merged
@AlwinEsch
Copy link
Member Author

The added documents are now visible here: http://alwinesch.github.io/group__cpp__kodi__addon__pvr.html

@ksooo
Copy link
Member

ksooo commented Aug 13, 2019 •
edited

Great @AlwinEsch . ✌️

I will review as as soon as I find some time.

@ksooo ksooo added the Type: Breaking change fix or feature that will cause existing functionality to change label Aug 13, 2019
ksooo
ksooo reviewed Aug 13, 2019
View reviewed changes
Copy link
Member

@ksooo ksooo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work. Some general questions and suggestions, though.

xbmc/addons/kodi-addon-dev-kit/include/kodi/addon-instance/PVRClient.h Outdated Show resolved Hide resolved
xbmc/addons/kodi-addon-dev-kit/include/kodi/addon-instance/PVRClient.h Outdated Show resolved Hide resolved
xbmc/addons/kodi-addon-dev-kit/include/kodi/addon-instance/PVRClient.h Outdated Show resolved Hide resolved
xbmc/addons/kodi-addon-dev-kit/include/kodi/addon-instance/PVRClient.h Outdated Show resolved Hide resolved
xbmc/addons/kodi-addon-dev-kit/include/kodi/addon-instance/PVRClient.h Outdated Show resolved Hide resolved
@AlwinEsch AlwinEsch added the No Jenkins do not run automatic Jenkins builds on this PR label Aug 14, 2019
@AlwinEsch AlwinEsch mentioned this pull request Aug 17, 2019
Merged
13 tasks
@AlwinEsch
Copy link
Member Author

Not final, but is updated:
pvr.demo: kodi-pvr/pvr.demo#65
Related docs: http://alwinesch.github.io/group__cpp__kodi__addon__pvr.html

Test with pvr.demo was OK.

@AlwinEsch AlwinEsch mentioned this pull request Aug 29, 2019
Merged
@jenkins4kodi jenkins4kodi added the Rebase needed PR that does not apply/merge cleanly to current base branch label Aug 29, 2019
@AlwinEsch AlwinEsch mentioned this pull request Sep 12, 2019
Closed
13 tasks
@AlwinEsch AlwinEsch mentioned this pull request Sep 21, 2019
Merged
13 tasks
@stsichler
Copy link
Contributor

IMO, convertion to a C++ plugin API is a very bad idea.

I'm a professional C/C++ developer and for many years I was responsible for an image processing plugin library for both Windows and Linux that was loaded by several applications.
We once started with a C++ API, but then quickly realized that we have to switch to a pure C API, because of severe limitations of a C++ API (at least on Windows):
Namely, your application and the plugin DLL need to be compiled with exactly the same CRT configuration and in most cases, you also need exactly the same compiler and compiler settings. Especially, that means that you cannot mix Debug and Release builds or different CRT versions (with respect to security hotfixes). Your plugins will crash when you do that.

So, I strongly believe that the conversion to a C++ plugin API will complicate plugin development instead of simplifying it.

@phunkyfish
Copy link
Contributor

It will still use a C API under the hood. Think of c++ being an implementation language of which there could be others.

@stsichler
Copy link
Contributor

Hm, ok. I currently don't fully overlook the changes.
I just wanted to caution you about passing C++ types like std::string across DLL boundaries. this may potentionally crash in such circumstances described.

@ksooo
Copy link
Member

ksooo commented Oct 30, 2019 •
edited

passing C++ types like std::string across DLL boundaries.

Full ack. Good thing is that we will not do that. :-) Everything that crosses shared library boundaries is plain C. The new C++ API is "just" a convenience wrapper for a C API. The addon dev needs not to deal with C stuff any longer.

@AlwinEsch
[addons][pvr] move all "C" parts to independent header place
88baa69 
This thought to have it cleaner and to take for other languages.
@AlwinEsch
[addons][pvr] change "C" API to use everywhere PVR_ERROR as return
1e41759 
To allow by them a direct handling if e.g. connection error comes.
@AlwinEsch
[addons][pvr] use uint64_t for timer types
9362b39 
Before was already 29 bits used where the unsigned int comes to his end.
This use now a 64 bit for them.
@AlwinEsch
[addons][pvr] add C++ PVR instance class
f9a6f80 
This add the new instance class for PVR system. There everything
supported like before but now on easier way on addons.

This are splittet to separate files.
@AlwinEsch
[addons][pvr] set -1 by defines on recordings and timers
63330cf 
This use "PVR_[TIMER][RECORDING]_VALUE_NOT_AVAILABLE" to define it as -1.
@AlwinEsch
[addons][pvr] add complete reworked documentation
980a31a 
This add back the on first commit removed docs in a good usable
doxygen style and much increased on most places.
@AlwinEsch
[addons][pvr][upload] rename addon id "xbmc.pvrclient" to "kodi.pvrcl…
a5ea7ed 
…ient"

As this is a very big change it makes sense for me to rename them too.
@AlwinEsch
[addons][pvr] include only the "C" related headers on kodi
b5b9527 
Only on PVRClient.cpp is the C++ part included to make a check about.
@AlwinEsch
[addons][pvr] complete "C" language cleanup of PVR changes
58f50fb 
Only ignored parts are the "#define"'s within "C" headers , as them
within a 'extern "C"' becomes his parts two spaces but not the define.
@AlwinEsch
[addons][pvr] make menu hook transfer to Kodi in "const"
c7a558b 
Before was it without, but as this need nowhere a change it must be "const"
to prevent wrong changes.
@AlwinEsch
[addons][pvr] increase version to 7.0.0
f993d54 
Also are his new header depends addd to there.
@AlwinEsch
[addons] add script to generate function lists for used parts in inte…
7ef55fa 
…rface group

This thought to generate a easily by user usable list about parts
and allow c&p for new projects.

This scan the headers about and if a `/// @copydetails ...header_addon_auto_check`
or `/// @copydetails ...source_addon_auto_check` is included becomes about related
group the list generated.

This list is added in .gitignore to prevent problems if by API changes not updated.

There must be a system in automatic doxygen build start by doc upload added.
@AlwinEsch AlwinEsch mentioned this pull request Jun 12, 2020
Merged
@AlwinEsch
Copy link
Member Author

jenkins build this please

@AlwinEsch AlwinEsch merged commit 0c10510 into xbmc:master Jun 12, 2020
@AlwinEsch AlwinEsch deleted the change-pvr-addons branch June 12, 2020 19:16
@StormTrooper StormTrooper mentioned this pull request Jun 14, 2020
Closed
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Jun 14, 2020
@AlwinEsch @Maven85
Merge pull request xbmc#16485 from AlwinEsch/change-pvr-addons
1d2641f 
[addons][pvr] change addon system to C++ style
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Jun 15, 2020
@AlwinEsch @Maven85
Merge pull request xbmc#16485 from AlwinEsch/change-pvr-addons
9714287 
[addons][pvr] change addon system to C++ style
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 3, 2020
@AlwinEsch @Maven85
Merge pull request xbmc#16485 from AlwinEsch/change-pvr-addons
6e6e810 
[addons][pvr] change addon system to C++ style
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 4, 2020
@AlwinEsch @Maven85
Merge pull request xbmc#16485 from AlwinEsch/change-pvr-addons
5a09485 
[addons][pvr] change addon system to C++ style
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 4, 2020
@AlwinEsch @Maven85
Merge pull request xbmc#16485 from AlwinEsch/change-pvr-addons
7899ae8 
[addons][pvr] change addon system to C++ style
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 5, 2020
@AlwinEsch @Maven85
Merge pull request xbmc#16485 from AlwinEsch/change-pvr-addons
38b946f 
[addons][pvr] change addon system to C++ style
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 5, 2020
@AlwinEsch @Maven85
Merge pull request xbmc#16485 from AlwinEsch/change-pvr-addons
126172f 
[addons][pvr] change addon system to C++ style
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 6, 2020
@AlwinEsch @Maven85
Merge pull request xbmc#16485 from AlwinEsch/change-pvr-addons
f5e84e9 
[addons][pvr] change addon system to C++ style
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 6, 2020
@AlwinEsch @Maven85
Merge pull request xbmc#16485 from AlwinEsch/change-pvr-addons
88f0952 
[addons][pvr] change addon system to C++ style
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 6, 2020
@AlwinEsch @Maven85
Merge pull request xbmc#16485 from AlwinEsch/change-pvr-addons
be0b0f4 
[addons][pvr] change addon system to C++ style
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 6, 2020
@AlwinEsch @Maven85
Merge pull request xbmc#16485 from AlwinEsch/change-pvr-addons
8675e50 
[addons][pvr] change addon system to C++ style
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 7, 2020
@AlwinEsch @Maven85
Merge pull request xbmc#16485 from AlwinEsch/change-pvr-addons
e47e97b 
[addons][pvr] change addon system to C++ style
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FernetMenta FernetMenta FernetMenta left review comments

@emveepee emveepee emveepee left review comments

@ksooo ksooo ksooo approved these changes

@phunkyfish phunkyfish phunkyfish left review comments

@a1rwulf a1rwulf a1rwulf left review comments

@Rechi Rechi Rechi left review comments

@garbear garbear garbear approved these changes

Assignees
No one assigned
Labels
API change: Binary add-ons Component: Binary add-ons Component: PVR PR Cleanup: Merge Candidate Candidate for merging after PR cleanup Type: Breaking change fix or feature that will cause existing functionality to change Type: Feature non-breaking change which adds functionality v19 Matrix
Projects
None yet
Milestone
Matrix 19.0-alpha 1
Development

Successfully merging this pull request may close these issues.

None yet