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

fix misconnected slot in WebView #4488

Merged
merged 1 commit into from
Aug 11, 2019
Merged

fix misconnected slot in WebView #4488

merged 1 commit into from
Aug 11, 2019

Conversation

patrickdupuis
Copy link
Contributor

@patrickdupuis patrickdupuis commented Jul 14, 2019 •
edited
Loading

Purpose and Motivation

This PR fixes an error message that is posted when opening HelpBrowser.instance in command line sclang.

QObject::connect: No such slot QtCollider::WebView::onRenderProcessTerminated(RenderProcessTerminationStatus, int)

Adding a new SLOT called onRenderProcessTerminated to the WebPage class seems to fix this. I've also specified that this SIGNAL and corresponding SLOT take a QWebEnginePage::RenderProcessTerminationStatus as first argument.

@jrsurge posted a proper fix in his comment. I've updated the PR accordingly.

Possibly related to some behaviour mentioned in #4256 This PR isn't a fix for any specific issue.

Types of changes

  • Bug fix

To-do list

  • Code is tested
  • All tests are passing
  • Updated documentation
  • This PR is ready for review

@patrickdupuis patrickdupuis added bug Issues that relate to unexpected/unwanted behavior. Don't use for PRs. qt5 comp: help schelp documentation labels Jul 14, 2019
@patrickdupuis patrickdupuis added this to the 3.10.3 milestone Jul 14, 2019
@jrsurge jrsurge self-requested a review July 26, 2019 10:41
jrsurge
jrsurge requested changes Jul 27, 2019
View reviewed changes
Copy link
Member

@jrsurge jrsurge left a comment

Choose a reason for hiding this comment

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

The current solution creates a new signal and connects that to the slot.
This will clear the error message, because it's a valid connection, but it's not the signal being emitted by the internal QWebEnginePage methods, so it will never actually get triggered.

The only way I can make this work is to use the functor-style connection (from Qt 5.x) rather than the older string-style approach. The functor-style can resolve namespaces and do implicit conversions, which is what is required here. For reference: https://doc.qt.io/qt-5/signalsandslots-syntaxes.html

Using the functor-style, the only change required should be:

- connect(page, SIGNAL(renderProcessTerminated(RenderProcessTerminationStatus, int)), this,
-            SLOT(onRenderProcessTerminated(RenderProcessTerminationStatus, int)));
+ connect(page, &WebPage::renderProcessTerminated, this, &WebView::onRenderProcessTerminated);

As a bonus, it looks a lot cleaner too?

@patrickdupuis
Copy link
Contributor Author

Awesome @jrsurge! I'll give it a try and update the PR.

@patrickdupuis patrickdupuis force-pushed the topic/renderProcessTerminated branch from 490752d to 2205c9f Compare July 28, 2019 16:48
@nhthn nhthn removed this from the 3.10.3 milestone Aug 1, 2019
mossheim
mossheim approved these changes Aug 11, 2019
View reviewed changes
Copy link
Contributor

@mossheim mossheim left a comment

Choose a reason for hiding this comment

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

awesome, thanks!

@mossheim mossheim merged commit 2115667 into supercollider:3.10 Aug 11, 2019
@mossheim mossheim changed the title add missing SIGNAL renderProcessTerminated fix misconnected slot in WebView Aug 11, 2019
@patrickdupuis patrickdupuis deleted the topic/renderProcessTerminated branch August 12, 2019 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mossheim mossheim mossheim approved these changes

@jrsurge jrsurge jrsurge approved these changes

Assignees
No one assigned
Labels
bug Issues that relate to unexpected/unwanted behavior. Don't use for PRs. comp: help schelp documentation qt5
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@patrickdupuis @jrsurge @mossheim @nhthn