Help Browser code execution #4883

elgiano · 2020-04-19T21:23:50Z

Purpose and Motivation

Explained more in detail in an RFC and open for discussion. Please let's discuss it, I don't want to push this in any way. I also would really benefit from some other pairs of eyes on it. And it needs testing on Windows.
RFC: https://github.com/elgiano/rfcs/blob/helpbrowser/rfcs/0009-helpbrowser-code-evaluation.md

fixed code execution in both HelpBrowser class and Widget + in WebView, if the rendered webpage has selectLine()/selectRegion() javascript functions. It works with selected text/line, and with selected text/region.
code execution in webviews is more consistently delegated to javascript, calling a function to get selected text, receiving it in a callback and then sending it to the interpreter
a workaround WebView's "funny" keypress behavior, now keyDownAction works on WebView only when an url is set. It gets all events (including the ones swallowed by the renderer) without duplication. WebView parent widgets also get all keystrokes, except the ones swallowed by the web renderer when focusing an input field (like Shift+Enter).
Using QcWebView in ScIDE Help Browser Widget

Types of changes

Bug fix

To-do list

Code is tested
It works on Linux and Mac.~~Missing testing on Windows~~. Also on Windows, thanks @dyfer
All tests are passing
Updated documentation
This PR is ready for review

dyfer · 2020-04-20T21:35:27Z

I've built it on Windows and tested code execution in the Help Browser - works fine. I haven't had a chance to look deeper yet, but doesn't seem like there are any apparent issues with Windows at first glance.

elgiano · 2020-04-20T23:14:11Z

thanks for testing it @dyfer!

elgiano · 2020-08-24T14:13:41Z

Rebased on 3.11

mossheim

thanks! i haven't tested it yet but i have a few questions about the code before reviewing further

HelpSource/editor.js

editors/sc-ide/widgets/help_browser.cpp

QtCollider/widgets/QcWebView.cpp

mossheim · 2020-10-21T14:16:03Z

QtCollider/widgets/QcWebView.cpp

@@ -159,7 +158,8 @@ void WebView::toPlainText(QcCallback* cb) const {
 void WebView::runJavaScript(const QString& script, QcCallback* cb) {
    if (page()) {
        if (cb) {
-            page()->runJavaScript(script, cb->asFunctor());
+            // convert QVariant to string until we deal with QVariants
+            page()->runJavaScript(script, [cb](const QVariant& t) { cb->call(t.toString()); });


i don't understand this comment

Fair enough :) Sorry for being criptic, now I also have to look it up again. We have a problem when passing a QVariant back into sclang:

runJavascript takes a callback function, which gets converted to this QcCallback

when the callback is called, sclang throws an error:
ERROR: Qt: Failed to write a slot when trying to run interpreter!

If anyone feels like trying it out, this code errors without this PR:

WebView().runJavaScript("1+1"){|res,e|res.class.postln}

This problem does not happen when we convert to string before calling the QCallback. I can definitely look deeper into this, and suggestions are very welcome :)

Ok I've investigated a bit more and I think this maybe deserves another PR. I'll try to explain here:

JavaScript can return a variety of types to sclang, so it gives a QVariant as argument to the callback.

The callback fires, emitting a signal that is caught by a QSignalSpy

QSignalSpy gets the argument as a void* (but it also gets its type), and throws it into a QVariant. Now we have a QVariant<QVariant>

QSignalSpy sends the arguments to QtCollider::runLang, which calls QtCollider::set, which fails for a QVariant<QVariant>

I have a fix for QSignalSpy to avoid creating a QVariant<QVariant> which makes possible to return Numbers, Strings and Arrays. The only type that still fails is QMap. I haven't tested if this code has consequences on any other call, it looks like a legit bugfix for me, I'm tempted to include it here... what do you think would be best?

thanks for looking into it! since this is the last PR before the 3.11.2 release, let's just leave this as it is. if you're interested you can make another PR for it, i'd also be happy to take a look myself.

ok, we keep it like it is then and I'll open another PR! Just to be extra clear, now all javascript calls from sclang will always return a string only

QtCollider/widgets/QcWebView.h

SCClassLibrary/Common/GUI/tools/HelpBrowser.sc

editors/sc-ide/CMakeLists.txt

editors/sc-ide/widgets/help_browser.cpp

mossheim · 2020-10-22T14:09:27Z

thanks for the quick update @elgiano . if you can address https://github.com/supercollider/supercollider/pull/4883/files#r509325107 i'm ready to review the rest of the PR after that :)

elgiano · 2020-10-22T14:39:46Z

Yeah, thanks for looking at it @brianlheim!
The one you point out is the one I don't really understand. What do you suggest I do about it? Shall I make the class hierarchy (my idea), or could you explain a bit more what your proposal is?
Thankssssssssssssss

elgiano · 2020-10-22T20:38:58Z

I think I got what you meant now! I pushed all changes for your pre-review, I hope it's fine now :)

mossheim

i think this approach makes sense overall. i have a few questions and then some comments assuming this is the way we want to proceed

QtCollider/widgets/QcWebView.cpp

QtCollider/widgets/QcWebView.h

editors/sc-ide/widgets/help_browser.cpp

SCClassLibrary/Common/GUI/Base/QWebView.sc

QtCollider/widgets/QcWebView.cpp

QtCollider/widgets/QcWebView.h

mossheim

thanks @elgiano ! the code all looks good except for one whitespace thing. i want to test it out before approving/merging.

mossheim · 2020-10-24T21:11:40Z

SCClassLibrary/Common/GUI/Base/QWebView.sc

-		<onSelectionChanged, <onTitleChanged, <onUrlChanged, <onScrollPositionChanged, <onContentsSizeChanged, <onAudioMutedChanged,
-		<onRecentlyAudibleChanged;
+	<onSelectionChanged, <onTitleChanged, <onUrlChanged, <onScrollPositionChanged, <onContentsSizeChanged, <onAudioMutedChanged,
+	<onRecentlyAudibleChanged, <enterInterpretsSelection = false;


please undo the whitespace changes

mossheim · 2020-10-24T21:19:33Z

QtCollider/widgets/QcWebView.cpp

@@ -159,7 +158,8 @@ void WebView::toPlainText(QcCallback* cb) const {
 void WebView::runJavaScript(const QString& script, QcCallback* cb) {
    if (page()) {
        if (cb) {
-            page()->runJavaScript(script, cb->asFunctor());
+            // convert QVariant to string until we deal with QVariants
+            page()->runJavaScript(script, [cb](const QVariant& t) { cb->call(t.toString()); });


thanks for looking into it! since this is the last PR before the 3.11.2 release, let's just leave this as it is. if you're interested you can make another PR for it, i'd also be happy to take a look myself.

QtCollider/widgets/QcWebView.h

mossheim · 2020-10-24T21:29:20Z

QtCollider/factories.cpp

@@ -26,6 +26,11 @@

 QC_DECLARE_QWIDGET_FACTORY(QLabel);

+#ifdef SC_USE_QTWEBENGINE
+#   include "widgets/QcWebView.h"   


aghh, you need to add a space here for linting

Suggested change

# include "widgets/QcWebView.h"

# include "widgets/QcWebView.h"

mossheim · 2020-10-24T21:32:30Z

@elgiano do we use websockets for anything at all after this PR?

elgiano · 2020-10-24T23:45:18Z

do we use websockets for anything at all after this PR?

After a little grep I haven't found any place where we are using them, which I also suspected.

mossheim · 2020-10-25T00:29:54Z

HelpSource/editor.js

@@ -38,7 +38,10 @@ const init = () => {
            lineWrapping: true,
            viewportMargin: Infinity,
            extraKeys: {
-                'Shift-Enter': evalLine
+                // noop: prevent both codemirror and the browser to handle Shift-Enter
+                'Shift-Enter': ()=>false 


missing a comma here

yesss, thanks for catching it! btw I changed to ()=>{} which looks like a more proper noop

mossheim

thanks again! i think this is all good. now that we are not using the websocket code, i would like to remove that too, but that can be done on the develop branch after the 3.11.2 release.

i also tested the latest version myself on macOS 10.14, windows 10, and arch linux, all fine.

elgiano · 2020-10-25T09:07:02Z

Thanks a lot @brianlheim and @dyfer!!!

elgiano changed the title ~~Webview~~ Help Browser code execution Apr 19, 2020

elgiano force-pushed the webview branch 2 times, most recently from 681b0a1 to cfa4c63 Compare April 20, 2020 00:31

mossheim added the env: SCIDE label Apr 21, 2020

elgiano force-pushed the webview branch 3 times, most recently from 4876831 to dd568e7 Compare July 1, 2020 22:36

elgiano force-pushed the webview branch 3 times, most recently from 6d8dd76 to 5ef0990 Compare July 21, 2020 12:09

elgiano added 2 commits August 24, 2020 16:07

webview keypress + js string callback

3b7edaa

HelpBrowser Widget: use QcWebView

e5ece0c

elgiano force-pushed the webview branch from 5ef0990 to e5ece0c Compare August 24, 2020 14:11

elgiano changed the base branch from develop to 3.11 August 24, 2020 14:13

mossheim mentioned this pull request Aug 25, 2020

3.11.2 Discussions #5146

Closed

mossheim reviewed Oct 21, 2020

View reviewed changes

elgiano force-pushed the webview branch from 1cc4ede to 424c2de Compare October 22, 2020 20:38

elgiano force-pushed the webview branch from 424c2de to 83fb247 Compare October 22, 2020 20:42

pre-review

2a4937a

elgiano force-pushed the webview branch from 83fb247 to 2a4937a Compare October 22, 2020 22:23

mossheim suggested changes Oct 22, 2020

View reviewed changes

elgiano force-pushed the webview branch 2 times, most recently from 8fbbb3a to cd869fa Compare October 23, 2020 02:24

mossheim reviewed Oct 24, 2020

View reviewed changes

under review

bb1077d

elgiano force-pushed the webview branch from cd869fa to bb1077d Compare October 24, 2020 23:39

mossheim reviewed Oct 25, 2020

View reviewed changes

elgiano force-pushed the webview branch from 80093b6 to 5909101 Compare October 25, 2020 00:36

fix shift-enter for sclang HelpBrowser

bb39141

elgiano force-pushed the webview branch from 5909101 to bb39141 Compare October 25, 2020 00:38

elgiano mentioned this pull request Oct 25, 2020

qtcollider: QcSignalSpy prevent creating QVariant<QVariant> #5216

Merged

4 tasks

mossheim approved these changes Oct 25, 2020

View reviewed changes

mossheim merged commit 0c16681 into supercollider:3.11 Oct 25, 2020

This was referenced Oct 25, 2020

sclang WebView/HelpBrowser can't evaluate code #4000

Closed

help browser: shift+enter doesn't evaluate code #4542

Closed

elgiano deleted the webview branch October 25, 2020 09:06

mossheim mentioned this pull request Oct 28, 2020

Merge 3.11 into develop #5223

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Help Browser code execution #4883

Help Browser code execution #4883

elgiano commented Apr 19, 2020 •

edited

Loading

dyfer commented Apr 20, 2020

elgiano commented Apr 20, 2020

elgiano commented Aug 24, 2020

mossheim left a comment

mossheim Oct 21, 2020

elgiano Oct 22, 2020

elgiano Oct 22, 2020

mossheim Oct 24, 2020

elgiano Oct 24, 2020

mossheim commented Oct 22, 2020

elgiano commented Oct 22, 2020

elgiano commented Oct 22, 2020

mossheim left a comment

mossheim left a comment •

edited

Loading

mossheim Oct 24, 2020

mossheim Oct 24, 2020

mossheim Oct 24, 2020

mossheim commented Oct 24, 2020

elgiano commented Oct 24, 2020 •

edited

Loading

mossheim Oct 25, 2020

elgiano Oct 25, 2020

mossheim left a comment

elgiano commented Oct 25, 2020

	# include "widgets/QcWebView.h"
	# include "widgets/QcWebView.h"

Help Browser code execution #4883

Help Browser code execution #4883

Conversation

elgiano commented Apr 19, 2020 • edited Loading

Purpose and Motivation

Types of changes

To-do list

dyfer commented Apr 20, 2020

elgiano commented Apr 20, 2020

elgiano commented Aug 24, 2020

mossheim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mossheim commented Oct 22, 2020

elgiano commented Oct 22, 2020

elgiano commented Oct 22, 2020

mossheim left a comment

Choose a reason for hiding this comment

mossheim left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mossheim commented Oct 24, 2020

elgiano commented Oct 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mossheim left a comment

Choose a reason for hiding this comment

elgiano commented Oct 25, 2020

elgiano commented Apr 19, 2020 •

edited

Loading

mossheim left a comment •

edited

Loading

elgiano commented Oct 24, 2020 •

edited

Loading