[ui] Reduce confusion when qml loading fails #1728
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@fabiencastan Just a friendly ping :-) |
fabiencastan
requested changes
Jul 28, 2022
meshroom/ui/app.py
Outdated
Comment on lines
54
to
65
| # If MESHROOM_OUTPUT_QML_WARNINGS is not set and an error in qml files happen we're left | ||
| # without any output except "QQmlApplicationEngine failed to load component". This is | ||
| # extremely hard to debug to someone who does not know about MESHROOM_OUTPUT_QML_WARNINGS | ||
| # beforehand because by default Qml will output errors to stdout. | ||
| if "QQmlApplicationEngine failed to load component" in message and \ | ||
| not cls.outputQmlWarnings: | ||
| logging.warning("Failed to load QML component, set MESHROOM_OUTPUT_QML_WARNINGS=1 " | ||
| "to debug why") | ||
|
|
||
| # discard blacklisted Qt messages related to QML when 'output qml warnings' is set to false | ||
| if not cls.outputQmlWarnings and any(w in message for w in cls.qmlWarningsBlacklist): | ||
| return |
There was a problem hiding this comment.
Not tested:
Suggested change
| # If MESHROOM_OUTPUT_QML_WARNINGS is not set and an error in qml files happen we're left | |
| # without any output except "QQmlApplicationEngine failed to load component". This is | |
| # extremely hard to debug to someone who does not know about MESHROOM_OUTPUT_QML_WARNINGS | |
| # beforehand because by default Qml will output errors to stdout. | |
| if "QQmlApplicationEngine failed to load component" in message and \ | |
| not cls.outputQmlWarnings: | |
| logging.warning("Failed to load QML component, set MESHROOM_OUTPUT_QML_WARNINGS=1 " | |
| "to debug why") | |
| # discard blacklisted Qt messages related to QML when 'output qml warnings' is set to false | |
| if not cls.outputQmlWarnings and any(w in message for w in cls.qmlWarningsBlacklist): | |
| return | |
| if not cls.outputQmlWarnings: | |
| # If MESHROOM_OUTPUT_QML_WARNINGS is not set and an error in qml files happen we're left | |
| # without any output except "QQmlApplicationEngine failed to load component". This is | |
| # extremely hard to debug to someone who does not know about MESHROOM_OUTPUT_QML_WARNINGS | |
| # beforehand because by default Qml will output errors to stdout. | |
| if "QQmlApplicationEngine failed to load component" in message: | |
| logging.warning("Set MESHROOM_OUTPUT_QML_WARNINGS=1 to get more detailed error message.") | |
| # discard blacklisted Qt messages related to QML when 'output qml warnings' is not enabled | |
| elif any(w in message for w in cls.qmlWarningsBlacklist): | |
| return |
There was a problem hiding this comment.
@p12tic Could you have a look to do this minor update before merge?
p12tic
force-pushed
the
reduce-qml-crash-confusion
branch
from
eb3ea46
to
d3e3bfd
Compare
|
@fabiencastan friendly ping :-) I think this PR is good to go in. |
meshroom/ui/app.py
Outdated
| logging.warning("Failed to load QML component, set MESHROOM_OUTPUT_QML_WARNINGS=1 " | ||
| "to debug why") | ||
|
|
||
| if any(w in message for w in cls.qmlWarningsBlacklist): |
There was a problem hiding this comment.
Suggested change
| if any(w in message for w in cls.qmlWarningsBlacklist): | |
| # discard blacklisted Qt messages related to QML when 'output qml warnings' is not enabled | |
| elif any(w in message for w in cls.qmlWarningsBlacklist): |
There was a problem hiding this comment.
I have not tested it, but "elif" would make sense, isn't it?
There was a problem hiding this comment.
Agreed, this way full original message will be included.
meshroom/ui/app.py
Outdated
Comment on lines
63
to
64
| logging.warning("Failed to load QML component, set MESHROOM_OUTPUT_QML_WARNINGS=1 " | ||
| "to debug why") |
There was a problem hiding this comment.
Suggested change
| logging.warning("Failed to load QML component, set MESHROOM_OUTPUT_QML_WARNINGS=1 " | |
| "to debug why") | |
| logging.warning("Set MESHROOM_OUTPUT_QML_WARNINGS=1 to get a detailed error message.") |
There was a problem hiding this comment.
Addressed. Sorry for not noticing these changes in your original suggestion. I should just have applied it instead of rewriting manually.
p12tic
force-pushed
the
reduce-qml-crash-confusion
branch
from
d3e3bfd
to
0170d01
Compare
Currently we disable all logging out of qml by default. This is problematic in case qml loading fails for any reason (e.g. missing dependencies) as the user will be presented with a message that is not actionable: "QQmlApplicationEngine failed to load component". There is no way to understand what's happening unless the user knows about MESHROOM_OUTPUT_QML_WARNINGS. This is improved by checking for presence of "QQmlApplicationEngine failed to load component" and warning if qml logging is disabled in that case.
p12tic
force-pushed
the
reduce-qml-crash-confusion
branch
from
0170d01
to
9365a37
Compare
fabiencastan
approved these changes
Aug 25, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently we disable all logging out of qml by default. This is problematic in case qml loading fails for any reason (e.g. missing dependencies) as the user will be presented with a message that is not actionable: "QQmlApplicationEngine failed to load component". There is no way to understand what's happening unless the user knows about MESHROOM_OUTPUT_QML_WARNINGS.
This is improved by checking for presence of "QQmlApplicationEngine failed to load component" and warning if qml logging is disabled in that case.
There has been many issues with this log message and no further logs in the bug tracker, so I'm not the only one who hit this issue. Hopefully this improves debugging experience for everyone.