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

feat: update to Kodi Nexus and python 3.10 #46

Merged
merged 7 commits into from Jul 11, 2023
Merged

feat: update to Kodi Nexus and python 3.10 #46

merged 7 commits into from Jul 11, 2023

Conversation

sanzoghenzo
Copy link
Contributor

@sanzoghenzo sanzoghenzo commented May 31, 2023 •
edited by Lord-Grey

Hi there!
I tried to update the add-on to Kodi v20 (the version I'm using), hopefully useful (but not tested) also for v19 asked in #45.

  • refactorings to adhere to PEP8 and new add-on structure
  • take advantage of new settings API
  • move most of the logic to HyperionMonitor (xbmc.Monitor subclass)
  • the state classes are now methods in the HyperionMonitor class, but the same state pattern is used.

I had to repackage the script.module.protobuf, I'll do a PR in the official kodi repository to ease the installation.

I suppose also this add-on can be merged in the offical repository; I can do it, but I think it would be better if it comes from the hyperion project itself!

Fixes #45

@sanzoghenzo
feat: update to Kodi Nexus and python 3.10
c2dd396 
- refactorings to adhere to PEP8 and new add-on structure
- take advantage of new settings API
- move most of the logic to the Monitor subclass
@sanzoghenzo sanzoghenzo mentioned this pull request May 31, 2023
Merged
3 tasks
@sanzoghenzo
Copy link
Contributor Author

Is there anyone here?
Are you interested in an updated version or should I submit it to the kodi official repositories myself?

thanks for the attention

@al-madjus
Copy link

For what it's worth I've tried your plugin and it works perfectly, good job! I'm not affiliated with this project but would also like your changes to be merged, so hopefully someone will reply to this.

@Lord-Grey
Copy link
Contributor

@sanzoghenzo Thank you very much for your contribution and getting the add-on aligned to the latest standards.
Very much appreciated!!!

A couple of questions and feedback

  1. Just for my interest. What has been the rationale to repackage protobuf which now bring in an additional dependency.
    Are there any new features to be benefit from or is it more housekeeping and align to latest developments?

  2. See additional comments in as part of the code review comments

  3. A couple of month ago, I tried to do some updates to the add-on too, but never publish anything.
    I added the capability to maintain the image ratio to ensure that the image is not scaled wrongly.
    I have no Python knowledge and would like to ask you, if what I did makes sense or I am happy, if you would further get it in line with the right python coding standards/conventions.

I added my go as a patch on top of your PR:
hyperion_kodi.patch.txt

In addition, I addressed already some of the feedback provided by 2.

4.I assume that you and @al-madjus did not test the add-on on an rpi4, correct?
Otherwise I would like to hear how you configured Kodi/LibeElect to have screen capture working.

@sanzoghenzo
Copy link
Contributor Author

Hi @Lord-Grey , thanks for taking the time to review this.
Unfortunately I cannot see your comments, have you properly submitted the review?
In the meantime, I'll incorporate your patch.

  1. Just for my interest. What has been the rationale to repackage protobuf which now bring in an additional dependency.

protobuf is simply missing from the official kodi repository for newer versions, so it needed to be re-uploaded (still waiting for the PR to be merged)

  1. I assume that you and @al-madjus did not test the add-on on an rpi4, correct? Otherwise I would like to hear how you configured Kodi/LibeElect to have screen capture working.

You are correct, I'm running my kodi on a thinkcentre (x64) machine.

Lord-Grey
Lord-Grey requested changes Jul 10, 2023
View reviewed changes
resources/settings.xml Outdated Show resolved Hide resolved
resources/lib/monitor.py Outdated Show resolved Hide resolved
resources/settings.xml Show resolved Hide resolved
@Lord-Grey
Copy link
Contributor

Unfortunately I cannot see your comments, have you properly submitted the review?

Can you see the comments now?

Thanks for all the other answers and your go issuing a ProtoBuf PR!

@sanzoghenzo
fix: various improvements
be20252 
- use pillow for image processing
- preserving aspect ratio
- capture was issued only at connection
- reconnect on host or port settings change
- remove unused labels
@sanzoghenzo
Copy link
Contributor Author

Now we should be all set, I also handled the case where the connection parameters are changed while a video is still playing (I don't even know if it is possible, but better safe than sorry!)

@al-madjus
Copy link

4.I assume that you and @al-madjus did not test the add-on on an rpi4, correct? Otherwise I would like to hear how you configured Kodi/LibeElect to have screen capture working.

Hi,
I did not test on the RPi4 either, but on a PC running ChimeraOS and Kodi. Unfortunately I do not have an RPi4 to test on.

@Lord-Grey
Copy link
Contributor

Now we should be all set, I also handled the case where the connection parameters are changed while a video is still playing (I don't even know if it is possible, but better safe than sorry!)

That is great.
I did some further updates to align the fanart, etc. with the latest Hyperion icon sets.
Plus I did some minor editorial text correction.

Would you mind granting me access to your repository? Then I would just do a commit...

@Lord-Grey
Copy link
Contributor

Lord-Grey commented Jul 11, 2023 •
edited

@sanzoghenzo Please have a look at the additional commits and updates.
Ping me, if you are ready and do not want to do further updates.
Then I will merge the PR.

@sanzoghenzo
Copy link
Contributor Author

I fixed some wording in the readme, and while I was at it I added the Italian translation.

I'm not sure about keeping the debug logs for the capture size and aspect ratio, since they are displayed at every screen grab and pollute the log big time. How do you feel if I remove it?

@al-madjus would you be so nice to test the latest commits? I moved things around and is working fine for me, at least the image is sent to Hyperion (I still can't get my yeelight lights to cooperate!). If I watch the live video in the led Visualization it flickers a lot, but I don't know how it translates to to the actual leds.

@Lord-Grey
Copy link
Contributor

That you for the corrections. Looks I had fat-fingers… :)

On the debug statements… I intended to have them removed before merging.
Would you mind removing those statements, please? I am currently not at my desk.
Thank you very much in advance.

@sanzoghenzo
Copy link
Contributor Author

Done! I also saw the hardcoded 500ms duration in the hyperion send_image method, aligned to the capture sleep_time

@Lord-Grey
Copy link
Contributor

Thank you!
Ready to be merged?

@Lord-Grey Lord-Grey mentioned this pull request Jul 11, 2023
Closed
@Lord-Grey Lord-Grey merged commit 0434735 into hyperion-project:master Jul 11, 2023
@sanzoghenzo
Copy link
Contributor Author

Thanks @Lord-Grey for taking the time to move this forward!

What do you think about pushing the changes in the Official kodi repository?

We should follow these instructions to create a single commit PR, and verify everything with the addon checker (it is run on CI, but it's better to be prepared before making the PR!)

As per kodi repo mantainers instructions, I'm afraid it should be you the one that submits the PR as project owner 😉

@Lord-Grey
Copy link
Contributor

@sanzoghenzo

For publishing the add.on to the official repository, let me discuss within the core team

Neverheless, I addressed the findings raised by the kodi-addon-checker and created a new PR (but reused your kodi20 branch).
May I ask you having a look, if everything is in sync?

As we are on the way cleaning up the Kodi add-ons....
Would you like having a look getting https://github.com/hyperion-project/hyperion.control to Nexus and proper python coding, too?

@sanzoghenzo
Copy link
Contributor Author

sanzoghenzo commented Jul 12, 2023 •
edited

Oh, I didn't even know the existence of hyperion.control.

May I ask why it is a separate add-on? I think it would be handy for end users to have a single kodi add-on to handle everything hyperion related... or am I missing something?
But maybe we can discuss this in the forum, this pr is definitely not the right place 😅

@al-madjus
Copy link

@al-madjus would you be so nice to test the latest commits? I moved things around and is working fine for me, at least the image is sent to Hyperion (I still can't get my yeelight lights to cooperate!). If I watch the live video in the led Visualization it flickers a lot, but I don't know how it translates to to the actual leds.

Hi, sorry for the delay. I've checked it on my Linux box running Kodi with a couple of videos, one from YouTube and another on my hard drive, and it works great, no issues detected.
Thanks to both of you for your work on this, I was afraid Hyperion had gotten abandoned on Kodi. I've been using it for years and can't live without it anymore XD

@Lord-Grey
Copy link
Contributor

What do you think about pushing the changes in the Official kodi repository?

@sanzoghenzo PR4350 issued

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Lord-Grey Lord-Grey Lord-Grey 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.

update to Python 3 (Kodi v19)
3 participants
@sanzoghenzo @al-madjus @Lord-Grey