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

QML: Disable automatic qmldir generation to work around QTBUG-100326 #4673

Merged
merged 1 commit into from
Feb 16, 2022

Conversation

Holzhaus
Copy link
Member

This fixes the import of the MathUtils JavaScript library in the Mixxx
QML module. See https://bugreports.qt.io/browse/QTBUG-100326 for details.

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

This works for me on top of #4679

By the way, I have back ported the patch, so it is not required for Ubuntu.

Can we predict in which version the patch will be released? This would allow to install a version guard.

PlayerDropArea 0.0 res/qml/Mixxx/PlayerDropArea.qml
MathUtils 0.0 res/qml/Mixxx/MathUtils.mjs
Copy link
Member

Choose a reason for hiding this comment

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

Qt does it the other way around:

Suggested change
PlayerDropArea 0.0 res/qml/Mixxx/PlayerDropArea.qml
MathUtils 0.0 res/qml/Mixxx/MathUtils.mjs
MathUtils 0.0 res/qml/Mixxx/MathUtils.mjs
PlayerDropArea 0.0 res/qml/Mixxx/PlayerDropArea.qml

Copy link
Member Author

Choose a reason for hiding this comment

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

The order is irrelevant. But sure, we can sort it alphabetically...

linktarget mixxx-libplugin
optional plugin mixxx-libplugin
classname MixxxPlugin
typeinfo mixxx-lib.qmltypes
Copy link
Member

Choose a reason for hiding this comment

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

This is required to keep qmllint happy

Suggested change
typeinfo mixxx-lib.qmltypes
typeinfo mixxx-lib.qmltypes
import QtQuick

Copy link
Member Author

Choose a reason for hiding this comment

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

This is unrelated and should be taken care of in a separate PR, that also adds this import to qt_add_qml_module. The qmldir is the same one that Qt generates automatically, just with the MathUtils line added.

Copy link
Member

Choose a reason for hiding this comment

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

I have already taken care in #4679. So If we merge that first, we should add the import here as well.

This fixes the import of the MathUtils JavaScript library in the Mixxx
QML module. See https://bugreports.qt.io/browse/QTBUG-100326 for details.
Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

Please add the import QtQuick so that we don't need another PR for that.

@Holzhaus
Copy link
Member Author

Holzhaus commented Feb 16, 2022

Please add the import QtQuick so that we don't need another PR for that.

No. As I explained earlier, the goal of this PR is to work around that specific QTBUG. Hence, I added a qmldir that is identical to the generated one except that the JS module is included (i.e. it's exactly how it would look if that Qt bug was fixed).

If I add that import line here, then the generated qmldir and the manual qmldir would be out of sync. And the PR title would be misleading because it also contains other changes that don't fix that QTBUG.

I think it's really bad practice to split bugfixes and slip them into multiple unrelated PRs. #4679 hasn't even been merged yet. If it was, then sure, that line should be added after rebasing this PR.
And if this PR was merged first, you can add that line in #4679 after pulling in main.

By the way, we previously decided that we use "Request Changes" instead of "Comment" if we want to block merge until the fixes from the review comments have been re-reviewed by the same person. So it's basically saying "The PR in the current form is unmergeable and the fix will be so complicated that nobody but me can review it properly." Is this really necessary when the review only contains two comments, one of which asks to swap 2 lines and the other one asks to include unrelated changes?

@daschuer
Copy link
Member

Ok, than I will take care in #4679

@daschuer daschuer merged commit 65abfac into mixxxdj:main Feb 16, 2022
@daschuer daschuer mentioned this pull request Feb 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants