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

[script.plexmod] 0.7.6 #2571

Merged
merged 7 commits into from
Apr 3, 2024
Merged

[script.plexmod] 0.7.6 #2571

merged 7 commits into from
Apr 3, 2024

Conversation

pannal
Copy link

@pannal pannal commented Jan 2, 2024

Description

PlexMod (for Kodi)
This is a fork of the official open-source Plex client for Kodi "plex-for-kodi" (Plex4Kodi) maintained by me (pannal). The official client was abandoned years ago, this fork is over 800 commits ahead and supports all Kodi versions since Leia/18.

This submission has been agreed upon in advance with Plex Inc's legal team.

Source, Discussion

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: [script.foo.bar] 1.0.0

Ref: #2565

@pannal
Copy link
Author

pannal commented Jan 2, 2024

@romanvm regarding your comment in the old PR and its size: You already have the base repository of this addon in your Kodi repository, script.plex. If it helps, you can significantly reduce the amount to review by just reviewing the changes between script.plex and script.plexmod.

@pannal pannal changed the title [script.plexmod] 0.7.3 [script.plexmod] 0.7.4 Jan 14, 2024
@pannal
Copy link
Author

pannal commented Feb 9, 2024

@romanvm how are we looking with merging this? The reason I ask is that 0.7.5 will be released tomorrow and will probably enter stable state immediately. Does it make sense to update this PR to 0.7.5, or would that significantly delay the merge?

Thanks!

@keithah
Copy link
Member

keithah commented Feb 13, 2024

Spoke w/ @romanvm and he said he will review it when he has time piece by piece. Appreciate you continuing this! If you want an invite to our slack as a guest, email me at Keith-at-kodi.tv and I can send it over.

@ruuk sounds like we should remove the old one from the repo, if you haven't already?

@romanvm
Copy link
Collaborator

romanvm commented Feb 13, 2024

I started the review but the process is not very fast because GitHub PR review UI is painfully slow with such big number of files. And this is considering the fact, that I have a very good computer with AMD Ryzen 7 8 cores and 32MB RAM.

I see that you are using asyncio module. Please read this section of Kodi Wiki and implement the required workaround: https://kodi.wiki/view/Python_Problems#asyncio

@ruuk
Copy link
Member

ruuk commented Feb 13, 2024

Spoke w/ @romanvm and he said he will review it when he has time piece by piece. Appreciate you continuing this! If you want an invite to our slack as a guest, email me at Keith-at-kodi.tv and I can send it over.

@ruuk sounds like we should remove the old one from the repo, if you haven't already?

I'll have to find out. This is the first I'm hearing about this. I don't know of any formal decision to abandon the app. (FWIW, I'm personally happy to see pannal's version in the repo.)

@pannal
Copy link
Author

pannal commented Feb 13, 2024

I'm not aware of any official plans to remove the original addon, either. I went through a lengthy process to get this one approved, especially through the legal issues (naming). I'm sure it has been discussed internally but I haven't heard anything.

I see that you are using asyncio module. Please read this section of Kodi Wiki and implement the required workaround:

I'll check, but wouldn't the original addon have to do the same? I'm fairly certain I haven't added that dependency, but I'll double check.

@keithah @romanvm so should I go ahead and update this PR with the latest stable, or would that make your current review harder?

Thanks for taking care of this huge merge!

@romanvm
Copy link
Collaborator

romanvm commented Feb 14, 2024

I'll check, but wouldn't the original addon have to do the same? I'm fairly certain I haven't added that dependency, but I'll double check.

@pannal According to the diff you have provided imcplib package was added in this addon. And the original addon exists for a long time, probably since Python 2 times, so I doubt that it uses async code with asyncio library.

@pannal
Copy link
Author

pannal commented Feb 14, 2024

@pannal According to the diff you have provided imcplib package was added in this addon. And the original addon exists for a long time, probably since Python 2 times, so I doubt that it uses async code with asyncio library.

Ah yeah, you're right.

@pannal
Copy link
Author

pannal commented Feb 14, 2024

Updated with 0.7.5 and your asyncio suggestion. Diff to script.plex: plexinc/plex-for-kodi@master...pannal:plex-for-kodi:develop_kodi21#files_bucket (the asyncio change is not in that diff yet, only in this PR)

@pannal pannal changed the title [script.plexmod] 0.7.4 [script.plexmod] 0.7.5 Feb 14, 2024
@romanvm
Copy link
Collaborator

romanvm commented Feb 14, 2024

Updated with 0.7.5 and your asyncio suggestion.

I'd recommend to add the asyncio workaround at the very top level, that is, to both of your addon entrypoints: default.py and plugin.py before all other imports, unless you absolutely sure that one of them won't start asyncio event loop so that entrypoint can be left unchanged.

@pannal
Copy link
Author

pannal commented Feb 15, 2024

None of them will, they both run the same entrypoint in main.py, and we don't use the asyncio features of icmplib.

@romanvm
Copy link
Collaborator

romanvm commented Feb 15, 2024

Ok then as long as it won't interfere with other addons using asyncio.

@pannal
Copy link
Author

pannal commented Feb 16, 2024

@keithah I've sent you an email, did you receive it? Thanks!

Copy link
Collaborator

@romanvm romanvm left a comment

Choose a reason for hiding this comment

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

LGTM

@pannal
Copy link
Author

pannal commented Feb 22, 2024

Appreciate you continuing this! If you want an invite to our slack as a guest, email me at Keith-at-kodi.tv and I can send it over.

Did you receive my email? Or wasn't this meant for me?

@ruuk
Copy link
Member

ruuk commented Feb 22, 2024

@ruuk sounds like we should remove the old one from the repo, if you haven't already?

I've talked to people and the ball is rolling on officially sunsetting the add-on. Hopefully it's not an issue to leave it where it is and allow it to coexist for a bit. There's kind of a process here for handling this 🙂.

@keithah
Copy link
Member

keithah commented Feb 22, 2024

Appreciate you continuing this! If you want an invite to our slack as a guest, email me at Keith-at-kodi.tv and I can send it over.

Did you receive my email? Or wasn't this meant for me?

It was for you! I don't see the email. Can you try again? Keith@Kodi dot tv

@pannal
Copy link
Author

pannal commented Mar 2, 2024

@romanvm would it be OK for you if I push 0.7.6 into this PR? It fixes one of the biggest issues that P4K first time users have - the necessity to disable DNS rebind protection in their DNS server for the plex.direct domain.

@romanvm
Copy link
Collaborator

romanvm commented Mar 2, 2024

Only if you provide a diff with changes so I won't have to go over all the files.

@pannal
Copy link
Author

pannal commented Mar 2, 2024

@pannal pannal changed the title [script.plexmod] 0.7.5 [script.plexmod] 0.7.6 Mar 3, 2024
@pannal
Copy link
Author

pannal commented Mar 3, 2024

Thank you so much!

Copy link
Collaborator

@romanvm romanvm left a comment

Choose a reason for hiding this comment

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

Ok, it's LGTM. If nobody else from the team has any input I'll merge this in a couple of days. Pleas do not push any updates in this PR.

@pannal
Copy link
Author

pannal commented Mar 9, 2024

I won't. Thank you for sticking with me and the very frequent release-cycle I'm on right now.

@romanvm romanvm added the Approved Approved and is ready to merge label Apr 3, 2024
@romanvm romanvm merged commit 730cc50 into xbmc:matrix Apr 3, 2024
1 check passed
@pannal
Copy link
Author

pannal commented Apr 5, 2024

Thank you again!

@pannal pannal deleted the matrix branch April 20, 2024 02:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Approved and is ready to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants