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 #304, increase QML initialization waiting time #305

Merged
merged 5 commits into from
Oct 6, 2018

Conversation

davidlatwe
Copy link
Collaborator

Problem

In previous PR #304, the new feature that allow user to stop collecting process at startup, will make GUI stuck at "Collecting.." phase if the collecting process ended in short time (before QML get initialized).

That feature rely on QML to receive the signal firstRun to reset those button, so if the signal firstRun emitted before QML complete initialization, GUI stocked.

Fix

Increase QML initialization waiting time from 500 to 1500, give it enough time to be ready to receive signal firstRun

@davidlatwe davidlatwe requested review from BigRoy and mottosso October 2, 2018 15:29
@davidlatwe
Copy link
Collaborator Author

With the increased waiting time, the state machine is now able to work right on the first reset, Footer.startup flag and other extra code are no longer needed.

@davidlatwe
Copy link
Collaborator Author

davidlatwe commented Oct 3, 2018

I just realized that there were some logic conflict in my description about the bug, I miss understood it.
It does relate to video card's work load, but the cause of the bug wasn't quite right.

In PR #304, I added these code in pyblish_qml/qml/Overiew.qml here

        mode: {
            if (startup == true) {
                setMessage("Collecting..")
                return 1
            }
            return overview.state == "publishing" ? 1 : overview.state == "finished" ? 2 : 0
        }

This was a part of code for implementing stoppable collecting process.
And this was the True Bug.

QML initialization waiting time not long enough only made the QML Footer not able to display stop button and "Collecting.." "Ready" message on startup, like this :

test

but won't block the process, so the fix code above just made it worse..

Therefore..

That feature rely on QML to receive the signal firstRun to reset those button, so if the signal firstRun emitted before QML complete initialization, GUI stocked.

This is not right, it didn't rely on that, but I make it does, and the bug pops.

Anyway, the bug isn't exists anymore, although increasing QML initialization waiting time wasn't the real medicine to fix (commit 786ca30 was the true fix), but did increase the chance to display GUI right.

If some how the wait-time value 1500 still not enough due to heavy background video card work load, the worst case was only not able to display stop button and "Collecting.." "Ready" message on startup.

Does this make sense to you ?
Sorry again, for this boo-boo I made. :(

mode: {
if (startup == true) {
setMessage("Collecting..")
return 1
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was the true bug.

@BigRoy
Copy link
Member

BigRoy commented Oct 3, 2018

Thanks @davidlatwe - I'll play with this hopefully soon to test it. Just wondering, is the longer 1500 delay still needed if it's fixed already? What's the current worst case?

@davidlatwe
Copy link
Collaborator Author

Thanks @BigRoy :)

is the longer 1500 delay still needed if it's fixed already?

Not really needed, but would increase the chance that QML event handler get initialized to catch signal firstRun if background workload making it slow response.

What's the current worst case?

I think you meant 500 delay ? If so, then the worst case would be the first collecting process will startup with the GUI below, instead of "Collecting.." message and a stop button :

test

No messages and all the buttons were early exposed, not really serious.

@BigRoy
Copy link
Member

BigRoy commented Oct 5, 2018

Seems to be working fine on my machine here. Putting this live in production Today, if anything pops up I'll let you know.

@davidlatwe
Copy link
Collaborator Author

davidlatwe commented Oct 5, 2018

Thanks @BigRoy ! Great news !

@BigRoy
Copy link
Member

BigRoy commented Oct 5, 2018

It seems to work nicely here.

I did get this comment from one of the artists Today though:

I've had pyblish stall on repair actions btw, happened a couple of times today. Not sure why, but it just hangs on 'busy'

That would be the right click menu on plug-ins that trigger an action.
I think it's unrelated to this particular feature (just thought I'd mention it here), but have you seen that happen too maybe?

@davidlatwe
Copy link
Collaborator Author

Hmm, No, I haven't.
I do have some repair actions on hand, but didn't hanging in pass few day :)

@davidlatwe
Copy link
Collaborator Author

Yep, just checked the change again, should be unrelated, I could bet a case of beer 🍺 , haha
Thanks for approval @BigRoy : )
Then I will merge this tomorrow, if there's no other issue pops up.

@BigRoy
Copy link
Member

BigRoy commented Oct 5, 2018

Yep, just checked the change again, should be unrelated, I could bet a case of beer beer , haha

With how often artists are wrong I'd stay away from joining that bet. Nice work again David, thanks. 🍻

@davidlatwe
Copy link
Collaborator Author

Merging !

@davidlatwe davidlatwe merged commit 5b0b092 into pyblish:master Oct 6, 2018
@davidlatwe davidlatwe deleted the fix-304 branch October 6, 2018 13:09
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.

2 participants