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

Added PVR.EpgEventIcon guiinfo label #14372

Merged
merged 1 commit into from
Aug 29, 2018
Merged

Added PVR.EpgEventIcon guiinfo label #14372

merged 1 commit into from
Aug 29, 2018

Conversation

lightglitch
Copy link
Contributor

Description

Similar to #14361 but for the VideoPlayer

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

@sualfred
Copy link
Member

Shouldn't it be mapped to the PVR.xxx values where all other PVR related stuff is stored?

const infomap pvr[] = {{ "isrecording", PVR_IS_RECORDING },

@lightglitch
Copy link
Contributor Author

I don't think so, this should work similar to the VideoPlayer.Plot API, it's not for the PVR.

Copy link
Member

@ksooo ksooo left a comment

Choose a reason for hiding this comment

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

Not Videoplayer.EpgEventIcon, but PVR.EpgEventItem. Videoplayer knows nothing about EPG.

@lightglitch
Copy link
Contributor Author

@ksooo but you also have epg info (related to pvr) on videoplayer like:

VIDEOPLAYER_HAS_EPG
VIDEOPLAYER_NEXT_PLOT
VIDEOPLAYER_NEXT_STARTTIME

///   \table_row3{   <b>`VideoPlayer.HasEpg`</b>,
///                  \anchor VideoPlayer_HasEpg
///                  _boolean_,
///     Returns true if epg information is available for the currently playing
///     programme (PVR).
///   }
///   \table_row3{   <b>`VideoPlayer.NextPlot`</b>,
///                  \anchor VideoPlayer_NextPlot
///                  _string_,
///     Plot of the programme that will be played next (PVR).
///   \table_row3{   <b>`VideoPlayer.NextStartTime`</b>,
///                  \anchor VideoPlayer_NextStartTime
///                  _string_,
///     Start time of the programme that will be played next (PVR).
///   }

@ksooo
Copy link
Member

ksooo commented Aug 29, 2018

but you also have epg info (related to pvr) on videoplayer like

How about trusting me a little bit? I know this code for a couple of years now. Well, all labels related to EPG are subject to get moved to PVR. They were added to VideoPlayer scope at times were we did not have a sharp eye on software architecture...

Plot and starttime are generic properties, valid for both 'normal' videos and PVR. Thus, PVR 'overrides' the generic properties.

@lightglitch lightglitch changed the title Added VideoPlayer.EpgEventIcon API Added PVR.EpgEventIcon API Aug 29, 2018
@lightglitch
Copy link
Contributor Author

@ksooo Request changes made.

Copy link
Member

@ksooo ksooo left a comment

Choose a reason for hiding this comment

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

Only some minors.

/// \table_row3{ <b>`PVR.EpgEventIcon`</b>,
/// \anchor PVR_EpgEventIcon
/// _string_,
/// Returns the thumbnail for the epg event associated with the item (if it exists)

This comment was marked as spam.

@@ -593,6 +593,7 @@
#define PVR_TIMESHIFT_PROGRESS_START_TIME (PVR_STRINGS_START + 70)
#define PVR_TIMESHIFT_PROGRESS_END_TIME (PVR_STRINGS_START + 71)
#define PVR_STRINGS_END PVR_TIMESHIFT_PROGRESS_END_TIME
#define PVR_EPG_EVENT_ICON (PVR_STRINGS_START + 72)

This comment was marked as spam.

@ksooo
Copy link
Member

ksooo commented Aug 29, 2018

Just a comment regarding the wording: Added PVR.EpgEventIcon API

What you are doing in Kodi terminology is adding a guiinfo label, not an API.

@lightglitch lightglitch changed the title Added PVR.EpgEventIcon API Added PVR.EpgEventIcon guiinfo label Aug 29, 2018
@lightglitch
Copy link
Contributor Author

Review requests implemented.

Copy link
Contributor

@FernetMenta FernetMenta left a comment

Choose a reason for hiding this comment

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

What @ksooo says

Copy link
Member

@ksooo ksooo left a comment

Choose a reason for hiding this comment

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

Nice, thanks.

@ksooo
Copy link
Member

ksooo commented Aug 29, 2018

jenkins build this please

@ksooo ksooo added this to the Leia 18.0-beta2 milestone Aug 29, 2018
@ksooo
Copy link
Member

ksooo commented Aug 29, 2018

@sualfred fyi

@ksooo ksooo merged commit 937c247 into xbmc:master Aug 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants