-
Notifications
You must be signed in to change notification settings - Fork 757
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
sclang: refactor server menu #3870
Conversation
This fixes issues where server menu flashes when updating because we were detroying the entire thing to rebuild it - now we just incrementally update.
This is causing some breakage for me:
|
@telephon can you explain why you approved this PR given my comment above? |
Does this PR depend on the fixes from #3871? |
|
I approved of it because it fixes issues that scott discovered in his own code. I don't want to hold up patches to peoples own contributions. I didn't have this failure for some reason. |
@telephon try testing this out against 3.10 and see if you get the same breakage. |
now I get it, too. I did a bit of digging (it is not so convenient that the error doesn't say what call actually failed), I found that it is here: insertAction {
|before, action|
if (before.isKindOf(Number)) {
before = this.actions[before]
};
^this.invokeMethod('insertAction', before, action.asMenuAction); // <--- this is probably wrong, because invokeMethod takes a boolean as third argument.
} |
invokeMethod { arg method, arguments, synchronous = true;
_QObject_InvokeMethod
^this.primitiveFailed
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to also fix the bug in the invokeMethod call (see conversation).
Ah here I found the fix, which is in develop: #3871 |
#3871 is now in |
sorry, I don't understand the abbreviation. |
continuous integration |
@telephon have your requests been met? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. one small bug which I will address in a separate PR
This fixes issues where server menu flashes when updating because we were detroying the entire thing to rebuild it - now we just incrementally update.