thp / pyotherside

Python Bindings for Qt 5 and Qt 6. Allows you to access a CPython 3 interpreter directly from your Qt QML user interface code.
https://thp.io/2011/pyotherside/
Other
364 stars 49 forks source link

Fixes PyOtherSideQtRCImporter for submodule imports #134

Closed timegrid closed 5 months ago

timegrid commented 6 months ago

It looks like the importer fix for python 3.12 in #131 does not cover imports of submodules. In moment we use importNames() like this:

addImportPath("qrc:/src")
importNames("backend.qml_bridge", ["BRIDGE"], () => { [...] });

This stopped working with the upgrade to python 3.12, #131 did not fix it. When used for a submodule, the path variable is set to the value of __path__ from the parent package according to the reference. By removing this path is None condition, it works again. Not sure though, if this MR will break other usecases though.

@apollo13: could you please test this against your setup?

apollo13 commented 6 months ago

Puh will see about testing that, I will try over the weekend. Can you check what path is in your case? Maybe it would be "more" correct to restore the initial check: https://github.com/thp/pyotherside/pull/131/files#diff-453f8c4ed0f10bda6f41ccfd2ab26d7bb45ecc92355ac956b53c75ff9eb1962aL25

So essentially:

if path is None or all(x.startswith('qrc:') for x in path):
timegrid commented 6 months ago

In our application the path parameters for different cases look like this:

backend -> None
backend.qml_bridge -> ['qrc:/src/backend']
backend.models.model_item ->  ['qrc:/src/backend/models']
cffi._pycparser -> ['/media/data/code/moment/venv/lib/python3.12/site-packages/cffi']

So excluding non qrc paths makes sense. Restoring this former check works, too, so I'll adjust this PR. Thanks for the hint.

apollo13 commented 6 months ago

Yeah, that makes sense! Thank for double checking.

thp commented 5 months ago

@apollo13 @timegrid Is this ready to go in?

apollo13 commented 5 months ago

I didn't get around to test it. But since it restores the original check that I dropped and reallows sub imports it is ready imo

On Fri, May 31, 2024, at 08:38, Thomas Perl wrote:

@apollo13 https://github.com/apollo13 @timegrid https://github.com/timegrid Is this ready to go in?

— Reply to this email directly, view it on GitHub https://github.com/thp/pyotherside/pull/134#issuecomment-2141326600, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAT5C7H5IV7U6G2GKHSF3LZFALHTAVCNFSM6AAAAABHCXUEBGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNBRGMZDMNRQGA. You are receiving this because you were mentioned.Message ID: @.***>

timegrid commented 5 months ago

I agree. Additionally some moment users tested a patched version for a while, and everything seems fine with the imports.