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

Fix message count (discussion) #87

Conversation

pieter-edelman-nictiz
Copy link
Collaborator

Hi Vadim, I noticed that the error/warning count wouldn't always get communicated to the QML side, and then I found your comment about setting the warning count before the error count. I believe your original approach with a general class that gets instantiated is not really compatible with the way Qt/QML signalling works, or at least stresses what it's supposed to do.
Since the ValidationResult is a really simple structure with three elements that gets used just twice, maybe it is best just to write these six resulting elements out directly, like below. This works reliably.
Another approach that works is to first instantiate a new ValidationResult, then set its properties and then bind it to the property that QML can access. But this doesn't allow for changing properties of this object afterwards in any reliable way. Alternatively, you can expand it with explicit signals, but that makes things quite complicated.

@vadi2
Copy link
Member

vadi2 commented Jun 21, 2019

Yes, that's what I suspected, but I had plans in the future for allowing more than just 2 validators - using a remote server, or different versions of the Java validators, hence why I went with the class... I agree it's probably stressing Qml.Net too much and we need to report this as an issue. Haven't created an explicit test case to submit to the project yet. I agree with this change.

@pieter-edelman-nictiz
Copy link
Collaborator Author

That's what I suspected ;)

@pieter-edelman-nictiz
Copy link
Collaborator Author

Maybe it's best just to use a simple approach for now and deal with the more complex situation in a proper way when it becomes relevant? I think it can be done but it needs some TLC.

@vadi2
Copy link
Member

vadi2 commented Jun 21, 2019

TLC?

But yes, I agree on the simpler approach for now.

@vadi2
Copy link
Member

vadi2 commented Jun 21, 2019

I tried a struct initially because that made most sense with my C++ background but things were very weird with them: qmlnet/qmlnet#135 😅

@pieter-edelman-nictiz
Copy link
Collaborator Author

Tender Loving Care I believe ;)

Wow, that's a weird issue. But the class approach isn't working very well either ...

Copy link
Member

@vadi2 vadi2 left a comment

Choose a reason for hiding this comment

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

👍 looks good, feel free to merge!

@vadi2
Copy link
Member

vadi2 commented Jun 24, 2019

Fixes #39

@pieter-edelman-nictiz pieter-edelman-nictiz merged commit 80a5f77 into health-validator:master Jun 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants