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.

Sign up for GitHub

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

Jump to bottom

[plugin.audio.cbcradio] v2.0.4 #4374

Merged
merged 1 commit into from
Sep 23, 2023
Merged

[plugin.audio.cbcradio] v2.0.4 #4374

merged 1 commit into from
Sep 23, 2023

Conversation

ihemingway
Copy link

Description

Plugin for steaming live stations from CBC Radio One and CBC Music

Checklist:

  • My code follows the add-on rules and piracy stance of this project.
  • I have read the CONTRIBUTING document
  • Each add-on submission should be a single commit with using the following style: [plugin.video.foo] v1.0.0

Additional information :

  • Submitting your add-on to this specific branch makes it available to any Kodi version equal or higher than the branch name with the applicable Kodi dependencies limits.
  • add-on development wiki page.
  • Kodi pydocs provide information about the Python API
  • PEP8 codingstyle which is considered best practice but not mandatory.
  • This add-on repository has automated code guideline check which could help you improve your coding. You can find the results of these check at Codacy. You can create your own account as well to continuously monitor your python coding before submitting to repo.
  • Development questions can be asked in the add-on development section on the Kodi forum.
  • If you see no activity on your PR after a week (so at least one weekend has passed) then please go to the #kodi-dev freenode IRC channel to reach out to the team

basrieter
basrieter requested changes Sep 13, 2023
View reviewed changes
Copy link
Contributor

@basrieter basrieter left a comment

Choose a reason for hiding this comment

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

Please see my comments.

plugin.audio.cbcradio/addon.xml Outdated
</extension>
<extension point="xbmc.addon.metadata">
<summary lang="en_GB">CBC Radio</summary>
<license>GNU General Public License, v2</license>
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use the SPDX identifier (https://spdx.org/licenses/) and make sure the license matches the license from the LICENSE.md. It currentlty does not!

Copy link
Author

Choose a reason for hiding this comment

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

done

plugin.audio.cbcradio/default.py Outdated
# cbc music has 5 min news breaks at the top of the hour
news_break()
while calc_minutes() in [00, 1, 2, 3, 4, 5]:
xbmc.sleep(10000)
Copy link
Contributor

Choose a reason for hiding this comment

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

This call completely block execution for 10 seconds without the option to abort. You should also use the MONITOR.waitForAbort() here and test for abort afterwards. Just like in line 198

Copy link
Author

Choose a reason for hiding this comment

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

done

plugin.audio.cbcradio/addon.xml Show resolved Hide resolved
@basrieter
Copy link
Contributor

Also make the add-on checker happy:
image

@ihemingway ihemingway force-pushed the 4fbf0c06-2bce-4d2a-a58e-f9ae93995ba8 branch from 8faea09 to d226e6c Compare September 13, 2023 17:15
ihemingway
ihemingway commented Sep 13, 2023
View reviewed changes
plugin.audio.cbcradio/cbc.py Outdated
PLAYER.updateInfoTag(play_item)
chill(5)
if MONITOR.abortRequested():
abort()
Copy link
Author

@ihemingway ihemingway Sep 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

ah crap.... was going to do an abort func and then changed it to chill()
i'll fix this tomorrow. i literally just have to delete lines 212-13.

Copy link
Author

Choose a reason for hiding this comment

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

i guess have a look anyway and let me know if there is anything besides this to fix before i resubmit.

Copy link
Author

Choose a reason for hiding this comment

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

actually, it should technically never reach the non-existent func. the check for MONITOR.abortRequested() is in the chill() func.... testing on my dev box i never get an exception when exiting.

up to you if you want it removed or not.

@ihemingway ihemingway force-pushed the 4fbf0c06-2bce-4d2a-a58e-f9ae93995ba8 branch from 845847e to 99ec79c Compare September 20, 2023 18:15
@ihemingway
Copy link
Author

removed those two lines. should be fine now unless there are further problems

@basrieter basrieter merged commit 8a9c12b into xbmc:nexus Sep 23, 2023
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@basrieter basrieter basrieter approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ihemingway @basrieter