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

Help Browser code execution #4883

Merged
merged 5 commits into from
Oct 25, 2020
Merged

Help Browser code execution #4883

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3b7edaa
webview keypress + js string callback
elgiano Apr 19, 2020
e5ece0c
HelpBrowser Widget: use QcWebView
elgiano Apr 19, 2020
2a4937a
pre-review
elgiano Oct 22, 2020
bb1077d
under review
elgiano Oct 24, 2020
bb39141
fix shift-enter for sclang HelpBrowser
elgiano Oct 25, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 4 additions & 10 deletions HelpSource/editor.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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': ()=>{},
// prevent only codemirror to handle Ctrl+D
'Ctrl-D': false
mossheim marked this conversation as resolved.
Show resolved Hide resolved
}
})

Expand All @@ -59,15 +62,6 @@ const init = () => {

}

const evalLine = () => {
// If we are not running in the SC IDE, do nothing.
if (!window.IDE) {
return;
}
// Ask IDE to eval line. Calls back to `selectLine()`
window.IDE.evaluateLine();
}

/* returns the code selection, line or region */
const selectRegion = (options = { flash: true }) => {
let range = window.getSelection().getRangeAt(0)
Expand Down
5 changes: 5 additions & 0 deletions QtCollider/factories.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,11 @@

QC_DECLARE_QWIDGET_FACTORY(QLabel);

#ifdef SC_USE_QTWEBENGINE
# include "widgets/QcWebView.h"
QC_DECLARE_QWIDGET_FACTORY(WebView);
#endif

static void doLoadFactories() {
QC_ADD_FACTORY(QcDefaultWidget);
QC_ADD_FACTORY(QcHLayoutWidget);
Expand Down
56 changes: 28 additions & 28 deletions QtCollider/widgets/QcWebView.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@

# include "QcWebView.h"
# include "../widgets/web_page.hpp"
# include "../QcWidgetFactory.h"
# include <QWebEnginePage>
# include <QWebEngineSettings>
# include <QWebEngineContextMenuData>
Expand All @@ -35,13 +34,10 @@
# include <QStyle>
# include <QWebEngineCallback>

QC_DECLARE_QWIDGET_FACTORY(WebView);

namespace QtCollider {

WebView::WebView(QWidget* parent): QWebEngineView(parent), _interpretSelection(false), _editable(false) {
WebView::WebView(QWidget* parent): QWebEngineView(parent), _editable(false) {
QtCollider::WebPage* page = new WebPage(this);
page->setDelegateReload(true);
setPage(page);
connectPage(page);

Expand All @@ -55,9 +51,7 @@ WebView::WebView(QWidget* parent): QWebEngineView(parent), _interpretSelection(f
page->action(QWebEnginePage::Paste)->setShortcut(QKeySequence::Paste);
page->action(QWebEnginePage::Reload)->setShortcut(QKeySequence::Refresh);

connect(this, SIGNAL(interpret(QString)), qApp, SLOT(interpret(QString)), Qt::QueuedConnection);

connect(this, SIGNAL(loadFinished(bool)), this, SLOT(updateEditable(bool)));
connect(this, SIGNAL(loadFinished(bool)), this, SLOT(pageLoaded(bool)));
}

void WebView::connectPage(QtCollider::WebPage* page) {
Expand Down Expand Up @@ -159,7 +153,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()); });
Copy link
Contributor

Choose a reason for hiding this comment

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

i don't understand this comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

mossheim marked this conversation as resolved.
Show resolved Hide resolved
} else {
page()->runJavaScript(script, [](const QVariant&) {});
}
Expand Down Expand Up @@ -228,29 +223,34 @@ void WebView::contextMenuEvent(QContextMenuEvent* event) {
menu.exec(event->globalPos());
}

void WebView::keyPressEvent(QKeyEvent* e) {
int key = e->key();
int mods = e->modifiers();

if (_interpretSelection
&& (key == Qt::Key_Enter || (key == Qt::Key_Return && mods & (Qt::ControlModifier | Qt::ShiftModifier)))) {
QString selection = selectedText();
if (!selection.isEmpty()) {
Q_EMIT(interpret(selection));
return;
}
// webView's renderer keypresses don't arrive to webView
// duplicate them
bool WebView::eventFilter(QObject* obj, QEvent* event) {
if (event->type() == QEvent::KeyPress) {
// takes ownership of newEvent
QApplication::postEvent(this, new QKeyEvent(*static_cast<QKeyEvent*>(event)));
}

QWebEngineView::keyPressEvent(e);
event->ignore();
return false;
}

void WebView::updateEditable(bool ok) {
if (ok) {
if (_editable) {
page()->runJavaScript("document.documentElement.contentEditable = true");
} else {
page()->runJavaScript("document.documentElement.contentEditable = false");
}
// stop keypresses here to avoid duplicates in parents
bool WebView::event(QEvent* ev) {
if (ev->type() == QEvent::KeyPress)
return true;

return QWebEngineView::event(ev);
}

void WebView::pageLoaded(bool ok) { this->focusProxy()->installEventFilter(this); }

void WebView::setEditable(bool b) {
_editable = b;
if (_editable) {
page()->runJavaScript("document.documentElement.contentEditable = true");
} else {
page()->runJavaScript("document.documentElement.contentEditable = false");
}
}

Expand Down
22 changes: 7 additions & 15 deletions QtCollider/widgets/QcWebView.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ class WebView : public QWebEngineView {

public:
Q_INVOKABLE void setFontFamily(int genericFontFamily, const QString& fontFamily);
Q_INVOKABLE void triggerPageAction(int action, bool checked);
Q_INVOKABLE void triggerPageAction(int action, bool checked = false);
mossheim marked this conversation as resolved.
Show resolved Hide resolved
Q_INVOKABLE QAction* pageAction(QWebEnginePage::WebAction) const;

// QWebEnginePage forwards
Expand All @@ -58,13 +58,12 @@ class WebView : public QWebEngineView {
Q_INVOKABLE void navigate(const QString& url);

public Q_SLOTS:
void findText(const QString& searchText, bool reversed, QtCollider::QcCallback* cb);
mossheim marked this conversation as resolved.
Show resolved Hide resolved
void findText(const QString& searchText, bool reversed, QtCollider::QcCallback* cb = nullptr);
mossheim marked this conversation as resolved.
Show resolved Hide resolved

Q_SIGNALS:
void linkActivated(const QString&, int, bool);
void jsConsoleMsg(const QString&, int, const QString&);
void reloadTriggered(const QString&);
void interpret(const QString& code);

// QWebEnginePage forwards
void linkHovered(const QString& url);
Expand Down Expand Up @@ -99,16 +98,9 @@ public Q_SLOTS:
bool delegateReload() const;
void setDelegateReload(bool);

Q_PROPERTY(bool enterInterpretsSelection READ interpretSelection WRITE setInterpretSelection);
bool interpretSelection() const { return _interpretSelection; }
void setInterpretSelection(bool b) { _interpretSelection = b; }

Q_PROPERTY(bool editable READ editable WRITE setEditable);
bool editable() const { return _editable; }
void setEditable(bool b) {
_editable = b;
updateEditable(true);
}
void setEditable(bool b);

// QWebEnginePage properties
Q_PROPERTY(QString requestedUrl READ requestedUrl)
Expand Down Expand Up @@ -140,19 +132,19 @@ public Q_SLOTS:
inline static QUrl urlFromString(const QString& str) { return QUrl::fromUserInput(str); }

protected:
virtual void keyPressEvent(QKeyEvent*);
virtual void contextMenuEvent(QContextMenuEvent*);
void contextMenuEvent(QContextMenuEvent*) override;
bool event(QEvent* ev) override;
bool eventFilter(QObject* obj, QEvent* event) override;

public Q_SLOTS:
void onPageReload();
void onRenderProcessTerminated(QWebEnginePage::RenderProcessTerminationStatus, int);
void onLinkClicked(const QUrl&, QWebEnginePage::NavigationType, bool);
void updateEditable(bool);
void pageLoaded(bool);

private:
void connectPage(QtCollider::WebPage* page);

bool _interpretSelection;
bool _editable;
};

Expand Down
24 changes: 21 additions & 3 deletions SCClassLibrary/Common/GUI/Base/QWebView.sc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ WebView : View {

var <onLoadFinished, <onLoadFailed, <onLoadProgress, <onLoadStarted, <onLinkActivated, <onLinkHovered, <onReloadTriggered, <onJavaScriptMsg,
<onSelectionChanged, <onTitleChanged, <onUrlChanged, <onScrollPositionChanged, <onContentsSizeChanged, <onAudioMutedChanged,
<onRecentlyAudibleChanged;
<onRecentlyAudibleChanged, <enterInterpretsSelection = false;

*qtClass { ^'QtCollider::WebView'; }

Expand Down Expand Up @@ -107,6 +107,7 @@ WebView : View {
onReloadTriggered = func;
}

// this function is called by a javascript callback: see enterInterpretsSelection below
onInterpret {
|code|
code.interpret();
Expand Down Expand Up @@ -223,8 +224,25 @@ WebView : View {
url { ^this.getProperty('url') }
url_ { |url| this.setProperty('url', url) }

enterInterpretsSelection { ^this.getProperty('enterInterpretsSelection') }
enterInterpretsSelection_ { |b| this.setProperty('enterInterpretsSelection', b) }
enterInterpretsSelection_ { |b|
enterInterpretsSelection = b;
if(enterInterpretsSelection) {
this.keyDownAction = { |view, char, mods, u, keycode, key|
// 01000004 is Qt's keycode for Enter, needed on Mac
if((char == Char.ret).or(key == 0x01000004)){
case
{ mods.isShift } {
view.runJavaScript("selectLine()", this.onInterpret(_));
}
{ mods.isCtrl || mods.isCmd }{
view.runJavaScript("selectRegion()", this.onInterpret(_));
}
}
};
}{
this.keyDownAction = {}
}
mossheim marked this conversation as resolved.
Show resolved Hide resolved
}

editable { ^this.getProperty('editable') }
editable_ { |b| this.setProperty('editable', b) }
Expand Down
25 changes: 10 additions & 15 deletions SCClassLibrary/Common/GUI/tools/HelpBrowser.sc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,15 +184,15 @@ HelpBrowser {
.resize_(1)
.string_(str)
.value_(openNewWin)
.action_({ |b| openNewWin = b.value; webView.overrideNavigation = openNewWin; });
.action_({ |b| openNewWin = b.value; });
} {
str = "Open links in same window";
w = str.bounds.width + 5;
Button.new( window, Rect(x, y, w, h) )
.resize_(1)
.states_([[str],["Open links in new window"]])
.value_(openNewWin.asInteger)
.action_({ |b| openNewWin = b.value.asBoolean; webView.overrideNavigation = openNewWin; });
.action_({ |b| openNewWin = b.value.asBoolean; });
};

x = 0;
Expand Down Expand Up @@ -223,6 +223,8 @@ HelpBrowser {
{ "Monospace" }
));

webView.overrideNavigation = true;

webView.onLoadFinished = {
this.stopAnim;
window.name = "SuperCollider Help: %".format(webView.title);
Expand All @@ -233,16 +235,13 @@ HelpBrowser {
webView.onLinkActivated = {|wv, url|
var redirected, newPath, oldPath;
redirected = this.redirectTextFile(url);

if (not(redirected)) {
if(openNewWin) {
#newPath, oldPath = [url,webView.url].collect {|x|
if(x.notEmpty) {x.findRegexp("(^\\w+://)?([^#]+)(#.*)?")[1..].flop[1][1]}
};

#newPath, oldPath = [url,webView.url].collect {|x|
if(x.notEmpty) {x.findRegexp("(^\\w+://)?([^#]+)(#.*)?")[1..].flop[1][1]}
};
if(newPath!=oldPath) {
HelpBrowser.new(newWin:true).goTo(url);

if(newPath != oldPath && openNewWin) {
HelpBrowser.new(newWin:openNewWin).goTo(url);
mossheim marked this conversation as resolved.
Show resolved Hide resolved
} {
this.goTo(url);
};
Expand Down Expand Up @@ -272,11 +271,7 @@ HelpBrowser {
};

webView.enterInterpretsSelection = true;
webView.keyDownAction = { arg view, char, mods;
if( (char.ascii == 13) && (mods.isCtrl || mods.isCmd || mods.isShift) ) {
view.tryPerform(\evaluateJavaScript,"selectLine()");
};
};

window.view.keyDownAction = { arg view, char, mods, uni, kcode, key;
var keyPlus, keyZero, keyMinus, keyEquals, keyF;
var modifier, zoomIn;
Expand Down
3 changes: 3 additions & 0 deletions editors/sc-ide/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,11 +193,14 @@ set(ide_webengine_moc_hdrs
widgets/util/WebSocketClientWrapper.hpp
widgets/util/IDEWebChannelWrapper.hpp
${CMAKE_SOURCE_DIR}/QtCollider/widgets/web_page.hpp
${CMAKE_SOURCE_DIR}/QtCollider/widgets/QcWebView.h
${CMAKE_SOURCE_DIR}/QtCollider/QcCallback.hpp
)
set(ide_webengine_src
widgets/help_browser.cpp
widgets/util/WebSocketTransport.cpp
${CMAKE_SOURCE_DIR}/QtCollider/widgets/web_page.cpp
${CMAKE_SOURCE_DIR}/QtCollider/widgets/QcWebView.cpp
mossheim marked this conversation as resolved.
Show resolved Hide resolved
)

if(SC_USE_QTWEBENGINE)
Expand Down
Loading