-
-
Notifications
You must be signed in to change notification settings - Fork 70
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
add qmlbot #476
base: master
Are you sure you want to change the base?
add qmlbot #476
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good for a first interaction, thanks!
Please take a look at my comments, and please remove the PyCharm project files from the PR (the .idea
folder).
Co-authored-by: Bruno Oliveira <[email protected]>
Co-authored-by: Bruno Oliveira <[email protected]>
Co-authored-by: Bruno Oliveira <[email protected]>
seems like something is wrong with the imports. |
] # self is needed for it not to be collected by the gc | ||
return self._root.findChild(qt_api.QtQuick.QQuickItem, "contentloader") | ||
|
||
def loads(self, content: str) -> Any: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI decided to drop the load(path: Path) -> Any
variant, because the user can easily just call loads(p.read_text(encoding=...))
instead of load(p)
, with the advantage that we do not need to guess an encoding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, that is not the same. qml files can import other qml files based on relative path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure I follow, what does the fact that "qml files can import other qml files" relates to being able to load a qml file passing a Path
? Can you exemplify?
Feel free to add it back, I removed because it seemed redundant and it the test was failing, but if it is not redundant we can revisit this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could though pass the pass in the url here self._comp.setData(content.encode("utf-8"), qt_api.QtCore.QUrl())
I think
though I don't know if it is really the same under the hood. this way or another still worth to keep.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In QML you can every file encapsulates a Component
i.e if MyApp.qml
lives near main.qml
You would do
import QtQuick
MyApp{}
and if i.e MyApp.qml
resides in ./impl/MyApp.qml
so you would need to import impl
import QtQuick
import "impl"
MyApp{}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh on CI it is showing the same error I got locally:
_____________________________ test_load_from_file _____________________________
qmlbot = <pytestqt.qml.qmlbot.QmlBot object at 0x00000193133134C8>
def test_load_from_file(qmlbot: QmlBot) -> None:
item = qmlbot.load(Path(__file__).parent / "sample.qml")
> assert item.property("hello") == "world"
E AttributeError: 'NoneType' object has no attribute 'property'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you on windows?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. 👍
Hmm tests are crashing now... 😬 |
I'll have a look later tonight 🤞 Also I saw that there is pytest-qml, seems like it is not maintained. You think it should fit better there or it would be better to merge pytest-qml here? |
for more information, see https://pre-commit.ci
resolves #474
TODO
load()