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

[addons][filesystem][base] add some new functions, improvement, cleanups and doc rework #17813

Merged
merged 28 commits into from
May 18, 2020

Conversation

AlwinEsch
Copy link
Member

@AlwinEsch AlwinEsch commented May 7, 2020

Description

This request was primary thought to update the filesystem parts, but as mostly all addons use them, need also all a update.

For this reason is the request a bit extended to add also other parts, where also related to all addons and to reduce the extreme big update rounds on all 100 addons.
This additional added parts are mostly minor things where not breaks the operation (related to working ways on addon itself).

Also some things why it looks bigger are clang cleanups on the headers and separation to extra "C" files.

Some parts added related to inpustream addons, then fixes about #17514, make the doxygen a bit cleaner.

NOTE: If nothing special comes, should after them the addon main version (ADDON_GLOBAL_VERSION_MAIN_MIN) hopefully a bit longer without changes.
Thats why some parts combined in this request, to make all to them on one time.

Docs visible here:

Commit 1:

[addons][filesystem] add function IsOpen to CFile

This thought to allow other place a check that related file
was really opened.

Commit 2:

[addons][filesystem] replace __stat64 with STAT_STRUCTURE

This done to have it more OS independent, before was about time in
not complete compatible between.

Commit 3:

[addons][filesystem] make interface "C" conform

Before was it not in correct "C" ABI between Kodi and Addon.
This add #ifdef about and change some parts on "C" side to allow
compile there.

Commit 4:

[addons][filesystem] cleanup namespace kodi::addon use

before was on every place this where makes the lines very long,
this change it to "using namespace kodi::addon" everywhere, where
reduce the size.

Commit 5:

[addons][filesystem] clang code cleanup

this cleanup the parts to match his style

Commit 6:

[addons][filesystem] cleanup documentation

This makes it cleaner and easier on review. Further are the parts
placed in groups.

Commit 7:

[addons][filesystem] add IoControl functions

Following are added:

  • bool IoControlGetSeekPossible()
  • bool IoControlGetCacheStatus(CacheStatus& status)
  • bool IoControlSetCacheRate(unsigned int rate)
  • bool IoControlSetRetry(bool retry)

Commit 8:

[addons][filesystem] add some functions to check file path relate to web

Following functions are added:

  • bool kodi::vfs::IsInternetStream(const std::string& path, bool strictCheck = false);
  • bool kodi::vfs::IsOnLAN(const std::string& path);
  • bool kodi::vfs::IsRemote(const std::string& path);
  • bool kodi::vfs::IsLocal(const std::string& path);
  • bool kodi::vfs::IsURL(const std::string& path);

Commit 9:

[addons][filesystem] separate "C" and "C++" parts of headers

By them becomes the "C" part placed in own header to see better what language
is where and to make easier for other languages.

Commit 10:

[addons][filesystem] add more functions to get web content

Following are added:

  • GetHttpHeader
    • To handle them "class HttpHeader"
  • GetMimeType
  • GetContentType
  • GetCookies

Commit 11:

[addons[filesystem] add translator for file read bits

This also add ADDON_ on begin on addon flags.
There three reasons about them:

  1. Prevent conflicts with Kodi's one where defined with "static const unsigned int".
  2. To have safe in case Kodi becomes changed and addon overseen.
  3. To match the enum begin all there with ADDON_

Commit 12:

[addons][filesystem] add #undef for CreateDirectory and DeleteFile

There about is on Windows macros where bring in this header conflict.
This undefine it and allow the use within kodi::vfs system.

Commit 13:

[addons][network] remove use of KODI_HANDLE on <kodi/c-api/network.h>

This allow then also remove use of #include "AddonBase.h" there and
make "C" API checks more easy and prevent not needed compile parts.

Commit 14:

[addons][base] fix possible memory overrun on kodi::Log(...) call

There was before "vsprintf" used, this now changed to "vsnprintf", where
his available array size is given to him.

Commit 15:

[addons][base] rename ADDON_LOG_... to LOG_... and combine with PVR

This remove the old and rename the new with remove of ADDON_ at begin.

Commit 16:

[addons][base] add documentation about "kodi::Log(...)" function

Commit 17:

[addons][base] move all "C" define's from AddonBase.h to addon_base.h

This moves it from C++ header to new added right "C" header.

Commit 18:

[addons][base] move ADDON_STATUS from AddonBase.h to addon_base.h

Also here it is to see as a "C" part, where better on this header.

Commit 19:

[addons] use "clang format off" on dependency check place in versions.h

The main reason, because cmake uses this area in this form to perform its
addon dependency check.

Commit 20:

[addons][base] don't pass C++ part inside AddonGlobalInterface

This change the class inserted there to KODI_HANDLE where mean void*.

Before with C++ class inside them it was not usable on "C" alone and
also not match a ABI guideline where only be "C" between two independent
parts.

Commit 21:

[addons][base] separate now all C parts complete to addon_base.h

Commit 22:

[addons][base] clang and sort cleanups on AddonBase.h

This change his code to match complete the clang design.

Commit 23:

[addons] add const C_STRUCT* GetCStructure() const and operator const C_STRUCT*() const

This added to allow in another class where use CStructHdl to get this from his values.

Commit 24:

[addons] add DestroyInstance function to addon base class

This thought to inform related addon that a instance becomes
destroyed.

Commit 25:

[addons][general] add function to check another addon is available

This thought for cases as another addon (e.g. PVR) need a special
inputstream addon.

Commit 26:

[addons] allow ask about Kodi's used addon type version and cleanup Dll

This add a new interface function where the addon can ask about version
in Kodi. Thought to hold easier for checks in other places of headers.

Further as by this request, an increase of all versions needed, are the exported
"C" functions updated and no more needed removed.

Commit 27:

[addons] fix open of addon settings dialog via addon

There was wrongly the "!" missing and always has broken the open.

Commit 28:

[addons] increase binary API versions

Motivation and Context

How Has This Been Tested?

All available addons created and nearly all runtime tested, where was OK on run.

Further one screensaver extended to call mostly all interface functions for tests.

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: Cleanup non-breaking change which removes non-working or unmaintained functionality Type: Improvement non-breaking change which improves existing functionality API change: Binary add-ons v19 Matrix Component: Documentation Component: FileSystem Filesystem labels May 7, 2020
@AlwinEsch AlwinEsch added this to the Matrix 19.0-alpha 1 milestone May 7, 2020
@AlwinEsch AlwinEsch requested a review from phunkyfish May 7, 2020 00:16
Copy link
Contributor

@phunkyfish phunkyfish left a comment

Choose a reason for hiding this comment

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

Looks good, just missing GetCookies() ;)

*numValues = values.size();
char **ret = static_cast<char**>(malloc(sizeof(char*)*values.size()));
char** ret = static_cast<char**>(malloc(sizeof(char*) * values.size()));
Copy link
Member

Choose a reason for hiding this comment

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

Another opportunity for new? I saw you made such a change above, just wondering

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense to use new, place there was come by clang cleanup and overseen it.

Must only learn how is on ** memory :-).

Copy link
Member Author

Choose a reason for hiding this comment

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

Is then char** ret = new char*[size]; for there, but can't used as his counterpart use free()
Here: https://github.com/xbmc/xbmc/blob/master/xbmc/addons/interfaces/AddonBase.cpp#L455-L465


static bool file_exists(void* kodiBase, const char* filename, bool useCache);
static int stat_file(void* kodiBase, const char* filename, struct __stat64* buffer);
static bool stat_file(void* kodiBase, const char* filename, struct STAT_STRUCTURE* buffer);
Copy link
Member

Choose a reason for hiding this comment

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

Is there another opportunity to change a stat's int return type to bool? I saw a comparison of a Stat() function against 0 above. Just wondering.

Copy link
Member Author

Choose a reason for hiding this comment

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

About them I change it complete, as void* is bad for other languages 😄.
Make it now with them:

    bool (*io_control_seek_possible)(void* kodiBase, void* file);
    bool (*io_control_get_cache_status)(void* kodiBase, void* file, struct VFS_IOCTRL_CACHE_STATUS_DATA* status);
    bool (*io_control_set_cache_rate)(void* kodiBase, void* file, unsigned int rate);
    bool (*io_control_set_retry)(void* kodiBase, void* file, bool retry);

@garbear
Copy link
Member

garbear commented May 8, 2020

No problems jump out at me

@AlwinEsch AlwinEsch force-pushed the rework-filesystem branch 2 times, most recently from a7d1f18 to 8c5e819 Compare May 14, 2020 20:53
@AlwinEsch AlwinEsch changed the title [addons][filesystem] add some new functions, improvement, cleanups and doc rework [addons][filesystem][base] add some new functions, improvement, cleanups and doc rework May 14, 2020
@AlwinEsch
Copy link
Member Author

Is updated and also now include some base addon parts with cleanups. As with the first parts must every addon updated, makes it sense to include also the other where need the same.

…nst C_STRUCT*() const"

This added to allow in another class where use CStructHdl to get this from his values.
This thought to inform related addon that a instance becomes
destroyed.
@AlwinEsch
Copy link
Member Author

Addon builds was all correct https://jenkins.kodi.tv/job/BuildMultiWithAddons-PR/64/, test patch becomes now removed.

Copy link
Member

@Rechi Rechi left a comment

Choose a reason for hiding this comment

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

Please follow the current code guidelines.

xbmc/addons/interfaces/General.cpp Outdated Show resolved Hide resolved
xbmc/addons/interfaces/General.cpp Outdated Show resolved Hide resolved
This add a new interface function where the addon can ask about version
in Kodi. Thought to hold easier for checks in other places of headers.

Further as by this request, an increase of all versions needed, are the exported
"C" functions updated and no more needed removed.
This thought for cases as another addon (e.g. PVR) need a special
inputstream addon.
There was wrongly the "!" missing and always has broken the open.
As this changes related to all places here increase of all.
Thats also why request heavier to make addon updates for all
only one time.
@phunkyfish
Copy link
Contributor

Jenkins build this please

@AlwinEsch
Copy link
Member Author

Jenkins build this please

There need jenkins a fix 😒 😺, a lot of times fails WIN32 or WIN64 with

  • Could Not Find f:\builds\workspace\WIN-[32][64]\gtestresults.xml

Lets try again 😏

@phunkyfish
Copy link
Contributor

Jenkins build this please

1 similar comment
@phunkyfish
Copy link
Contributor

Jenkins build this please

@AlwinEsch
Copy link
Member Author

The addon update round can begin.
The jenkins error is unrelated as it is on other Linux parts OK and also the 5 builds was all OK, only bug in Jenkins itself.

@AlwinEsch AlwinEsch merged commit 4545516 into xbmc:master May 18, 2020
Maven85 pushed a commit to Maven85/kodi that referenced this pull request May 19, 2020
[addons][filesystem][base] add some new functions, improvement, cleanups and doc rework
@AlwinEsch AlwinEsch deleted the rework-filesystem branch May 20, 2020 13:13
@basilgello
Copy link
Collaborator

@ksooo unmerged DigitalDevices/pvr.octonet#42 breaks the pipeline ;)

Maven85 pushed a commit to Maven85/kodi that referenced this pull request Jun 15, 2020
[addons][filesystem][base] add some new functions, improvement, cleanups and doc rework
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 3, 2020
[addons][filesystem][base] add some new functions, improvement, cleanups and doc rework
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 4, 2020
[addons][filesystem][base] add some new functions, improvement, cleanups and doc rework
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 4, 2020
[addons][filesystem][base] add some new functions, improvement, cleanups and doc rework
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 5, 2020
[addons][filesystem][base] add some new functions, improvement, cleanups and doc rework
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 5, 2020
[addons][filesystem][base] add some new functions, improvement, cleanups and doc rework
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 6, 2020
[addons][filesystem][base] add some new functions, improvement, cleanups and doc rework
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 6, 2020
[addons][filesystem][base] add some new functions, improvement, cleanups and doc rework
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 6, 2020
[addons][filesystem][base] add some new functions, improvement, cleanups and doc rework
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 7, 2020
[addons][filesystem][base] add some new functions, improvement, cleanups and doc rework
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API change: Binary add-ons Component: Documentation Component: FileSystem Filesystem Type: Cleanup non-breaking change which removes non-working or unmaintained functionality Type: Improvement non-breaking change which improves existing functionality v19 Matrix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants