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

VPN-6375 - Fix QML modules import #9531

Merged
merged 3 commits into from
May 13, 2024
Merged

VPN-6375 - Fix QML modules import #9531

merged 3 commits into from
May 13, 2024

Conversation

brizental
Copy link
Contributor

@brizental brizental commented May 10, 2024

Ok, something funky is happening with the Qt6.6 imports.

Here is the thing, this patch fixes it. But I kinda hate the way it fixes it, makes no sense.

Opening as a draft, as food for thought. It seems sometime Qt6.6 is unable to resolve QML files in the same module???? cc @lesleyjanenorton for QML knowledge.

Copy link
Member

@lesleyjanenorton lesleyjanenorton left a comment

Choose a reason for hiding this comment

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

But I kinda hate the way it fixes it, makes no sense.

I may actually hate this less than the qmldir files which were (I think) facilitating this before. I think my only concern here is that there are related bugs waiting to be found. For instance we'll definitely need to add similar import statements to ViewServers.qml, ViewMultiHop.qml and maybe some of the in-app auth views as well.

@brizental
Copy link
Contributor Author

brizental commented May 13, 2024

I may actually hate this less than the qmldir files which were (I think) facilitating this before.

Interesting. If this is the case, then we might have another easier and better fix. CMake is building the qmldir file for us now, it's not like they are gone. Maybe we just need to point the build to the right place where the file is now 🤔

Ok, so here is the thing: this fixes the issue and I don't know why.

With the removal of qmldir files manually written by developers
and the usage of qt_add_qml_module, it's not like there will not
be any more qmldir files, it's that they will be generated for us.

When I looked at the generated files I noted they have this in the header

linktarget mozillavpn-uiplugin
plugin mozillavpn-uiplugin
classname Mozilla_VPNPlugin
typeinfo mozillavpn-ui.qmltypes
prefer :/Mozilla/VPN/

These are not defined in the lottie and nebula qmldir files -- which work.

I started playing around with options that would change this such as
NO_PLUGIN (which did nothing) and NO_PLUGIN_OPTINAL which fixed the import
issues we were having :shrug.
@@ -5,6 +5,7 @@
qt_add_qml_module(mozillavpn-ui
VERSION 1.0
URI Mozilla.VPN
NO_PLUGIN_OPTIONAL
Copy link
Contributor Author

@brizental brizental May 13, 2024

Choose a reason for hiding this comment

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

Ok, so here is the thing: this fixes the issue and I don't know why.

With the removal of qmldir files manually written by developers
and the usage of qt_add_qml_module, it's not like there will not
be any more qmldir files, it's that they will be generated for us.

When I looked at the generated files I noted they have this in the header

linktarget mozillavpn-uiplugin
optional plugin mozillavpn-uiplugin
classname Mozilla_VPNPlugin
typeinfo mozillavpn-ui.qmltypes
prefer :/Mozilla/VPN/

These are not defined in the lottie and nebula qmldir files -- which work.

I started playing around with options that would change this such as
NO_PLUGIN (which did nothing) and NO_PLUGIN_OPTINAL which fixed the import
issues we were having :shrug.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah, just for the record. After I aded this property
this is what the header looks like:

linktarget mozillavpn-uiplugin
plugin mozillavpn-uiplugin
classname Mozilla_VPNPlugin
typeinfo mozillavpn-ui.qmltypes
prefer :/Mozilla/VPN/

@brizental brizental marked this pull request as ready for review May 13, 2024 12:55
@brizental brizental changed the title VN-6375 - Import some QML files by folder path VN-6375 - Fix QML modules import May 13, 2024
@brizental brizental changed the title VN-6375 - Fix QML modules import VPN-6375 - Fix QML modules import May 13, 2024
@brizental brizental enabled auto-merge (squash) May 13, 2024 16:31
@github-actions github-actions bot added the 🛬 Landing This PR is marked as "auto-merge" label May 13, 2024
Copy link
Member

@lesleyjanenorton lesleyjanenorton left a comment

Choose a reason for hiding this comment

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

👍

@brizental brizental merged commit ce85c66 into main May 13, 2024
113 checks passed
@brizental brizental deleted the 6375-fix branch May 13, 2024 17:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🛬 Landing This PR is marked as "auto-merge"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants