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] change binary interface (webbrowser required parts) #16401

Merged
merged 5 commits into from
Aug 6, 2019

Conversation

AlwinEsch
Copy link
Member

@AlwinEsch AlwinEsch commented Jul 24, 2019

Description

The first 4 commits are include changes where required on the webbrowser.

Commits:

  1. To include own keyboard GUI on browser addon, also must be the keypress there direct handled and not on end after a OK (the from Kodi where block the whole screen is not direct usable)
  2. Only fix warnings where I see on windows
  3. This add on the GUI action callback also the give of buttoncode and unicode where needed to handle actions on webbrowser web. Further brings it two functions to set control visible or to select
  4. Is on audioengine where it can possible that audioengine is stopped and destructed and addon still try to call them, this check the engine is present before call
  5. Only version increases, this is then also related to [addon] fix ios runtime failure after PR 14908 #16397 where a small piece on interface becomes changed

Motivation and Context

To bring this hell of pain with webbrowser finally in Kodi 😅😏

This is only a independent part of them and I hope to have the browser parts soon final.

How Has This Been Tested?

Screenshots (if appropriate):

Here about the keyboard and action calls:
Screenshot_20190724_234454

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 changed the title [addons] change binary interface with webbrowser required parts [addons] change binary interface (webbrowser required parts) Jul 25, 2019
@AlwinEsch AlwinEsch added the Type: Feature non-breaking change which adds functionality label Jul 25, 2019
@@ -727,3 +752,124 @@ inline void KodiVersion(kodi_version_t& version)
}
} /* namespace kodi */
//------------------------------------------------------------------------------

//==============================================================================
namespace kodi {
Copy link
Member

Choose a reason for hiding this comment

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

move curly bracket into a new line

Copy link
Member Author

@AlwinEsch AlwinEsch Jul 26, 2019

Choose a reason for hiding this comment

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

Normally I would say yes and no problem, but I see it a little different with the addon heads. I know, it not match Code Guidelines!

Once the lowercase namespace to keep apart between Kodi page and addon page.
On the other hand, the namespace kodi { is added to each function, so when looking over the header an outsider sees directly what is needed.

If the namespace were just like in Kodi at the beginning of a file, the linebreak would make sense, but not there for me! If a line break is added to each namespace of a function or structure (maybe over 100 in future), the number of lines increases unnecessarily.

Copy link
Member

Choose a reason for hiding this comment

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

Why is the kodi namespace closed and opened immediately afterwards again?
Please do use custom code style, without adding it to code guidelines and clang-format.

Copy link
Member Author

Choose a reason for hiding this comment

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

One to see that it is an independent function and not within a class. The huge documentary can be damn long and could be overlooked.

The other is that under circumstances a "C" interface structure could come in between which should not be in a namespace.

In order to have the addon interface as inline it is not always possible to use the guidelines correctly.

//------------------------------------------------------------------------------

//==============================================================================
namespace kodi {
Copy link
Member

Choose a reason for hiding this comment

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

move curly bracket into a new line

@AlwinEsch AlwinEsch merged commit 2b14719 into xbmc:master Aug 6, 2019
@AlwinEsch AlwinEsch deleted the change-interface branch August 6, 2019 21:11
@ksooo ksooo added the Type: Breaking change fix or feature that will cause existing functionality to change label Aug 7, 2019
ksooo added a commit to ksooo/xbmc that referenced this pull request Aug 7, 2019
@AlwinEsch AlwinEsch restored the change-interface branch August 8, 2019 08:58
@AlwinEsch AlwinEsch deleted the change-interface branch August 12, 2019 04:25
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: Binary add-ons 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
Development

Successfully merging this pull request may close these issues.

3 participants