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.

Sign up for GitHub

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

Jump to bottom

Bug 172777 - Apply workarounds to known Qt JS issues #779

Merged
merged 8 commits into from
Sep 30, 2021
Merged

Bug 172777 - Apply workarounds to known Qt JS issues #779

merged 8 commits into from
Sep 30, 2021

Conversation

brizental
Copy link
Contributor

@brizental brizental commented Sep 30, 2021 •
edited
Loading

I just took the relevant commits from #744

@brizental
Make sure currentJob is never undefined on the Dispatcher
29f1318 
I thought this would fix some of the errors seen on Qt + iOS.
It did not, but it is a nice little imporvement, so I leave it.
@brizental
Stoping passing the metricsDatabase around and prefer using Context
68bf901 
This seems to fix _some_ of the errors in generated in QML.
Possibly related to https://bugreports.qt.io/browse/QTBUG-50319
@brizental
Avoid using this inside inner scope of arrow function
46da332 
This is due to a bug in the QML Javascript interpreter:
https://bugreports.qt.io/browse/QTBUG-95677

I tried to audit all the situations we use `this` in the code,
but it seems the dispatcher occurance is really the only one.
@brizental
Do not use same identifier as function name inside getter
6176865 
Related to: https://bugreports.qt.io/browse/QTBUG-96625
@brizental
Remove unnecessary log from dispatcher, update changelog
13c9ac6
@auto-assign auto-assign bot requested a review from chutten September 30, 2021 16:08
@brizental brizental removed the request for review from chutten September 30, 2021 16:08
@brizental brizental changed the title Bug 172777 - Apply workarounds to know Qt JS issues Bug 172777 - Apply workarounds to known Qt JS issues Sep 30, 2021
@brizental brizental mentioned this pull request Sep 30, 2021
Closed
@brizental brizental requested a review from mdboom September 30, 2021 16:17
mdboom
mdboom approved these changes Sep 30, 2021
View reviewed changes
Copy link
Contributor

@mdboom mdboom left a comment

Choose a reason for hiding this comment

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

I thought about adding comments like "this is a workaround for Qt", but there would be a lot of them and git blame should help remind us "why is this so weird..."

mdboom
mdboom approved these changes Sep 30, 2021
View reviewed changes
CHANGELOG.md Show resolved Hide resolved
@mdboom mdboom merged commit 0a6124e into main Sep 30, 2021
@brizental brizental deleted the 172777-qt-fixes branch October 11, 2021 09:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mdboom mdboom mdboom approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@brizental @mdboom