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

Use xbmcvfs functions to read and write filesystem files #803

Merged
merged 1 commit into from Mar 9, 2024

Conversation

sy6sy2
Copy link
Contributor

@sy6sy2 sy6sy2 commented Dec 28, 2023

At the moment, this plugin is not working on Apple TV devices.
This is because on tvOS the virtual filesystem used by Kodi hides a more complex system where in reality any XML file of Kodi is compressed in the NSUSerDefaults dictionary.
In others words, that mean that on tvOS a Kodi plugin cannot directly read or write an XML file, instead, the add-on needs to use the xbmcvfs functions in order to let Kodi performs the translation between the virtual filesystem and the "real" filesystem that is OS/device dependent.

This PR modify the plugin so that the XML reads and writes use xbmcvfs functions.
This way, this PR allow this add-on to works on tvOS devices.

For more details, see discussion here: https://forum.kodi.tv/showthread.php?tid=374294&pid=3177147#pid3177147

Tested on macOS and Apple TV with Kodi 20.

Copy link

sonarcloud bot commented Dec 28, 2023

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link

codecov bot commented Jan 6, 2024

Codecov Report

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (1233e29) 21.51% compared to head (ec76288) 21.50%.
Report is 26 commits behind head on master.

❗ Current head ec76288 differs from pull request most recent head dad390c. Consider uploading reports for the commit dad390c to get more accurate results

Files Patch % Lines
jellyfin_kodi/helper/xmls.py 0.00% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #803      +/-   ##
==========================================
- Coverage   21.51%   21.50%   -0.02%     
==========================================
  Files          63       63              
  Lines        8542     8547       +5     
  Branches     1572     1572              
==========================================
  Hits         1838     1838              
- Misses       6680     6685       +5     
  Partials       24       24              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mcarlton00
Copy link
Member

I don't have an apple device to test with, this seems like it should solve the problem, but it does have the potential to introduce another bug.

f = xbmcvfs.File(file_name)
b = f.read()
f.close()

In the event there's an issue with f.read(), the program with error out and stop execution, but the file will never be closed and be hanging out there in memory somewhere. Typically we'd address this using something like a with open(file_name, 'r') as f: context, but from a quick search it doesn't look like xbmcvfs supports that fully. I'm not sure how likely that is to happen in this case, but given that we've already had trouble with these files multiple times in the past I'm not entirely sure I want to blindly trust it. Open to opinions

@sy6sy2
Copy link
Contributor Author

sy6sy2 commented Jan 7, 2024

Thanks for your answer.

Did you able to reproduce the issue with f.read()?
I'm agree with you, we need to be sure that the file is close in any case!
Maybe something like that is ok?:

try:
    b = f.read()
except:
    print('Failed to read file')
finally:
    f.close()

As you said, the context feature is only available since Kodi 19 (or something like that... 😕 )

@oddstr13
Copy link
Member

oddstr13 commented Feb 3, 2024

If the content manager works properly for Kodi 19 and later, that is the correct way to go.
Kodi 18 and lower are explicitly no longer supported since last release, and we intend to actively remove Python 2 support and do some cleanup around that going forward.

@sy6sy2
Copy link
Contributor Author

sy6sy2 commented Feb 5, 2024

So I will try the context manager version and if it works I will modify this PR 😉
Thx.

@oddstr13 oddstr13 added bugfix apple This issue/pr is related to an apple device labels Feb 6, 2024
Copy link

sonarcloud bot commented Feb 8, 2024

Quality Gate Passed Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

@sy6sy2
Copy link
Contributor Author

sy6sy2 commented Feb 8, 2024

Context manager version just pushed.
Tried on tvOS without any problem

@Vergil365
Copy link

will this be fixed in the next update?
unfurtunatly i dont understand how to apply the fix to the jellyfin addon :(

@mcarlton00 mcarlton00 merged commit 8869ace into jellyfin:master Mar 9, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
apple This issue/pr is related to an apple device bugfix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants