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

[python] move xbmc.makeLegalFilename() to xbmcvfs module #17735

Merged
merged 1 commit into from
Apr 24, 2020

Conversation

enen92
Copy link
Member

@enen92 enen92 commented Apr 22, 2020

Description

I was not familiar with this method so decided to take a look at what it does to improve the documentation.

  • The method was using an argument (fatX) that still dates back from the original xbox days (and that was ignored internally)
  • The examples were not clear
  • Almost no-one uses it (see affected addons below)
  • The same method is exposed to binary addons in kodivfs

For consistency I think it makes more sense in xbmcvfs. It's a filesystem related (or at least helper) method anyway. Another good candidate is xbmc.translatePath (although that one is used by a lot of addons...)

This affects a total of 4 addons, none of which are yet in matrix:

(repo-scripts/helix) script.filecleaner/default.py:551:  dest_folder = xbmc.makeLegalFilename(dest_folder)
(repo-scripts/krypton) script.service.janitor/default.py:564: dest_folder = xbmc.makeLegalFilename(dest_folder)
(repo-scripts/leia) script.service.janitor/default.py:552:  dest_folder = unicode(xbmc.makeLegalFilename(dest_folder), encoding="utf-8")

(repo-plugins/helix) plugin.video.infowars/default.py:173:    xbmc.makeLegalFilename(filename)
(repo-plugins/helix) plugin.video.infowars/default.py:243:    final_path = xbmc.makeLegalFilename(final_path)
(repo-plugins/helix) plugin.video.infowars/default.py:268:    final_path = xbmc.makeLegalFilename(final_path)

(repo-plugins/jarvis) plugin.video.infowars/default.py:201:   xbmc.makeLegalFilename(filename)
(repo-plugins/jarvis) plugin.video.infowars/default.py:270:        final_path = xbmc.makeLegalFilename(final_path)
(repo-plugins/jarvis) plugin.video.infowars/default.py:293:        final_path = xbmc.makeLegalFilename(final_path)
(repo-plugins/jarvis) plugin.video.dailymotion_com/resources/lib/dailymotion.py:465:    vidfile = xbmc.makeLegalFilename(downloadDir + title + '.mp4')
(repo-plugins/jarvis) plugin.video.dailymotion_com/resources/lib/dailymotion.py:468:        tmp_file = xbmc.makeLegalFilename(tmp_file)

Motivation and Context

Improve the python api and its docs now that a breaking change is happening.

How Has This Been Tested?

Compile and tested in a sample addon.

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

Copy link
Member

@ronie ronie left a comment

Choose a reason for hiding this comment

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

looks good to me

@enen92
Copy link
Member Author

enen92 commented Apr 24, 2020

jenkins build and merge

@enen92
Copy link
Member Author

enen92 commented Apr 24, 2020

jenkins build and merge

@jenkins4kodi jenkins4kodi merged commit ef015c8 into xbmc:master Apr 24, 2020
@CastagnaIT
Copy link
Collaborator

CastagnaIT commented Apr 24, 2020

@enen92 if you're planning on moving even xbmc.translatePath
can you do it within a not-too-long space of time?
So we can limit add-ons breaks

thanks

@enen92
Copy link
Member Author

enen92 commented Apr 24, 2020

Yeah I'm thinking about moving both verifyPath and translatePath to xbmcvfs. The later will affect a lot of addons so I'm thinking about leaving a clone in xbmc with a warning (and hopefully remove it completely before matrix is final)

@CastagnaIT
Copy link
Collaborator

i think it is a very good idea

Maven85 pushed a commit to Maven85/kodi that referenced this pull request Apr 27, 2020
Maven85 pushed a commit to Maven85/kodi that referenced this pull request May 5, 2020
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 3, 2020
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 4, 2020
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 4, 2020
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 5, 2020
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 5, 2020
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 6, 2020
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 6, 2020
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 6, 2020
Maven85 pushed a commit to Maven85/kodi that referenced this pull request Aug 7, 2020
heitbaum added a commit to heitbaum/LibreELEC.tv that referenced this pull request Dec 29, 2020
xbmc.translatePath is deprecated and might be removed in future kodi versions. 
Please use xbmcvfs.translatePath instead.
The reference if from: xbmc/xbmc#17735
heitbaum added a commit to heitbaum/LibreELEC.tv that referenced this pull request Jan 1, 2021
xbmc.translatePath is deprecated and might be removed in future kodi versions. 
Please use xbmcvfs.translatePath instead.
The reference if from: xbmc/xbmc#17735
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.

None yet

5 participants