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

Fix missing description in the EPG and DVR #401

Merged
merged 3 commits into from Feb 6, 2019

Conversation

@Verequies
Copy link
Contributor

commented Feb 4, 2019

Some channels store the description in the channel subtitle in HTSP v32+.
These commits fix the issue and update pvr.hts to 4.4.13.

Copy link
Member

left a comment

Please take care of my annotations and thanks for your contribution. As I'm not able to test this I have to trust you that it actually works.

This was done by TVHeadend prior to HTSP v32.
*/
else if (rec.GetDescription().empty() && m_conn->GetProtocol() >= 32)

This comment has been minimized.

Copy link
@ksooo

ksooo Feb 4, 2019

Member

Why else ifand not just if? you might have gotten empty desc or summary above...

*/
else if (rec.GetDescription().empty() && m_conn->GetProtocol() >= 32)
{
if ((str = htsmsg_get_str(msg, "subtitle")) != NULL) {

This comment has been minimized.

Copy link
@ksooo

ksooo Feb 4, 2019

Member

You already obtained subtitle some lines above, so there is no need to get it from the htsp msg a second time. => if (!rec.GetSubtitle().empty())

This was done by TVHeadend prior to HTSP v32.
*/
else if (evt.GetDesc().empty() && m_conn->GetProtocol() >= 32)

This comment has been minimized.

Copy link
@ksooo

ksooo Feb 4, 2019

Member

Why else if and not just if? you might have gotten empty desc above...

*/
else if (evt.GetDesc().empty() && m_conn->GetProtocol() >= 32)
{
if ((str = htsmsg_get_str(msg, "summary")) != NULL) {

This comment has been minimized.

Copy link
@ksooo

ksooo Feb 4, 2019

Member

if (!evt.GetSummary().empty())

evt.SetDesc(str);
evt.SetSummary("");
}
else if ((str = htsmsg_get_str(msg, "subtitle")) != NULL) {

This comment has been minimized.

Copy link
@ksooo

ksooo Feb 4, 2019

Member

if (!evt.GetSubtitle().empty()). You may have gotten an empty summary above.

This comment has been minimized.

Copy link
@Verequies

Verequies Feb 5, 2019

Author Contributor

Keeping the else as we don't want to override the summary as description with the subtitle as description. That would cause issues.

else if (evt.GetDesc().empty() && m_conn->GetProtocol() >= 32)
{
if ((str = htsmsg_get_str(msg, "summary")) != NULL) {
evt.SetDesc(str);

This comment has been minimized.

Copy link
@ksooo

ksooo Feb 4, 2019

Member

evt.SetDesc(evt.GetSummary())

evt.SetSummary("");
}
else if ((str = htsmsg_get_str(msg, "subtitle")) != NULL) {
evt.SetDesc(str);

This comment has been minimized.

Copy link
@ksooo

ksooo Feb 4, 2019

Member

evt.SetDesc(evt.GetSubtitle())

else if (rec.GetDescription().empty() && m_conn->GetProtocol() >= 32)
{
if ((str = htsmsg_get_str(msg, "subtitle")) != NULL) {
rec.SetDescription(str);

This comment has been minimized.

Copy link
@ksooo

ksooo Feb 4, 2019

Member

rec.SetDescription(evt.GetSubtitle())

@Jalle19

This comment has been minimized.

Copy link
Contributor

commented Feb 4, 2019

I'm wondering if it would make more sense to use the "default" HTSP fields first, then after all that there would be the protocol version check, which if true fixes up fields that need fixing? This thing is becoming a bit unreadable.

@ksooo

This comment has been minimized.

Copy link
Member

commented Feb 4, 2019

I agree.

@Verequies Verequies force-pushed the Verequies:master branch 2 times, most recently from dda7c4a to 6bd5dfd Feb 5, 2019
Copy link
Member

left a comment

I like meaningfule and correct comments. ;-)

@@ -2634,6 +2645,28 @@ bool CTvheadend::ParseEvent ( htsmsg_t *msg, bool bAdd, Event &evt )
evt.SetYear(u32);
if (!htsmsg_get_u32(msg, "dvrId", &u32))
evt.SetRecordingId(u32);

/* Extra checks for HTSP v32+ */

This comment has been minimized.

Copy link
@ksooo

ksooo Feb 5, 2019

Member

Please remove this comment. It is more then obvious what the following code line does.

@@ -2470,6 +2470,24 @@ void CTvheadend::ParseRecordingAddOrUpdate ( htsmsg_t *msg, bool bAdd )
if ((str = htsmsg_get_str(msg, "fanartImage")) != NULL)
rec.SetFanartImage(GetImageURL(str));

/* Extra checks for HTSP v32+ */

This comment has been minimized.

Copy link
@ksooo

ksooo Feb 5, 2019

Member

Please remove this comment. It is more then obvious what the following code line does.

entries to avoid duplicate information being displayed.
This was done by TVHeadend prior to HTSP v32.
*/

This comment has been minimized.

Copy link
@ksooo

ksooo Feb 5, 2019

Member

This comment block fits best after if (evt.GetDesc().empty()) {, not before. Please move the comment.

/*
Due to changes in HTSP v32, try to use subtitle
as the description if the description and summary
failed. Clear the subtitle afterwards to avoid

This comment has been minimized.

Copy link
@ksooo

ksooo Feb 5, 2019

Member

This comment is wrong as it refeneces "summary" which is not avail for recordings.

This comment has been minimized.

Copy link
@Verequies

Verequies Feb 5, 2019

Author Contributor

Should this not be removed as well then? Thats why I referenced it.

pvr.hts/src/Tvheadend.cpp

Lines 2460 to 2461 in dc35ade

else if ((str = htsmsg_get_str(msg, "summary")) != NULL)
rec.SetDescription(str);

duplicate information being displayed.
This was done by TVHeadend prior to HTSP v32.
*/

This comment has been minimized.

Copy link
@ksooo

ksooo Feb 5, 2019

Member

This comment block fits best after if (rec.GetDescription().empty()) {, not before. Please move the comment.

@Verequies Verequies force-pushed the Verequies:master branch from 6bd5dfd to e1cbd32 Feb 5, 2019
@Verequies

This comment has been minimized.

Copy link
Contributor Author

commented Feb 5, 2019

Fixed up those issues, should be good now. Unless #401 (comment) needs to be removed?

@Jalle19
Jalle19 approved these changes Feb 6, 2019
Copy link
Contributor

left a comment

Definitely clearer now that the logic was moved

This was done by TVHeadend prior to HTSP v32.
*/
if (!rec.GetSubtitle().empty()) {

This comment has been minimized.

Copy link
@Jalle19

Jalle19 Feb 6, 2019

Contributor

This could be combined in the if clause before, i.e. rec.GetDescription.empty() && !rec.GetSubtitle().empty()

This comment has been minimized.

Copy link
@Verequies

Verequies Feb 6, 2019

Author Contributor

I can do that. Reason I didn't before was just incase another check in that section needed to be added later on. But I suppose it wouldn't.

Will up date this now.

@Verequies Verequies force-pushed the Verequies:master branch from e1cbd32 to 9832113 Feb 6, 2019
Verequies added 3 commits Feb 4, 2019
Some channels have the description within the subtitle on HTSP v32+.
Adding a check for this when the description is empty fixes the
missing EPG information.

Signed-off-by: Verequies <hamishclaxton@gmail.com>
Some channels have the description within the subtitle on HTSP v32+.
Adding a check for this when the description is empty fixes the
missing DVR information.

Signed-off-by: Verequies <hamishclaxton@gmail.com>
Signed-off-by: Verequies <hamishclaxton@gmail.com>
@Verequies Verequies force-pushed the Verequies:master branch from 9832113 to 8f9dbb6 Feb 6, 2019
@Verequies

This comment has been minimized.

Copy link
Contributor Author

commented Feb 6, 2019

Fixed that up and amended a comment.

@ksooo
ksooo approved these changes Feb 6, 2019
@ksooo

This comment has been minimized.

Copy link
Member

commented Feb 6, 2019

Thanks for your contribution.

@ksooo ksooo merged commit e394c79 into kodi-pvr:master Feb 6, 2019
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/jenkins/pr-merge This commit cannot be built
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@Verequies

This comment has been minimized.

Copy link
Contributor Author

commented Feb 6, 2019

No problem, happy to help :)

@dkonerma

This comment has been minimized.

Copy link
Contributor

commented Feb 7, 2019

Hmm,

assuming we have e.g. an series episode that is named by subtitle.
Then we clear out the valid and usable subtitle and move that to description, just because description is empty otherwise? Or do I get that wrong? ;-)

Cheers
Dietmar.

@Verequies

This comment has been minimized.

Copy link
Contributor Author

commented Feb 7, 2019

It attempts to use the summary as the description first, and if that is empty then it attempts to use the subtitle as the description. In my testing TV episodes such as "The Big Bang Theory" on 9 Go, work fine. They show the episode name as the subtitle, however they have a description. But for the majority of channels, the description is in the subtitle.

Yes you are correct though. Is there any issue? I've been testing this for the past few days and have had no issue personally. Another person on the Kodi forum also confirmed this fixed their issues.

@ksooo

This comment has been minimized.

Copy link
Member

commented Feb 7, 2019

assuming we have e.g. an series episode that is named by subtitle.
Then we clear out the valid and usable subtitle and move that to description, just because description is empty otherwise? Or do I get that wrong? ;-)

We only restore functionality that was "always" in tvheadend, until it got removed there recently. So, there should be no new issues, because functunality stays the same from Kodi user's perspective.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.