[addons][vfs] cleanup, "C" interface fix, documentation rework #17527

AlwinEsch · 2020-03-20T22:32:18Z

Description

This request revises the VFS addon interface. The changes in the addon itself are rather minor, since only the CVFSEntry::IoControl has been revised on the addon to avoid the Kodi-side header "IFileTypes.h" (this only corresponded to C++).

This API change now translates the Kodi site to its own for addon, which makes this "C" ABI compatible and more importantly, safer in the event of changes to IFileTypes.h, which could inadvertently cause problems with addons.

Another API change is that the individual structures are now in their own arrays, so it is safer if a callback function or property is added. However, this does not require any code changes to addon and is fixed with a new creation.

The commit "[settings] allow TranslatableIntegerSettingOption to get string from" allows the addon itself a string ID to its network drive connection that can be created in Kodi's settings. This is independent and will still be moved in a own request.

There are also some clang code cleanups in it.

In addition, everything between Kodi and addon is now in pure "C".

Here is the new documention view available: http://alwinesch.github.io/group__cpp__kodi__addon__vfs.html

Motivation and Context

Related to #17514 and to become the documentation in future available for everything 😁

How Has This Been Tested?

Tested now on few addons, need to test more and comes next days, also is the API not so big that it becomes critical.

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

garbear · 2020-03-21T03:51:27Z

xbmc/addons/kodi-addon-dev-kit/include/kodi/addon-instance/VFS.h

-    KodiToAddonFuncTable_VFSEntry toAddon;
+    AddonProps_VFSEntry* props;
+    AddonToKodiFuncTable_VFSEntry* toKodi;
+    KodiToAddonFuncTable_VFSEntry* toAddon;


what about std::unique ptrs?

We can not use a C++ std::unique in a "C" ABI, thats one of the main reason of this request.

oh yea, didn't look at the location of this file.

NP, thanks a lot for review!

garbear · 2020-03-21T03:54:40Z

xbmc/network/GUIDialogNetworkSetup.h

@@ -24,6 +24,7 @@ class CGUIDialogNetworkSetup : public CGUIDialogSettingsManualBase
    int defaultPort;       //!< Default port to use for protocol
    std::string type;      //!< URL type for protocol
    int label;             //!< String ID to use as label in dialog
+    std::string addonId; //!< Addon identifier, leaved empty if inside Kodi


It's not evident to me, where is this used?

Currently vfs addons are forced to take Kodi's own string id's (e.g. at vfs.sftp https://github.com/xbmc/vfs.sftp/blob/Leia/vfs.sftp/addon.xml.in#L21) and the std::pair<int, int> in settings this could not be identified.

This is then taken here https://github.com/xbmc/xbmc/pull/17527/files#diff-8211fa801621c92df9bf6515d2d45b8aR200 to give to replaced pair with TranslatableIntegerSettingOption his addon id.

EDIT: Without is the "My special secure VFS addon" not possible to give on GUI

thanks for the explanation

garbear · 2020-03-21T03:55:00Z

Two comments, otherwise looks good

xbmc/addons/AddonBindings.cmake

This was before given to Kodi to have on his calls to addon. That basically not match a "C" ABI where the use of "C++" parts within "C" not allowed! With this it becomes given via "void*" to Kodi and with a "static_cast" changed back to required class on addon.

Added to separate "C" and "C++" part and further to allow a "C" only in future.

On this commit becomes them replaced to use independent enum and structures, to pass related data between Kodi and Addon. This prevents conflicts when using "C" and ensures a secure ABI interface. In addition, it is much safer in the event that the Kodi changes what in its headers. This can easily occur because previously it was not directly visible that IFileTypes.h was also used on addons. Only currently still used on libXBMC_addon.h and removed on PVR rework.

This done to prevent API backward problems if something becomes replaced e.g. on props or to kodi calls.

This changes the interface to pure "C" and no "C++" dependencies in it.

With this change the whole VFS documentation becomes reworked and to have usable on doxygen. There are much parts added to understand his usage.

AlwinEsch · 2020-03-27T18:19:08Z

If nothing against then I bring it in tommorow and then update of the addons.
Working test of all vfs addons was OK.

jenkins build this please

[addons][vfs] cleanup, "C" interface fix, documentation rework

AlwinEsch added Type: Cleanup non-breaking change which removes non-working or unmaintained functionality Type: Improvement non-breaking change which improves existing functionality Component: Binary add-ons Documentation API change: Binary add-ons v19 Matrix labels Mar 20, 2020

AlwinEsch added this to the Matrix 19.0-alpha 1 milestone Mar 20, 2020

AlwinEsch requested review from arnova, afedchin and notspiff March 20, 2020 22:32

AlwinEsch requested review from DaveTBlake and garbear as code owners March 20, 2020 22:32

garbear reviewed Mar 21, 2020

View reviewed changes

garbear approved these changes Mar 21, 2020

View reviewed changes

AlwinEsch mentioned this pull request Mar 21, 2020

change XFILE::EIoControl to VFS_IOCTRL on IoControl and some cleanups xbmc/vfs.sftp#51

Merged

AlwinEsch force-pushed the change-vfs branch from 0d4048d to 9b29e3d Compare March 21, 2020 13:58

AlwinEsch mentioned this pull request Mar 22, 2020

[Matrix] change to use on newest Kodi Version xbmc/vfs.smb2#11

Merged

AlwinEsch commented Mar 23, 2020

View reviewed changes

xbmc/addons/AddonBindings.cmake Outdated Show resolved Hide resolved

AlwinEsch force-pushed the change-vfs branch from 9b29e3d to 36d92a0 Compare March 23, 2020 21:58

AlwinEsch mentioned this pull request Mar 25, 2020

[settings] allow TranslatableIntegerSettingOption to get string from addon #17555

Merged

13 tasks

AlwinEsch added 4 commits March 27, 2020 14:25

[addons][vfs] clang style cleanup on addon header

8494c2a

[addons][vfs] add "#ifdef __cplusplus" to addon header

4ae8eea

Added to separate "C" and "C++" part and further to allow a "C" only in future.

AlwinEsch added 4 commits March 27, 2020 14:25

[addons][vfs] make all "C" interface structures to own memory

67f8140

This done to prevent API backward problems if something becomes replaced e.g. on props or to kodi calls.

[addons][vfs] change ABI interface to match pure "C"

fd38f56

This changes the interface to pure "C" and no "C++" dependencies in it.

[addons][vfs] rework documentation

8d52f4c

With this change the whole VFS documentation becomes reworked and to have usable on doxygen. There are much parts added to understand his usage.

[addons][vfs] increase API version to 2.3.0

26ab79e

AlwinEsch force-pushed the change-vfs branch from 36d92a0 to 26ab79e Compare March 27, 2020 13:25

AlwinEsch merged commit 1e627d2 into xbmc:master Mar 28, 2020

AlwinEsch deleted the change-vfs branch March 28, 2020 19:36

Maven85 pushed a commit to Maven85/kodi that referenced this pull request Mar 30, 2020

Merge pull request xbmc#17527 from AlwinEsch/change-vfs

3d21370

[addons][vfs] cleanup, "C" interface fix, documentation rework

Maven85 pushed a commit to Maven85/kodi that referenced this pull request Mar 30, 2020

Merge pull request xbmc#17527 from AlwinEsch/change-vfs

8bde5b8

[addons][vfs] cleanup, "C" interface fix, documentation rework

mkreisl mentioned this pull request Apr 5, 2020

Build broken xbmc/vfs.sftp#53

Open

Maven85 pushed a commit to Maven85/kodi that referenced this pull request May 5, 2020

Merge pull request xbmc#17527 from AlwinEsch/change-vfs

58f9d4c

[addons][vfs] cleanup, "C" interface fix, documentation rework

Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 3, 2020

Merge pull request xbmc#17527 from AlwinEsch/change-vfs

307664b

[addons][vfs] cleanup, "C" interface fix, documentation rework

Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 4, 2020

Merge pull request xbmc#17527 from AlwinEsch/change-vfs

b01e00b

[addons][vfs] cleanup, "C" interface fix, documentation rework

Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 4, 2020

Merge pull request xbmc#17527 from AlwinEsch/change-vfs

d701e14

[addons][vfs] cleanup, "C" interface fix, documentation rework

Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 5, 2020

Merge pull request xbmc#17527 from AlwinEsch/change-vfs

89c4a8c

[addons][vfs] cleanup, "C" interface fix, documentation rework

Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 6, 2020

Merge pull request xbmc#17527 from AlwinEsch/change-vfs

8af52f6

[addons][vfs] cleanup, "C" interface fix, documentation rework

Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 6, 2020

Merge pull request xbmc#17527 from AlwinEsch/change-vfs

75f0971

[addons][vfs] cleanup, "C" interface fix, documentation rework

Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 6, 2020

Merge pull request xbmc#17527 from AlwinEsch/change-vfs

c5226a8

[addons][vfs] cleanup, "C" interface fix, documentation rework

Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 7, 2020

Merge pull request xbmc#17527 from AlwinEsch/change-vfs

bcfeea2

[addons][vfs] cleanup, "C" interface fix, documentation rework

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[addons][vfs] cleanup, "C" interface fix, documentation rework #17527

[addons][vfs] cleanup, "C" interface fix, documentation rework #17527

AlwinEsch commented Mar 20, 2020 •

edited

Loading

garbear Mar 21, 2020

AlwinEsch Mar 21, 2020

garbear Mar 21, 2020

AlwinEsch Mar 21, 2020

garbear Mar 21, 2020

AlwinEsch Mar 21, 2020 •

edited

Loading

garbear Mar 21, 2020

garbear commented Mar 21, 2020

AlwinEsch commented Mar 27, 2020

[addons][vfs] cleanup, "C" interface fix, documentation rework #17527

[addons][vfs] cleanup, "C" interface fix, documentation rework #17527

Conversation

AlwinEsch commented Mar 20, 2020 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of change

Checklist:

garbear Mar 21, 2020

Choose a reason for hiding this comment

AlwinEsch Mar 21, 2020

Choose a reason for hiding this comment

garbear Mar 21, 2020

Choose a reason for hiding this comment

AlwinEsch Mar 21, 2020

Choose a reason for hiding this comment

garbear Mar 21, 2020

Choose a reason for hiding this comment

AlwinEsch Mar 21, 2020 • edited Loading

Choose a reason for hiding this comment

garbear Mar 21, 2020

Choose a reason for hiding this comment

garbear commented Mar 21, 2020

AlwinEsch commented Mar 27, 2020

AlwinEsch commented Mar 20, 2020 •

edited

Loading

AlwinEsch Mar 21, 2020 •

edited

Loading