Skip to content

Remove unused hack #20618

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

ratijas
Copy link

@ratijas ratijas commented May 22, 2025

Description

Introduced back in 798c1f1 and unused since 5992e66, this is no longer an appropriate way to perform type checks in QML by modern Qt standards. Just use instanceof operator instead, if needed; it works since Qt 5.10

Source: https://doc.qt.io/qt-6/qtqml-javascript-hostenvironment.html

Type of change

  • Chore/cleanup

How Has This Been Tested?

  • Search for the function name in all project files, find none.

Checklist:

Introduced back in 798c1f1 and unused
since 5992e66, this is no longer an
appropriate way to perform type checks in QML by modern Qt standards.
Just use instanceof operator instead, if needed; it works since Qt 5.10

Source: https://doc.qt.io/qt-6/qtqml-javascript-hostenvironment.html
@github-actions github-actions bot added the PR: Community Contribution 👑 Community Contribution PR's label May 22, 2025
Copy link
Contributor

Test Results

23 783 tests  ±0   23 781 ✅ ±0   46s ⏱️ -1s
     1 suites ±0        2 💤 ±0 
     1 files   ±0        0 ❌ ±0 

Results for commit b09a42c. ± Comparison against base commit 43a055c.

@fieldOfView
Copy link
Collaborator

Strictly speaking, removing this function constitutes a major SDK version change, which is a pain because it means that all Marketplace plugins (whether they use the function or not) need to be changed and resubmitted.

Please see if there is a way to keep the function, but make it use the instanceof operator under the hood. Bonus points: have it emit a deprecation warning, though I don't remember if there's a generic way to do that from QML.

@ratijas
Copy link
Author

ratijas commented May 23, 2025

Strictly speaking, removing this function constitutes a major SDK version change, which is a pain because it means that all Marketplace plugins (whether they use the function or not) need to be changed and resubmitted.

Oh, I was under an impression that it was some kind of entry point for the whole application like what would be main.qml in some other projects. Is it a piece of library with external API? (Irrelevant, but it was also literally just the first file I opened in the workspace while searching for .qml components.)

Please see if there is a way to keep the function, but make it use the instanceof operator under the hood. Bonus points: have it emit a deprecation warning, though I don't remember if there's a generic way to do that from QML.

No can do. The function expects a string, but you can't just resolve a string back to a type that it represents. Unfortunately, as of Qt 6.10 QML still lacks any sort of dedicated deprecation syntax/API either, you'd just have to console.warn() or something.

@fieldOfView
Copy link
Collaborator

Points taken. Then it is up to the Cura team to decide if this is minor enough to skip the SDK version update. To be honest, I don't know if any plugin is using the call from their QML.

@jellespijker jellespijker requested a review from Copilot May 23, 2025 15:15
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot wasn't able to review any files in this pull request.

Files not reviewed (1)
  • resources/qml/Cura.qml: Language not supported

@ratijas
Copy link
Author

ratijas commented Jun 7, 2025

Sooo… now what?

If it's gonna stuck forever, I'd rather just close and delete the branch honestly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Community Contribution 👑 Community Contribution PR's
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants