bramwalet / Subliminal.bundle

Plex Metadata agent plugin based on Subliminal
MIT License
150 stars 143 forks source link

Add subtitle folder support++ #4

Closed pannal closed 10 years ago

pannal commented 10 years ago

Sorry for adding that "reapply gitignore" commit.

For full feature support this needs the LocalMediaExtended.bundle: https://github.com/pannal/LocalMediaExtended.bundle

bramwalet commented 10 years ago

We need tot check if subliminal supports subfolders in scan_video. See http://subliminal.readthedocs.org/en/latest/api/video.html When we scan the video before we seach subtitles, both embedded and subs next tot the video file are detected. However, if we download subtitles on initial scan, then do a metadata refresh, there is no way of knowing that subtitles exist already.

bramwalet commented 10 years ago

Also, I think we are better off saving the sub as metadata instead on the filesystem. What's your opinion?

pannal commented 10 years ago

Hmm, the reason I liked the subliminal bundle in the first place was that the subs are NOT stored as metadata, as I find it hard to manage. I often have to download different subtitles because some aren't well aligned or of low quality - simply being able to delete stuff in a (sub-) folder makes this really convenient.

Perhaps we can let the user decide?

bramwalet commented 10 years ago

I agree with you that metadata is hard to manage by the user. But having dependencies of other bundles, or even settings that depend on a fork of a specific bundle, is hard to explain to the average user. I also think that we cannot guarantee that writing to the filesystem of media folders is always permitted. The user can get permission problems. But I agree that the expert user should have more influence.

Therefore I suggest the following:

When an expert user overrides the folder settings, LocalMediaExtended should be used.

pannal commented 10 years ago

That sounds pretty nice to me. We should add LocalMediaExtended and subliminal to UnsupportedAppstore btw.

I guess I'm gonna release the LocalMedaExtended bundle apart from subliminal aswell, as it may be beneficial to more than just subliminal users (many subs come stored in a subfolder).

bramwalet commented 10 years ago

I was looking at the changed files in the pull request, but I cannot figure out why subliminal_ is imported and added as new files? Is it possible to remove it from the pull request, or did you change the source of the subliminal library in any way? I think only DefaultPrefs.json and init.py from Contents/Code should change.

pannal commented 10 years ago

No, my git version was too old so it tried to add .pyc files because it was ignoring .gitignore. Therefore I had to re-add all the files locally, after updating git (and clearing the git cache) which lead to that clutter.

I can split this up for you into a separate pull request. Otherwise you can just merge, as I didn't change anything in the libraries.

Wait no, that isn't the case. That's a test folder of mine. I'm gonna remove it.

bramwalet commented 10 years ago

Merged your branch manually and removed the folder in my branch. Thanks!

bramwalet commented 10 years ago

Created issue #5 for saving the subtitle and preferences to influence the behaviour.