From ffbdc8c65fcaa93b854f53a73a8e1a8a859ddf1e Mon Sep 17 00:00:00 2001
From: Nicolas Vuaille <nicolas.vuaille@kitware.com>
Date: Wed, 27 May 2020 15:04:12 +0200
Subject: [PATCH] Global clean up

---
 CMakeLists.txt                                | 15 +++++++++----
 PVKernelPlugin/PVInterface.py                 | 21 +++++++++++++++----
 PVKernelPlugin/PVInterpreter.cxx              | 10 +++++----
 PVKernelPlugin/PVInterpreter.h                |  3 +++
 PVKernelPlugin/PVKernelServer.cxx             |  2 +-
 PVKernelPlugin/paraview.plugin                |  3 ++-
 PVKernelPlugin/pqJupyterKernelLauncher.cxx    |  3 ---
 PVKernelPlugin/pqJupyterKernelLauncher.h      |  2 +-
 PVKernelPlugin/pqJupyterPluginActions.cxx     |  4 ----
 PVKernelPlugin/pqJupyterPluginActions.h       |  3 +--
 .../pqJupyterPluginTraceReaction.cxx          |  5 +++--
 11 files changed, 45 insertions(+), 26 deletions(-)

diff --git a/CMakeLists.txt b/CMakeLists.txt
index dec4c66..e5b8074 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -29,13 +29,20 @@ paraview_plugin_build(
   LIBRARY_SUBDIRECTORY "${PARAVIEW_PLUGIN_SUBDIR}"
   PLUGINS ${plugins})
 
-set(PARAVIEW_EXECUTABLE  "${ParaView_DIR}/bin/paraview" CACHE PATH "Path to a ParaView program")
-
-set(ENV_EXECUTABLE  "/usr/bin/env" CACHE PATH "Path to the `env` program")
+# Find ParaView executable
+# This program is launched at kernel start.
+find_program(PARAVIEW_EXECUTABLE NAMES "paraview" PATHS "${ParaView_DIR}/bin" DOC "Path to ParaView")
+
+# Find env executable.
+# This is needed to pass the notebook config file to the kernel at runtime.
+# In the kernel.json file, the ${connexion_file} var is replaced at runtime with the path of the
+# actual connection file. But this replacement is done only in the `argv` list, not in the `env` one.
+# Moreover, first item in the `argv' list should be the executable.
+# See kernel.json.in
+find_program(ENV_EXECUTABLE NAMES "env" DOC "Path to the `env` program")
 
 set(KERNEL_INSTALL_PATH "~/.local/share/jupyter/kernels" CACHE PATH "Path where to install the kernel")
 set(PLUGIN_INSTALL_PATH "${CMAKE_INSTALL_PREFIX}/${CMAKE_INSTALL_LIBDIR}/${PARAVIEW_PLUGIN_SUBDIR}/${PROJECT_NAME}")
 
 configure_file(kernel.json.in ${CMAKE_BINARY_DIR}/kernel.json)
 install(FILES ${CMAKE_BINARY_DIR}/kernel.json DESTINATION ${KERNEL_INSTALL_PATH}/ParaView)
-
diff --git a/PVKernelPlugin/PVInterface.py b/PVKernelPlugin/PVInterface.py
index e4ffd90..b01c973 100644
--- a/PVKernelPlugin/PVInterface.py
+++ b/PVKernelPlugin/PVInterface.py
@@ -5,7 +5,6 @@ from paraview import vtk
 # *****************
 # Pipeline
 # *****************
-
 # -----------------------------------------------------------------------------
 def _CreatePipelineSource(grid, name='MyGrid', representation='Surface'):
     """ Create a data producer containg the input vtk grid as data.
@@ -115,7 +114,6 @@ def RemoveFromPipeline(source=None):
 # *****************
 # Data Array
 # *****************
-
 # -----------------------------------------------------------------------------
 def AddPointArray(data, name="MyDataArray", proxy=None):
     AddArray(data, name, proxy, "POINT")
@@ -151,7 +149,12 @@ def AddArray(data, name, proxy, association):
     # VTK_data has shape (nbPoints, nbOfComponents)
     VTK_data = numpy_support.numpy_to_vtk(num_array=array)
     VTK_data.SetName(name)
-    fieldata = grid.GetPointData() if association == "POINT" else grid.GetCellData() if association == "CELL" else grid.GetFieldData()
+    fieldata = grid.GetFieldData()
+    if association == "POINT":
+        fieldata = grid.GetPointData()
+    elif association == "CELL":
+        fieldata = grid.GetCellData()
+
     fieldata.AddArray(VTK_data)
     fieldata.SetActiveScalars(name)
     proxy.MarkModified(None)
@@ -185,7 +188,6 @@ def ColorByArray(arrayName, proxy=None):
 # *****************
 # Selection
 # *****************
-
 # -----------------------------------------------------------------------------
 def GetSelection(proxy=None, port=0):
     if not proxy:
@@ -212,3 +214,14 @@ def GetSelectedCells(proxy=None, port=0):
 def GetSelectedPoints(proxy=None, port=0):
     sel = GetSelection(proxy, port)
     return sel[1] if sel[0] == "POINT" else []
+
+
+# *****************
+# Display
+# *****************
+# -----------------------------------------------------------------------------
+def Display(w = 400, h = 300, all_views = False):
+    ''' Dummy declaration for autocompletion purpose. It is handled in cpp code.
+    see also PVInterpreter.cxx
+    '''
+    pass
diff --git a/PVKernelPlugin/PVInterpreter.cxx b/PVKernelPlugin/PVInterpreter.cxx
index 66d8af3..1bef5e2 100644
--- a/PVKernelPlugin/PVInterpreter.cxx
+++ b/PVKernelPlugin/PVInterpreter.cxx
@@ -110,14 +110,14 @@ void PVInterpreter::handle_events(
 //------------------------------------------------------------------------------
 void PVInterpreter::initialize_python()
 {
-  // TODO should we really do that ? It is done in the python shell ...
   m_interpreter->Push("from paraview.simple import *");
 }
 
 //------------------------------------------------------------------------------
 bool PVInterpreter::push_to_new_cell(const std::string& code)
 {
-  // https://jupyter-notebook.readthedocs.io/en/stable/extending/frontend_extensions.html
+  // js API can be found here:
+  // https://github.com/jupyter/notebook/blob/76a323e677b7080a1e9a88437d6b5cea6cc0403b/notebook/static/notebook/js/notebook.js
   std::string jsCode = "var c = Jupyter.notebook.insert_cell_below('code');\nc.set_text(\"";
   jsCode += code;
   jsCode += "\")";
@@ -144,9 +144,11 @@ nl::json PVInterpreter::execute_request_impl(int execution_counter, const std::s
   qscode.append("\n");
 
   /**
-   * Defines Display method:
-   *
+   * Defines Display method. It is equivalent to this python declaration:
+   * ```
    * def Display(w = 400, h = 300, all_views = False):
+   * ```
+   * We need to catch it here to call the `display_data` method.
    */
   QRegularExpression screenshotCommand("^Display\\(((\\d+),(\\d+)(,(\\w+))?)?\\)");
 
diff --git a/PVKernelPlugin/PVInterpreter.h b/PVKernelPlugin/PVInterpreter.h
index 3c37553..0e7afd3 100644
--- a/PVKernelPlugin/PVInterpreter.h
+++ b/PVKernelPlugin/PVInterpreter.h
@@ -29,6 +29,9 @@ public:
    */
   bool push_to_new_cell(const std::string& code);
 
+  /**
+   * Initialize the python interpreter by importing paraview.simple
+   */
   void initialize_python();
 
 private:
diff --git a/PVKernelPlugin/PVKernelServer.cxx b/PVKernelPlugin/PVKernelServer.cxx
index c800a40..61a09b7 100644
--- a/PVKernelPlugin/PVKernelServer.cxx
+++ b/PVKernelPlugin/PVKernelServer.cxx
@@ -18,7 +18,7 @@
 PVKernelServer::PVKernelServer(zmq::context_t& context, const xeus::xconfiguration& config)
   : xserver_zmq(context, config)
 {
-  // 50ms interval is sort enough so that users will not notice significant latency
+  // 50ms interval is short enough so that users will not notice significant latency
   // yet it is long enough to minimize CPU load caused by polling.
   m_pollTimer = new QTimer();
   m_pollTimer->setInterval(50);
diff --git a/PVKernelPlugin/paraview.plugin b/PVKernelPlugin/paraview.plugin
index edf2e0f..e62c821 100644
--- a/PVKernelPlugin/paraview.plugin
+++ b/PVKernelPlugin/paraview.plugin
@@ -1,7 +1,8 @@
 NAME
   ParaViewJupyter
 DESCRIPTION
-  A Jupyter Kernel for ParaView, autostarting.
+  A Jupyter Kernel to control ParaView. Intended to be autoloaded at startup when using Jupyter notebook.
+  Should not be manually loaded.
 CONDITION
   PARAVIEW_USE_QT
 REQUIRES_MODULES
diff --git a/PVKernelPlugin/pqJupyterKernelLauncher.cxx b/PVKernelPlugin/pqJupyterKernelLauncher.cxx
index 953b31d..f3f9d4b 100644
--- a/PVKernelPlugin/pqJupyterKernelLauncher.cxx
+++ b/PVKernelPlugin/pqJupyterKernelLauncher.cxx
@@ -79,9 +79,6 @@ pqJupyterKernelLauncher::pqJupyterKernelLauncher(QObject* p)
 {
 }
 
-//-----------------------------------------------------------------------------
-pqJupyterKernelLauncher::~pqJupyterKernelLauncher() {}
-
 //-----------------------------------------------------------------------------
 void pqJupyterKernelLauncher::onStartup()
 {
diff --git a/PVKernelPlugin/pqJupyterKernelLauncher.h b/PVKernelPlugin/pqJupyterKernelLauncher.h
index b30ee0d..ff5b05b 100644
--- a/PVKernelPlugin/pqJupyterKernelLauncher.h
+++ b/PVKernelPlugin/pqJupyterKernelLauncher.h
@@ -12,7 +12,7 @@ class pqJupyterKernelLauncher : public QObject
 
 public:
   pqJupyterKernelLauncher(QObject* p = nullptr);
-  ~pqJupyterKernelLauncher();
+  ~pqJupyterKernelLauncher() = default;
 
   // Callback for shutdown.
   void onShutdown();
diff --git a/PVKernelPlugin/pqJupyterPluginActions.cxx b/PVKernelPlugin/pqJupyterPluginActions.cxx
index f83ce57..2bb77b5 100644
--- a/PVKernelPlugin/pqJupyterPluginActions.cxx
+++ b/PVKernelPlugin/pqJupyterPluginActions.cxx
@@ -12,10 +12,6 @@ pqJupyterPluginActions::pqJupyterPluginActions(QObject* p)
 {
   QIcon icon = qApp->style()->standardIcon(QStyle::SP_DialogNoButton);
   QAction* a = this->addAction(new QAction(icon, "Record Trace", this));
-  QObject::connect(a, SIGNAL(triggered(bool)), this, SLOT(onAction()));
 
   new pqJupyterPluginTraceReaction(a);
 }
-
-//-----------------------------------------------------------------------------
-pqJupyterPluginActions::~pqJupyterPluginActions() {}
diff --git a/PVKernelPlugin/pqJupyterPluginActions.h b/PVKernelPlugin/pqJupyterPluginActions.h
index 5d51131..2658d93 100644
--- a/PVKernelPlugin/pqJupyterPluginActions.h
+++ b/PVKernelPlugin/pqJupyterPluginActions.h
@@ -8,8 +8,7 @@ class pqJupyterPluginActions : public QActionGroup
   Q_OBJECT
 public:
   pqJupyterPluginActions(QObject* p);
-  ~pqJupyterPluginActions();
-
+  ~pqJupyterPluginActions() = default;
 };
 
 #endif
diff --git a/PVKernelPlugin/pqJupyterPluginTraceReaction.cxx b/PVKernelPlugin/pqJupyterPluginTraceReaction.cxx
index fe3b157..cf7d519 100644
--- a/PVKernelPlugin/pqJupyterPluginTraceReaction.cxx
+++ b/PVKernelPlugin/pqJupyterPluginTraceReaction.cxx
@@ -19,7 +19,7 @@ public:
   QPointer<QAction> standardTraceAction;
   QString extraLines;
 
-  pqInternals() {}
+  pqInternals() = default;
 
   void findStandardTraceAction()
   {
@@ -57,6 +57,8 @@ pqJupyterPluginTraceReaction::pqJupyterPluginTraceReaction(QAction* p)
   this->updateAction();
 }
 
+// Cannot be '= default' because member QScopedPointer<pqInternals> uses forward declared pqInternals.
+// see https://doc.qt.io/qt-5/qscopedpointer.html#forward-declared-pointers
 //-----------------------------------------------------------------------------
 pqJupyterPluginTraceReaction::~pqJupyterPluginTraceReaction() {}
 
@@ -115,7 +117,6 @@ void pqJupyterPluginTraceReaction::onTriggered()
     QStringList traceLines = traceString.split("\n");
 
     // remove empty lines at begining of trace
-    // TODO : why are they here ?
     while (!traceLines.isEmpty() && traceLines.first().trimmed().isEmpty())
     {
       traceLines.removeFirst();
-- 
GitLab