Add ability to set Title of podcast using title.txt #73
Conversation
|
@ben-xo I just noticed that my commit isn't signed yet. I don't believe I can do that for code that is already committed. Shall I do a new PR? |
Not unless you want to - it'll be signed by me when it gets merged in anyway 👍 |
|
@ben-xo then I'll leave it like this :). Let me know if you have feedback on the PR! |
I'll do you a code review since I do them regularly for my team at Mixcloud. :) |
There was a problem hiding this comment.
Your changes make sense and cover a case I hadn't considered. Please let me know if I've understood the problem (I.e what exactly it causes problems for in iTunes) correctly. I will need to add a unit test before releasing it - I did my unit test chores during lockdown so I've set that as a standard now. However, what you've raised in this pull request looks fine to me! Thank you for doing so, I appreciate your time!
| if(file_exists(MP3_DIR() . 'title.txt')) | ||
| define('TITLE', file_get_contents(MP3_DIR() . 'title.txt')); | ||
| elseif(basename(MP3_DIR())) |
There was a problem hiding this comment.
Ok! So this change looks legit to me, and although you didn't explicitly say this, it makes sense for the configuration where one deployment of dir2cast is powering multiple sun folders (otherwise you could achieve the same thing just by editing dir2cast.ini and setting the title there).
Two things spring to mind:
- From the description you gave of the problem you are trying to solve, it occurs to me that perhaps MP3_DIR() should be doing a urldecode() somewhere so that the %20 space characters are not in displayed in the title. Please let me know if that is actually the problem! Regardless of that, I can see other uses (titles with characters that you just aren't allowed to have in a folder name such as '/')
- You didn't add a unit test for the new functionality. I realise that adding those is generally quite boring 😂 so I'm happy to add one for you unless you beat me to it.
There was a problem hiding this comment.
- From the description you gave of the problem you are trying to solve, it occurs to me that perhaps MP3_DIR() should be doing a urldecode() somewhere so that the %20 space characters are not displayed in the title. Please let me know if that is actually the problem! Regardless of that, I can see other uses (titles with characters that you just aren't allowed to have in a folder name such as '/')
Correct! So the exact use case for me is that I'm using dir2cast in a Docker container to host my own audiobooks as a Podcast. This allows me to add the audiobook as an RSS feed to Snipd, which has the ability to 'highlight' parts of the audio. Every audiobook therefore gets it's own folder. However, Snipd doesn't seem to handle image URLs with spaces very well. Reminder to myself to log that as a defect there as well :).
- You didn't add a unit test for the new functionality. I realise that adding those is generally quite boring 😂 so I'm happy to add one for you unless you beat me to it.
Ah sorry, missed that one! I'm happy to add a Unit test myself. However, I have no experience in PHP (I made the edit based on copying what you did already haha). So if you could point towards some documentation on how you've structured these tests, then I'll try to see if I can build something that works :).
There was a problem hiding this comment.
Ah sorry, missed that one! I'm happy to add a Unit test myself. However, I have no experience in PHP (I made the edit based on copying what you did already haha). So if you could point towards some documentation on how you've structured these tests, then I'll try to see if I can build something that works :).
Really no ptoblem. If you look for the tests which reference description.txt (or other files which are on adjacent lines to your change on line 2120) and tests which reference TITLE this may be enough to get going - but it's probably not the most straightforward. If you can't make head nor tail of it, let me know and I'll do it.
|
I've extended your PR in this one: #74 - that's the one i'll merge, with credit |
|
@ben-xo nice, very impressive! Thanks for adding all the tests and additional documentation :) |
Adding the ability to add a title.txt to the MP3 folder would set the title of the podcast. Adding this since I have some podcasts with spaces in the title, but when trying to load the iTunes image, this wouldn't always play nice.
Previous
Folder:
/localdir/some podcast with spaces/URL podcast:
www.base-url.com/some%20podcast%20with%spacesItunes image:
www.base-url.com/some%20podcast%20with%spaces/itunes_image.jpgCurrent
Folder:
/localdir/somepodcastwithoutspaces/Title.txt:
Some podcast with spacesURL podcast:
www.base-url.com/somepodcastwithoutspacesItunes image:
www.base-url.com/somepodcastwithoutspaces/itunes_image.jpg