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

[PVR] Refactor PVR jobs. Get rid of PVRJobs.(h|cpp). #16698

Merged
merged 1 commit into from Oct 2, 2019

Conversation

ksooo
Copy link
Member

@ksooo ksooo commented Oct 2, 2019

Refactoring of PVR jobs. Basic ideas: Use lambda jobs to reduce boilerplate code and better encapsulation by reducing visibility of the job implementations.

Runtime-tested on macOS and Android, latest Kodi master.

@ksooo ksooo added Type: Cleanup non-breaking change which removes non-working or unmaintained functionality Component: PVR v19 Matrix labels Oct 2, 2019
@ksooo ksooo added this to the Matrix 19.0-alpha 1 milestone Oct 2, 2019
@ksooo ksooo requested a review from phunkyfish October 2, 2019 07:25
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.

Just a couple of things

xbmc/pvr/CMakeLists.txt Outdated Show resolved Hide resolved
xbmc/pvr/PVREventlogJob.cpp Outdated Show resolved Hide resolved
xbmc/pvr/PVREventlogJob.h Outdated Show resolved Hide resolved
xbmc/pvr/PVRGUIChannelNavigator.cpp Outdated Show resolved Hide resolved
xbmc/pvr/PVRGUIChannelNavigator.cpp Outdated Show resolved Hide resolved
xbmc/pvr/PVRManager.cpp Outdated Show resolved Hide resolved
@ksooo
Copy link
Member Author

ksooo commented Oct 2, 2019

@Rechi @phunkyfish all requested/suggested changes are in. Please comment or approve.

@ksooo
Copy link
Member Author

ksooo commented Oct 2, 2019

jenkins build this please

std::string m_icon;

Event(bool bNotifyUser, bool bError, const std::string& label, const std::string& msg, const std::string& icon)
: m_bNotifyUser(bNotifyUser), m_bError(bError), m_label(label), m_msg(msg), m_icon(icon) {}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
: m_bNotifyUser(bNotifyUser), m_bError(bError), m_label(label), m_msg(msg), m_icon(icon) {}
: m_bNotifyUser(bNotifyUser)
, m_bError(bError)
, m_label(label)
, m_msg(msg)
, m_icon(icon)
{
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope. This looks so ugly that I will not accept this coding guideline.

Copy link
Member

Choose a reason for hiding this comment

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

As I already told you, if you do not like the style submit a PR that changes the docs and clang-format configuration.

public:
CPVRChannelTimeoutJobBase() = delete;
CPVRChannelTimeoutJobBase(PVR::CPVRGUIChannelNavigator& channelNavigator, int iTimeout)
: m_channelNavigator(channelNavigator)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
: m_channelNavigator(channelNavigator)
: m_channelNavigator(channelNavigator)

Copy link
Member Author

Choose a reason for hiding this comment

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

No.

{
public:
CPVRChannelEntryTimeoutJob(PVR::CPVRGUIChannelNavigator& channelNavigator, int iTimeout)
: CPVRChannelTimeoutJobBase(channelNavigator, iTimeout) {}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
: CPVRChannelTimeoutJobBase(channelNavigator, iTimeout) {}
: CPVRChannelTimeoutJobBase(channelNavigator, iTimeout)
{
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope.

{
public:
CPVRChannelInfoTimeoutJob(PVR::CPVRGUIChannelNavigator& channelNavigator, int iTimeout)
: CPVRChannelTimeoutJobBase(channelNavigator, iTimeout) {}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
: CPVRChannelTimeoutJobBase(channelNavigator, iTimeout) {}
: CPVRChannelTimeoutJobBase(channelNavigator, iTimeout)
{
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Never.

{
public:
CPVRLambdaJob() = delete;
CPVRLambdaJob(const std::string& type, F&& f) : m_type(type), m_f(std::forward<F>(f)) {}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
CPVRLambdaJob(const std::string& type, F&& f) : m_type(type), m_f(std::forward<F>(f)) {}
CPVRLambdaJob(const std::string& type, F&& f)
: m_type(type)
, m_f(std::forward<F>(f))
{
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Once more, sorry no.

class CPVRManagerJobQueue
{
public:
CPVRManagerJobQueue() : m_triggerEvent(false) {}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
CPVRManagerJobQueue() : m_triggerEvent(false) {}
CPVRManagerJobQueue()
: m_triggerEvent(false)
{
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Nonono. ;-)

@phunkyfish
Copy link
Contributor

Looks good 😉

@ksooo ksooo merged commit a1b36fd into xbmc:master Oct 2, 2019
@ksooo ksooo deleted the pvr-refactor-jobs branch October 2, 2019 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: PVR Type: Cleanup non-breaking change which removes non-working or unmaintained functionality v19 Matrix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants