Store streamed video subtitle on custom subtitles path if defined by user

[hagaygo]

Description

The fix is by checking if a custom subtitle path is defined, if so, do store the downloaded subtitle on the custom path.
This way the user is allowed to keep the old kodi behavior if indeed needed.

Motivation and context

fixes the "regression" described on #24742 which was introduced on #24252

How has this been tested?

Tested locally on a windows machine on my setup.

When no custom subtitles path is defined , the new behavior is kept.
When setting a custom subtitle path the old behavior is back , and the file is stored as stream.lang.srt on the custom subtitle path.

What is the effect on users?

Users like me which utilized the old behavior can keep it.
Users which did not set custom subtitles path should not be affected at all.

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)
• Student submission (PR was done for educational purposes and will be treated as such)
• 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

[hagaygo]

@fuzzard @neo1973
If i understand the labels correctly , this will not get into omega (v21) but only to v22 ?
If so it means v21 users will have the "regression" i mentioned (most users don't try beta versions and especially since it was introduced between b2 and b3)

[arnova]

@fuzzard : IMO this should go in for v21 as well.

[CastagnaIT]

@hagaygo please apply jenkins changes
and also the commit title should follow our guidelines
in short: too long title, and missing component prefix

[hagaygo]

@CastagnaIT
I hope it is o.k now.

[CastagnaIT]

when you merge the 2 commits in a single one then will be ok

[hagaygo]

@CastagnaIT
Squashed the commits

jenkins4kodi popped a message again about coding style although the previous fix seems to be in.
Tried to apply the patch just to make sure and i get

error: patch failed: xbmc/video/dialogs/GUIDialogSubtitles.cpp:556
error: xbmc/video/dialogs/GUIDialogSubtitles.cpp: patch does not apply

So i guess it is a false positive or something like that.

Thanks for your patience.

[CastagnaIT]

yes was the previous check, when jenkins will have finish should disappear

[ksooo]

Sorry, need to put a -1 to overrule the one +1 this PR already has. I think the code should be polished a bit more before merge.

[ksooo]

If URIUtils::IsHTTP(strCurrentFile)evaluates to false you do not need the value of subPath - so no point to always calculate it. The code should be optimized to only calculate subPath if it is actually needed.

[hagaygo]

Hi

Thanks for your feedback.

While i understand the motive for optimization here (although i am not sure how really "pricey" is a call for TranslatePath) the only solutions i can think of (not a cpp expert) are :

1.Make the .empty() check inline in the if conidtion something like :

if (URIUtils::IsHTTP(strCurrentFile) && CSpecialProtocol::TranslatePath("special://subtitles").empty())

and move the subPath back to the original location

In this case , we will only save the call to TranslatePath when IsHttp is false but we'll have 2 assignments in the code for TranslatePath, which is kinda of duplicate code in same method logic (if the settings key is gonna changed somewhere in the future , we'll have 2 lines to update here) i perosnally prefer a more maintainable code than more optimized one (again , not sure about the "cost" of TranslatePath call).

2.Create a wrapper method that will return the value of CSpecialProtocol::TranslatePath("special://subtitles") and call it all around this method, we can add some sort of caching so multiple calls (can happen when IsHttp is false and path is not empty) won't call to TranslatePath again.

I personally think current code is the best balance between the issues here and if there is a big "cost" when calling TranslatePath the optimization needs to be done there and not in this code block (and every other place that it get called).

[da-anda]

unless I read the code wrong, the subpath is always needed here, because if URIUtils::isHTTP is false, the subPath still is used in the else part of this condition. So subPath has to be resolved regardless if isHTTP is true or false

[fuzzard]

From a performance perspective, wouldnt the empty check be better off first? Surely its quicker than ishttp (that does stack checks/special folder checks etc.

From what i can gather, the old behaviour was always just false for this check, and thats the behaviour you are after when a special subtitles path is set. So @ksooo i think the subpath is necessary, but the ordering of the statements should be least to most expensive for an early fail in the event a user has the special path set.