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] add gui control classes to new addon interface #12259

Merged
merged 1 commit into from Jun 8, 2017

Conversation

@AlwinEsch
Copy link
Member

AlwinEsch commented Jun 7, 2017

Description

This change alow addons to edit all used controls on his processed window.

For the moment is this mostly the last request to create the global addon
interface function.

Most further requests are to change addon types iself to new style.

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

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 this to the L 18.0-alpha1 milestone Jun 7, 2017
@AlwinEsch AlwinEsch force-pushed the AlwinEsch:add-callbacks-7 branch from 1693350 to 0710de4 Jun 7, 2017
Copy link
Member

ksooo left a comment

5000+ lines of code to review. That was hard candy. You ow me a beer at next Devcon, @AlwinEsch :-)

return nullptr;
}

return pAddonWindow->GetAddonControl(control_id, CGUIControl::GUICONTROL_TEXTBOX, "textbox");

This comment has been minimized.

Copy link
@ksooo

ksooo Jun 7, 2017

Member

Most of the method implementations in this source file are almost identical. May I suggest to factor out the common functionality. Something like the following would do the trick and significantly reduce the size of this source file:

void* Interface_GUIWindow::get_control(void* kodiBase, void* handle, int control_id, const char* function, CGUIControl::GUICONTROLTYPES type, std::string typeName)
{
  CAddonDll* addon = static_cast(kodiBase);
  CGUIAddonWindow* pAddonWindow = static_cast(handle);
  if (!addon || !pAddonWindow)
  {
    CLog::Log(LOGERROR, "Interface_GUIWindow::%s - invalid handler data (kodiBase='%p', handle='%p') on addon '%s'",
                          function, addon, handle, addon ? addon->ID().c_str() : "unknown");
    return nullptr;
  }

  return pAddonWindow->GetAddonControl(control_id, type, typeName);
}

And change the implementations like this (example):

void* Interface_GUIWindow::get_control_text_box(void* kodiBase, void* handle, int control_id)
{
  return get_control(kodiBase, handle, control_id, __FUNCTION__, CGUIControl::GUICONTROL_TEXTBOX, "textbox");
}

This comment has been minimized.

Copy link
@AlwinEsch

AlwinEsch Jun 7, 2017

Author Member

If this is not problematic with a static_cast, looks it really better.

This comment has been minimized.

Copy link
@AlwinEsch

AlwinEsch Jun 7, 2017

Author Member

Forget my comment and becomes changed, thanks a lot!

static char* get_text(void* kodiBase, void* handle);

static unsigned int get_cursor_position(void* kodiBase, void* handle);
static void set_cursor_position(void* kodiBase, void* handle, unsigned int position);

This comment has been minimized.

Copy link
@ksooo

ksooo Jun 7, 2017

Member

cosmetics: swap the above two lines, for consistency reasons (first the setter, then the getter). At least you used this order for the other read-write properties in this header.

static void set_enabled(void* kodiBase, void* handle, bool enabled);
static void set_selected(void* kodiBase, void* handle, bool selected);

static void add_label(void* kodiBase, void* handle, const char* label);

This comment has been minimized.

Copy link
@ksooo

ksooo Jun 7, 2017

Member

Question: Why "add", not "set"? Can this control have multiple labels and this functions adds another one? If it can only have max. one label, I suggest to name this function "set_label".

In this PR, you introduce other methods/functions having "add" in it's name. Same question applies to all this cases.

This comment has been minimized.

Copy link
@AlwinEsch

AlwinEsch Jun 7, 2017

Author Member

It can have more strings in a list, for them is the set_scrolling and reset also used.

This comment has been minimized.

Copy link
@ksooo

ksooo Jun 8, 2017

Member

Ah, okay then.

control->SetVisible(visible);
}

void Interface_GUIControlImage::set_filename(void* kodiBase, void* handle, const char* filename, const bool use_Cache)

This comment has been minimized.

Copy link
@ksooo

ksooo Jun 7, 2017

Member

"const bool" makes no real sense. remove the "const" before "bool", please.

Also, the naming scheme used for the last parameter (the underscore in the middle of the name) is quite unusual. I would just name it "useCache".

This comment has been minimized.

Copy link
@AlwinEsch

AlwinEsch Jun 7, 2017

Author Member

Thought to have all removed, thank you. It flies out.

With the "use_cache" has I used to have more equal to "C" name style (has wrongly set one character as uppercase).

Only leaved everywhere the "kodiBase" as sign for me that a C++ base is behind.

return;
}

// create message

This comment has been minimized.

Copy link
@ksooo

ksooo Jun 7, 2017

Member

Remove this comment, please. If somebody needs this comment to understand the following lines of code, he's just, well ..., hmpf ;-)

There are other occurrences of exactly this useless comment in this PR. Fix all of it, please.

{
CLog::Log(LOGERROR, "Interface_GUIControlSlider::%s - invalid handler data (kodiBase='%p', handle='%p') on addon '%s'",
__FUNCTION__, addon, control, addon ? addon->ID().c_str() : "unknown");
return 0;

This comment has been minimized.

Copy link
@ksooo

ksooo Jun 7, 2017

Member

"return -1;" to indicate error, like done in all (?) other methods returning int?

{
CAddonDll* addon = static_cast<CAddonDll*>(kodiBase);
CGUISpinControlEx* control = static_cast<CGUISpinControlEx*>(handle);
if (!addon || !control)

This comment has been minimized.

Copy link
@ksooo

ksooo Jun 7, 2017

Member

check parameter "text" for not being nullptr?

This comment has been minimized.

Copy link
@AlwinEsch

AlwinEsch Jun 7, 2017

Author Member

Like above, equal to my head and was lost in memory.

{
CLog::Log(LOGERROR, "Interface_GUIControlSpin::%s - invalid handler data (kodiBase='%p', handle='%p') on addon '%s'",
__FUNCTION__, addon, control, addon ? addon->ID().c_str() : "unknown");
return 0;

This comment has been minimized.

Copy link
@ksooo

ksooo Jun 7, 2017

Member

"return -1;"?

This comment has been minimized.

Copy link
@AlwinEsch

AlwinEsch Jun 7, 2017

Author Member

Not sure what's the best, has set it as 0 to have equal with float error return and is normally the default value if nothing becomes set before.

///
virtual ~CEdit()
{
}

This comment has been minimized.

Copy link
@ksooo

ksooo Jun 7, 2017

Member

One of my favorites (as you know): "virtual ~CEdit() = default;" ? Here and everywhere in this PR. ;-)

This comment has been minimized.

Copy link
@AlwinEsch

AlwinEsch Jun 7, 2017

Author Member

No problem becomes changed, is still new for me :-)

m_interface->kodi_gui->control_rendering->set_callbacks(m_interface->kodiBase, m_controlHandle, this,
OnCreateCB, OnRenderCB, OnStopCB, OnDirtyCB);
else
kodi::Log(ADDON_LOG_FATAL, "kodi::gui::controls::CRendering can't create control class from Kodi !!!");

This comment has been minimized.

Copy link
@ksooo

ksooo Jun 7, 2017

Member

Use FUNCTION here and at the other places where you hard coded the funtion name for logging?

@AlwinEsch

This comment has been minimized.

Copy link
Member Author

AlwinEsch commented Jun 7, 2017

Thanks a lot and the beer is ready.

@AlwinEsch AlwinEsch force-pushed the AlwinEsch:add-callbacks-7 branch from 0710de4 to c08e5ca Jun 7, 2017
Copy link
Member

ksooo left a comment

Just another small nitpick, the rest is looking very good now.

}
//@}

void* Interface_GUIWindow::GetControl(void* kodiBase, void* handle, int control_id, const char* function, CGUIControl::GUICONTROLTYPES type, std::string typeName)

This comment has been minimized.

Copy link
@ksooo

ksooo Jun 8, 2017

Member

"const std::string& typeName" - pass by value is inefficient.

static void set_enabled(void* kodiBase, void* handle, bool enabled);
static void set_selected(void* kodiBase, void* handle, bool selected);

static void add_label(void* kodiBase, void* handle, const char* label);

This comment has been minimized.

Copy link
@ksooo

ksooo Jun 8, 2017

Member

Ah, okay then.

This change alow addons to edit all used controls on his processed window.

For the moment is this mostly the last request to create the global addon
interface function.

Most further requests are to change addon types iself to new style.
@AlwinEsch AlwinEsch force-pushed the AlwinEsch:add-callbacks-7 branch from c08e5ca to 9ac43de Jun 8, 2017
@AlwinEsch

This comment has been minimized.

Copy link
Member Author

AlwinEsch commented Jun 8, 2017

jenkins build this please

@ksooo
ksooo approved these changes Jun 8, 2017
Copy link
Member

ksooo left a comment

Good to go.

@AlwinEsch AlwinEsch merged commit 07fc5e5 into xbmc:master Jun 8, 2017
1 check passed
1 check passed
default You're awesome. Have a cookie
Details
@AlwinEsch AlwinEsch deleted the AlwinEsch:add-callbacks-7 branch Jun 8, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.