change season/episode infolabels for specials

[ronie]

the PR removes the odd season/episode numbering we use in infolabels for episodes that are specials.

before:

ListItem.Season:
ListItem.Episode: S1

after:

ListItem.Season: 0
ListItem.Episode: 1

(the same change applies to VideoPlayer.Season / VideoPlayer.Episode)

fixes #17402

for skinners / addon devs it's much easier to work with those infolabels this way.

[emveepee]

@razzeee , @ronie , @ksooo This is impacting PVR recordings. The documentation says S0+E0 is ok for invalid for PVR Recordings but they are now displayed as S0 because of the = test
https://github.com/xbmc/xbmc/blob/master/xbmc/guilib/guiinfo/VideoGUIInfo.cpp#L236

[ronie]

i'm not really familiar with pvr, can you please point me to this documentation?

[emveepee]

i'm not really familiar with pvr, can you please point me to this documentation?

https://codedocs.xyz/AlwinEsch/kodi/struct_p_v_r___r_e_c_o_r_d_i_n_g.html#afe2e4edaf0b7589e5a464a543b7f1cda

[ronie]

thanx!

i've only now found out that PVR uses their own code for season / episode infolabels:

xbmc/xbmc/pvr/guilib/guiinfo/PVRGUIInfo.cpp

Lines 573 to 583 in dfad22e

	case VIDEOPLAYER_EPISODE:
	case LISTITEM_EPISODE:
	if (epgTag->EpisodeNumber() > 0)
	{
	if (epgTag->SeriesNumber() == 0) // prefix episode with 'S'
	strValue = StringUtils::Format("S%i", epgTag->EpisodeNumber());
	else
	strValue = StringUtils::Format("%i", epgTag->EpisodeNumber());
	return true;
	}
	return false;

xbmc/xbmc/pvr/guilib/guiinfo/PVRGUIInfo.cpp

Lines 565 to 572 in dfad22e

	case VIDEOPLAYER_SEASON:
	case LISTITEM_SEASON:
	if (epgTag->SeriesNumber() > 0)
	{
	strValue = StringUtils::Format("%i", epgTag->SeriesNumber());
	return true;
	}
	return false;

since i haven't touched that part, i wonder how this PR could have an effect on PVR...

[emveepee]

It is cosmetic but looks odd on non-series PVR recordings. I expect the problem is worse for addons that use legacy code and memset the API returns.

I do have another concern about how this might cause user confusion when it is properly used on a special when there is no Episode name.

[ksooo]

PVR has own code part for EPG, but uses videoinfotag code path for recordings. Thus, changes in the video path can affect recordings.

[ksooo]

The documentation says S0+E0 is ok for invalid for PVR Recordings but they are now displayed as S0 because of the = test

I suggest to change PVR API to not allow 0/0 to signal "not available" any longer. This means, to update the API documentation and to adapt all PVR add-ons.

As I'm not available for doing this during the next couple of months due to time constraints, somebody else would have to step up.

[emveepee]

I suggest to change PVR API to not allow 0/0 to signal "not available" any longer. This means, to update the API documentation and to adapt all PVR add-ons.

As I'm not available for doing this during the next couple of months due to time constraints, somebody else would have to step up.

@ksooo is that feasible without knowing what the backend is sending? I checked pvr.hts and I see

  /* season/episode (tvh 4.3+) */
  rec.iSeriesNumber = recording.GetSeason();
  rec.iEpisodeNumber = recording.GetEpisode();

Rather than adding a condition to each PVR could we not just modify how they are copied https://github.com/xbmc/xbmc/blob/master/xbmc/pvr/recordings/PVRRecording.cpp#L78 to

if (recording.iSeriesNumber!=0 || recording.iEpisodeNumber!=0)
{
    m_iSeason = recording.iSeriesNumber;
    m_iEpisode = recording.iEpisodeNumber;
}

Also is memset required on any of the PVR types functions at this time? If not I could flag issues on the official addons that still use them (like mine) to remove it.

[ksooo]

is that feasible without knowing what the backend is sending? I checked pvr.hts and I see

It is feasible for the maintainer of the soon, yes. If there is no maintainer, the add-on will die, former or later.

Rather than adding a condition to each PVR could we not just modify how they are copied

We should fix the route cause, which is bad API design here, not hack around the actual problem, just because the latter is less effort.

Also is memset required on any of the PVR types functions at this time?

If the addon creates the struct, then it must initialize its members - 'memset' works for this, although better approach is 'TYPE foo = {};' - so there is no need to file issues for add-ons still using memset.

[emveepee]

Rather than adding a condition to each PVR could we not just modify how they are copied

We should fix the route cause, which is bad API design here, not hack around the actual problem, just because the latter is less effort.

Sorry I misunderstand your first comment I thought you wanted someone other than the maintainer to make this change. The recent update to pvr.hts shows how difficult that would be.

My comments about memset were more about how to factor this change in with PR1645 looming but now I see that likely won't be a Matrix issues anyway. I think we can close this discussion here as I opened a PR for your recommendation.