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

Web view enhancements #1071

Closed
wants to merge 23 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@pieper
Copy link
Member

pieper commented Jan 6, 2019

This extends the qSlicerWebView to better integrate with the Slicer ecosystem.

  • Allows C++ or Python code to execute JavaScript and get results back

  • Allows JavaScript code to call Python if user approves

  • Allows downloads to either save to disk or load directly to Slicer via Add Data dialog

  • Adds a download progress and cancel dialog for web downloads

  • Adds optional loading of zipfile contents to Add Data dialog

  • Adds a SelfTest for WebEngine related code

pieper added some commits Dec 29, 2018

ENH: enable web console in developer mode
QtWebEngine ships with the developer console
from Chrome, but only exposes it if an environment
variable is set.

This commit sets that variable if Slicer is in
developer mode.  With this, you can visit

http://localhost:1337

to access any WebEngineView (e.g. qSlicerWebWidget) in
the application, such as the ExtensionManager view.
ENH: enable web console in developer mode
QtWebEngine ships with the developer console
from Chrome, but only exposes it if an environment
variable is set.

This commit sets that variable if Slicer is in
developer mode.  With this, you can visit

http://localhost:1337

to access any WebEngineView (e.g. qSlicerWebWidget) in
the application, such as the ExtensionManager view.
ENH: web widget signal with evalJS result
Since javascript is evaluated asynchronously,
add a signal that can be used to listen for the
result.

This allows python/c++ in Slicer to communicate
with the javascript environment.
Merge branch 'webView-enhancements' of github.com:pieper/Slicer into …
…webView-enhancements

# Conflicts:
#	Applications/SlicerApp/Testing/Python/WebEngine.py
ENH: enable web console in developer mode
QtWebEngine ships with the developer console
from Chrome, but only exposes it if an environment
variable is set.

This commit sets that variable if Slicer is in
developer mode.  With this, you can visit

http://localhost:1337

to access any WebEngineView (e.g. qSlicerWebWidget) in
the application, such as the ExtensionManager view.
ENH: web widget signal with evalJS result
Since javascript is evaluated asynchronously,
add a signal that can be used to listen for the
result.

This allows python/c++ in Slicer to communicate
with the javascript environment.
ENH: improve qSlicerDataDialog
* Adds exernally callable addFile and addDirectory methods

Previously files and directories could only be added through
the GUI buttons, but with this change it's possible for code
to pre-populate the dialog, for example when data has been
downloaded and we want to offer it to the user.

* Adds auto-unzip option

If a file ending with .zip is added to the dialog, the user
is given the option to unzip the archive and load the files
inside.
ENH: add download behavior to qSlicerWebWidget
The change allows users to easily load data from websites directly
into Slicer.

* Default behavior now offers download or data load

The user can now save files to a selected location or can have
slicer save to a temp location and then load the downloaded files into
the Add Data dialog.

This is implemented in the default virtual method so that subclasses,
like the extension manager can still provide customized management
of downloads.

* Progress dialog and cancel option for downloads

A new qSlicerWebDownloadWidget shows progress and gives the
option of cancelling the download.
ENH: improve WebEngine SelfTest
* Self test now communicates to/from web javascript
environment and confirms that the correct data
is sent and received.

* Demo buttons are added to access various web pages
that may be interesting to use with Slicer.
Merge branch 'webView-enhancements' of github.com:pieper/Slicer into …
…webView-enhancements

# Conflicts:
#	Applications/SlicerApp/Testing/Python/WebEngine.py
ENH: add a proxy for calling python code from javascript
Javascript code running in a qSlicerWebWidget now has the option
to create a WebChannel and access a slicerPython variable and
invoke any code in the slicer python environment.

Because this could be a potential security issue, the user is
required to explicitly allow or deny access (and has the option
to always allow).

TODO: return values and exceptions are not currently passed back
to javascript.

TODO: allow the user to allow python for this session only without
allowing it forever.
ENH: track that user has approved python from webengine
With this, user is prompted once per session whether
web pages should be able to communicate with Slicer
via python.

Since a ctkMessageBox is used, the prefernce can be saved
in the settings file to always allow python.
ENH: add test code for calling python from javascript
This captures the methodology for calling python
from within a page running in a qSlicerWebView,
which works if invoked via the web concole of a
page, but does not work within a selftest.

The code is stubbed out but left as examples for
use by web page developers.

@pieper pieper requested review from jcfr and lassoan Jan 6, 2019

@lassoan
Copy link
Contributor

lassoan left a comment

Thank you for working on this, these features will be very useful. I've added a few minor comments inline.


def onEvalResult(self, js, result):
if js == "valueFromSlicer;":
if result != "42":

This comment has been minimized.

@lassoan

lassoan Jan 8, 2019

Contributor

:)

//-----------------------------------------------------------------------------
bool qSlicerDataDialogPrivate::checkAndHandleArchive(const QFileInfo& file)
{
if (file.suffix() == "zip")

This comment has been minimized.

@lassoan

lassoan Jan 8, 2019

Contributor

compare lowercase of suffix to "zip" to make "ZIP" work, too

// C function in vtkArchive
if (unzip(file.absoluteFilePath().toStdString().c_str(), this->temporaryArchiveDirectory->path().toStdString().c_str()))
{
this->addDirectory(QDir(this->temporaryArchiveDirectory->path()));

This comment has been minimized.

@lassoan

lassoan Jan 8, 2019

Contributor

If we save the scene, is default save location will be somewhere in the temp folder? It would be nice to reset the path of nodes that are loaded from the temp folder so that the files are saved in the .mrml file directory by default.

This comment has been minimized.

@pieper

pieper Jan 8, 2019

Author Member

Yes, that's true. Unfortunately all of the loading is done in the coreIOManager and by that time there's no way to tell it that these are from a temp directory. The only workaround I can think of would be to iterate through the storage nodes after the load and change the ones that correspond to the unzipped data, but that seems ugly.

I don't think the current behavior is so bad, because by the time a user goes to save they will have to decide where they want to put the data anyway, and they have control.

This comment has been minimized.

@pieper

pieper Jan 8, 2019

Author Member

Let's let Murat test this out for his use cases and suggest what would be good behavior.


// need to use a modal dialog here because 'download' will be deleted
// if we don't return 'accept from this slot
QMessageBox *messageBox = new QMessageBox(this);

This comment has been minimized.

@lassoan

lassoan Jan 8, 2019

Contributor

I think with the current implementation messagebox will not be deleted. You need to call deleteLater() or set messageBox->setAttribute(Qt::WA_DeleteOnClose, true). Other popup dialogs in this class may have the same issue.

This comment has been minimized.

@jamesobutler

jamesobutler Jan 8, 2019

Below is the specific note in the code about this. Also background discussion can be found in #951.

# Windows 10 peek feature in taskbar shows all hidden but not destroyed windows
# (after creating and closing a messagebox, hovering over the mouse on Slicer icon, moving up the
# mouse to the peek thumbnail would show it again).
# Popup windows in other Qt applications often show closed popups (such as
# Paraview's Edit / Find data dialog, MeshMixer's File/Preferences dialog).
# By calling deleteLater, the messagebox is permanently deleted when the current call is completed.
mbox.deleteLater()

This comment has been minimized.

@pieper

pieper Jan 8, 2019

Author Member

It seems to me the WA_DeleteOnClose attribute idea is the right one here, since this dialog shows progress/cancel information that exists through the event loop, while deleteLater says it deletes once the event loop is entered.

This comment has been minimized.

@pieper

pieper Jan 8, 2019

Author Member

Actually WA_DeleteOnClose wasn't right because this confirmation dialog is used later.

But I think it's actually deleted because I set 'this' to be it's parent, so that should close it up when the download dialog is deleted.

In the util.py case the dialog has a default parent of None (or maybe people set the parent to be mainWindow) so it would stay around.

This comment has been minimized.

@lassoan

lassoan Jan 8, 2019

Contributor

Yes, if the parent gets deleted (make sure it does, if it's a dialog then it might not) then it should work well without delete-on-close.

This comment has been minimized.

@pieper

pieper Jan 8, 2019

Author Member

Good point - the parent is the download progress widget, which is a child of the webwidget so it would have been deleted eventually, but I guess it's best to delete it when the download is complete to avoid the lingering window issue. I've updated to that and it seems to work.

/// the passed code, and the resulting QVariant is returned
/// as a string.
///
/// As a precaution, the user is prompted to allow or disallow

This comment has been minimized.

@lassoan

lassoan Jan 8, 2019

Contributor

Is this asked every time? Should there be an "always allow" option? Or always allow for a specific module?

This comment has been minimized.

@pieper

pieper Jan 8, 2019

Author Member

It's only asked once per session, and it's a ctkMessageBox, so it can be disabled persistently if you want. I think it's important to warn people though, since it's a pretty unusual feature and essentially gives the web page full access to your computer.

Show resolved Hide resolved Base/QTGUI/qSlicerWebWidget.cxx
@jcfr
Copy link
Member

jcfr left a comment

well done @pieper 👍 Few nitpicks and this should be good

def onEvalResult(self, js, result):
if js == "valueFromSlicer;":
if result != "42":
Exception("Did not get back expected result!")

This comment has been minimized.

@jcfr

jcfr Jan 8, 2019

Member

In python, it is usually better to return more specific exception. What about using RuntimeException or do assert result == "42"

@@ -52,6 +52,10 @@ class Q_SLICER_BASE_QTGUI_EXPORT qSlicerDataDialog : public qSlicerFileDialog
virtual bool exec(const qSlicerIO::IOProperties& readerProperties =
qSlicerIO::IOProperties());

/// for programmatic population of dialog

This comment has been minimized.

@jcfr

jcfr Jan 8, 2019

Member

These could be slots

@@ -70,6 +71,12 @@ protected slots:
/// Return true if the 2 items have the same filetype options.
/// I.e. same items int the TypeColumn combobox.
bool haveSameTypeOption(int row1, int row2)const;
/// check if file is an archive, and if so, give the user

This comment has been minimized.

@jcfr

jcfr Jan 8, 2019

Member

check -> Check

});

// Cancel
connect(cancelButton, &QPushButton::clicked, [=]() {

This comment has been minimized.

@jcfr

jcfr Jan 8, 2019

Member

Waiting 4.10.1 is cut, let's avoid c++11 lambdas

This comment has been minimized.

@pieper

pieper Jan 8, 2019

Author Member

Given the timing, I'd rather ifdef this functionality out of 4.10.1 rather than implementing the older way of doing things. The lambdas are just so much nicer...

// Qt includes
#include <QDialog>
class QWebEngineDownloadItem;

This comment has been minimized.

@jcfr

jcfr Jan 8, 2019

Member

extra line

Show resolved Hide resolved Base/QTGUI/qSlicerWebPythonProxy.cxx Outdated
Show resolved Hide resolved Base/QTGUI/qSlicerWebPythonProxy.cxx Outdated
Show resolved Hide resolved Base/QTGUI/qSlicerWebWidget.cxx

// This code uses C++11 lambdas, which are only available when
// Qt5 is used. This code will never actually be called unless
// QWebEngine is used, to it doesn't need to do anything in the

This comment has been minimized.

@jamesobutler

jamesobutler Jan 8, 2019

SUPER minor: Only pointing this out because I happened to read this and saw this typo (to --> so):

Suggested change Beta
// QWebEngine is used, to it doesn't need to do anything in the
// QWebEngine is used, so it doesn't need to do anything in the

pieper added some commits Jan 8, 2019

BUG: fix webwengine selftest logic
Now uses more specific exceptions and checks
return values more closely.
ENH: fixes to qSlicerDataDialog
* now handles .zip or .Zip or .ZIP the same as archives

* exec is invokable from python

* addDirectory and addFile are slots
BUG: fix webengine test so it passes
Was testing the failure case and forgot to set
the test back into passing state ; )
ENH: delete the download progress dialog when download complete
Avoid possible lingering window issues as discussed here:

#951
@pieper

This comment has been minimized.

Copy link
Member Author

pieper commented Jan 12, 2019

Committed in Revision 27702. Thanks for the reviews 👍

@pieper pieper closed this Jan 12, 2019

@pieper pieper deleted the pieper:webView-enhancements branch Jan 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment