Grid isolate scope (#10074) (#10133)

* Dashboard clean up

- Stop storing the grid element on the panel which caused a circular
reference and required special code to make it serializable.
- Call state.save directly rather than broadcasting change:vis

This is in prep for possibly making grid scope isolate and view/edit
mode implementation.

* Make dashboard grid scope isolate

This will help prepare for state changes for view/edit mode in
dashboard.js

* Remove unnecessary uses of findPanelByPanelIndex

can just use the panelElement directly

* Only need the panelIndex stored on the inner element

* Remove memory leak

Clean up panelElementMapping

src/core_plugins/kibana/public/dashboard/__tests__/dashboard_panels.js

@@ -19,7 +19,18 @@ describe('dashboard panels', function () {
 
       $el = angular.element(`
         <dashboard-app>
-          <dashboard-grid style="width: 600px; height: 600px;"></dashboard-grid>
+          <dashboard-grid
+            style="width: 600px; height: 600px;"
+            ng-if="!hasExpandedPanel()"
+            on-panel-removed="onPanelRemoved"
+            panels="state.panels"
+            get-vis-click-handler="filterBarClickHandler(state)"
+            get-vis-brush-handler="brushEvent(state)"
+            save-state="saveState"
+            toggle-expand="toggleExpandPanel"
+            create-child-ui-state="createChildUiState"
+            toggle-expand="toggleExpandPanel"
+           ></dashboard-grid>
         </dashboard-app>`);
       $compile($el)($scope);
       $scope.$digest();

src/core_plugins/kibana/public/dashboard/dashboard.html

@@ -56,7 +56,17 @@
     <p>Click the <a class="btn btn-xs navbtn-inverse" ng-click="kbnTopNav.open('add'); toggleAddVisualization = !toggleAddVisualization" aria-label="Add visualization">Add</a> button in the menu bar above to add a visualization to the dashboard. <br/>If you haven't setup a visualization yet visit <a href="#/visualize" title="Visualize">"Visualize"</a> to create your first visualization.</p>
   </div>
 
-  <dashboard-grid ng-if="!hasExpandedPanel()"></dashboard-grid>
+  <dashboard-grid
+    ng-if="!hasExpandedPanel()"
+    on-panel-removed="onPanelRemoved"
+    panels="state.panels"
+    get-vis-click-handler="filterBarClickHandler(state)"
+    get-vis-brush-handler="brushEvent(state)"
+    save-state="saveState"
+    toggle-expand="toggleExpandPanel"
+    create-child-ui-state="createChildUiState"
+    toggle-expand="toggleExpandPanel"
+  ></dashboard-grid>
 
   <dashboard-panel ng-if="hasExpandedPanel()"
                    panel="expandedPanel"
@@ -64,7 +74,7 @@
                    is-expanded="true"
                    get-vis-click-handler="filterBarClickHandler(state)"
                    get-vis-brush-handler="brushEvent(state)"
-                   save-state="state.save()"
-                   create-child-ui-state="createChildUiState(path, uiState)"
+                   save-state="saveState"
+                   create-child-ui-state="createChildUiState"
                    toggle-expand="toggleExpandPanel(expandedPanel.panelIndex)">
 </dashboard-app>

src/core_plugins/kibana/public/dashboard/dashboard.js

@@ -1,8 +1,8 @@
 import _ from 'lodash';
 import angular from 'angular';
-import chrome from 'ui/chrome';
 import uiModules from 'ui/modules';
 import uiRoutes from 'ui/routes';
+import chrome from 'ui/chrome';
 
 import 'plugins/kibana/dashboard/grid';
 import 'plugins/kibana/dashboard/panel/panel';
@@ -17,6 +17,7 @@ import { DashboardConstants } from './dashboard_constants';
 import UtilsBrushEventProvider from 'ui/utils/brush_event';
 import FilterBarFilterBarClickHandlerProvider from 'ui/filter_bar/filter_bar_click_handler';
 import { FilterUtils } from './filter_utils';
+import { getPersistedStateId } from 'plugins/kibana/dashboard/panel/panel_state';
 
 const app = uiModules.get('app/dashboard', [
   'elasticsearch',
@@ -182,6 +183,22 @@ app.directive('dashboardApp', function (Notifier, courier, AppState, timefilter,
         return $scope.uiState.createChild(path, uiState, true);
       };
 
+      $scope.saveState = function saveState() {
+        $state.save();
+      };
+
+      $scope.onPanelRemoved = (panelIndex) => {
+        _.remove($scope.state.panels, function (panel) {
+          if (panel.panelIndex === panelIndex) {
+            $scope.uiState.removeChild(getPersistedStateId(panel));
+            return true;
+          } else {
+            return false;
+          }
+        });
+        $state.save();
+      };
+
       $scope.brushEvent = brushEvent;
       $scope.filterBarClickHandler = filterBarClickHandler;
       $scope.expandedPanel = null;
@@ -255,12 +272,6 @@ app.directive('dashboardApp', function (Notifier, courier, AppState, timefilter,
         }
       });
 
-      // listen for notifications from the grid component that changes have
-      // been made, rather than watching the panels deeply
-      $scope.$on('change:vis', function () {
-        $state.save();
-      });
-
       // called by the saved-object-finder when a user clicks a vis
       $scope.addVis = function (hit) {
         pendingVis++;

src/core_plugins/kibana/public/dashboard/grid.js

@@ -1,6 +1,7 @@
 import _ from 'lodash';
 import $ from 'jquery';
 import Binder from 'ui/binder';
+import chrome from 'ui/chrome';
 import 'gridster';
 import uiModules from 'ui/modules';
 import { PanelUtils } from 'plugins/kibana/dashboard/panel/panel_utils';
@@ -11,7 +12,43 @@ const app = uiModules.get('app/dashboard');
 app.directive('dashboardGrid', function ($compile, Notifier) {
   return {
     restrict: 'E',
-    require: '^dashboardApp', // must inherit from the dashboardApp
+    scope: {
+      /**
+       * Used to create a child persisted state for the panel from parent state.
+       * @type {function} - Returns a {PersistedState} child uiState for this scope.
+       */
+      createChildUiState: '=',
+      /**
+       * Trigger after a panel has been removed from the grid.
+       */
+      onPanelRemoved: '=',
+      /**
+       * Contains information about this panel.
+       * @type {Array<PanelState>}
+       */
+      panels: '=',
+      /**
+       * Returns a click handler for a visualization.
+       * @type {function}
+       */
+      getVisClickHandler: '&',
+      /**
+       * Returns a brush event handler for a visualization.
+       * @type {function}
+       */
+      getVisBrushHandler: '&',
+      /**
+       * Call when changes should be propagated to the url and thus saved in state.
+       * @type {function}
+       */
+      saveState: '=',
+      /**
+       * Expand or collapse a panel, so it either takes up the whole screen or goes back to its
+       * natural size.
+       * @type {function}
+       */
+      toggleExpand: '=',
+    },
     link: function ($scope, $el) {
       const notify = new Notifier();
       const $container = $el;
@@ -20,9 +57,6 @@ app.directive('dashboardGrid', function ($compile, Notifier) {
       const $window = $(window);
       const binder = new Binder($scope);
 
-      // appState from controller
-      const $state = $scope.state;
-
       let gridster; // defined in init()
 
       // number of columns to render
@@ -34,25 +68,27 @@ app.directive('dashboardGrid', function ($compile, Notifier) {
 
       // debounced layout function is safe to call as much as possible
       const safeLayout = _.debounce(layout, 200);
-
-      $scope.removePanelFromState = (panelIndex) => {
-        _.remove($scope.state.panels, function (panel) {
-          return panel.panelIndex === panelIndex;
-        });
-      };
-
       /**
-       * Removes the panel with the given index from the $scope.state.panels array. Does not
-       * remove the ui element from gridster - that is triggered by a watcher that is
-       * triggered on changes made to $scope.state.panels.
-       * @param panelIndex {number}
+       * Mapping of panelIndex to the angular element in the grid.
        */
-      $scope.getPanelByPanelIndex = (panelIndex) => {
-        return _.find($scope.state.panels, function (panel) {
-          return panel.panelIndex === panelIndex;
-        });
+      const panelElementMapping = {};
+
+      // Tell gridster to remove the panel, and cleanup our metadata
+      function removePanelFromGrid(panelIndex, silent) {
+        const panelElement = panelElementMapping[panelIndex];
+        // remove from grister 'silently' (don't reorganize after)
+        gridster.remove_widget(panelElement, silent);
+        delete panelElementMapping[panelIndex];
+      }
+
+      $scope.removePanel = (panelIndex) => {
+        removePanelFromGrid(panelIndex);
+        $scope.onPanelRemoved(panelIndex);
       };
 
+      $scope.findPanelByPanelIndex = PanelUtils.findPanelByPanelIndex;
+      $scope.isFullScreenMode = !chrome.getVisible();
+
       function init() {
         $el.addClass('gridster');
 
@@ -79,18 +115,13 @@ app.directive('dashboardGrid', function ($compile, Notifier) {
           gridster.enable().enable_resize();
         });
 
-        $scope.$watchCollection('state.panels', function (panels) {
-          const currentPanels = gridster.$widgets.toArray().map(function (el) {
-            return getPanelFor(el);
-          });
-
-          // panels that are now missing from the panels array
-          const removed = _.difference(currentPanels, panels);
+        $scope.$watchCollection('panels', function (panels) {
+          const currentPanels = gridster.$widgets.toArray().map(
+            el => PanelUtils.findPanelByPanelIndex(el.panelIndex, $scope.panels)
+          );
 
           // panels that have been added
           const added = _.difference(panels, currentPanels);
-
-          if (removed.length) removed.forEach(removePanel);
           if (added.length) {
             // See issue https://github.com/elastic/kibana/issues/2138 and the
             // subsequent fix for why we need to sort here. Short story is that
@@ -105,16 +136,12 @@ app.directive('dashboardGrid', function ($compile, Notifier) {
                 return a.row - b.row;
               }
             });
-
             added.forEach(addPanel);
           };
 
-          // ensure that every panel can be serialized now that we are done
-          $state.panels.forEach(PanelUtils.makeSerializeable);
-
-          // alert interested parties that we have finished processing changes to the panels
-          // TODO: change this from event based to calling a method on dashboardApp
-          if (added.length || removed.length) $scope.$root.$broadcast('change:vis');
+          if (added.length) {
+            $scope.saveState();
+          }
         });
 
         $scope.$on('$destroy', function () {
@@ -122,13 +149,11 @@ app.directive('dashboardGrid', function ($compile, Notifier) {
           $window.off('resize', safeLayout);
 
           if (!gridster) return;
-          gridster.$widgets.each(function (i, el) {
-            const panel = getPanelFor(el);
+          gridster.$widgets.each(function (i, widget) {
+            const panelElement = panelElementMapping[widget.panelIndex];
             // stop any animations
-            panel.$el.stop();
-            removePanel(panel, true);
-            // not that we will, but lets be safe
-            PanelUtils.makeSerializeable(panel);
+            panelElement.stop();
+            removePanelFromGrid(widget.panelIndex, true);
           });
         });
 
@@ -138,65 +163,46 @@ app.directive('dashboardGrid', function ($compile, Notifier) {
         $scope.$on('globalNav:update', safeLayout);
       }
 
-      // return the panel object for an element.
-      //
-      // !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
-      // ALWAYS CALL PanelUtils.makeSerializeable AFTER YOU ARE DONE WITH IT
-      // !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
-      function getPanelFor(el) {
-        const $panel = el.jquery ? el : $(el);
-        const panel = $panel.data('panel');
-        panel.$el = $panel;
-        return panel;
-      }
-
-      // Tell gridster to remove the panel, and cleanup our metadata
-      function removePanel(panel, silent) {
-        // remove from grister 'silently' (don't reorganize after)
-        gridster.remove_widget(panel.$el, silent);
-        panel.$el.removeData('panel');
-        $scope.uiState.removeChild(getPersistedStateId(panel));
-      }
-
       // tell gridster to add the panel, and create additional meatadata like $scope
       function addPanel(panel) {
         PanelUtils.initializeDefaults(panel);
-
         const panelHtml = `
             <li>
                 <dashboard-panel
-                  remove="removePanelFromState(${panel.panelIndex})"
-                  panel="getPanelByPanelIndex(${panel.panelIndex})"
-                  is-full-screen-mode="!chrome.getVisible()"
+                  remove="removePanel(${panel.panelIndex})"
+                  panel="findPanelByPanelIndex(${panel.panelIndex}, panels)"
+                  is-full-screen-mode="isFullScreenMode"
                   is-expanded="false"
-                  get-vis-click-handler="filterBarClickHandler(state)"
-                  get-vis-brush-handler="brushEvent(state)"
-                  save-state="state.save()"
-                  toggle-expand="toggleExpandPanel(${panel.panelIndex})"
-                  create-child-ui-state="createChildUiState(path, uiState)">
+                  get-vis-click-handler="getVisClickHandler(state)"
+                  get-vis-brush-handler="getVisBrushHandler(state)"
+                  save-state="saveState"
+                  toggle-expand="toggleExpand(${panel.panelIndex})"
+                  create-child-ui-state="createChildUiState">
             </li>`;
-        panel.$el = $compile(panelHtml)($scope);
+        const panelElement = $compile(panelHtml)($scope);
+        panelElementMapping[panel.panelIndex] = panelElement;
+        // Store the panelIndex on the widget so it can be used to retrieve the panelElement
+        // from the mapping.
+        panelElement[0].panelIndex = panel.panelIndex;
 
         // tell gridster to use the widget
-        gridster.add_widget(panel.$el, panel.size_x, panel.size_y, panel.col, panel.row);
+        gridster.add_widget(panelElement, panel.size_x, panel.size_y, panel.col, panel.row);
 
         // Gridster may change the position of the widget when adding it, make sure the panel
         // contains the latest info.
-        PanelUtils.refreshSizeAndPosition(panel);
-
-        // stash the panel in the element's data
-        panel.$el.data('panel', panel);
+        PanelUtils.refreshSizeAndPosition(panel, panelElement);
       }
 
-      // when gridster tell us it made a change, update each of the panel objects
-      function readGridsterChangeHandler(e, ui, $widget) {
+      // When gridster tell us it made a change, update each of the panel objects
+      function readGridsterChangeHandler() {
         // ensure that our panel objects keep their size in sync
-        gridster.$widgets.each(function (i, el) {
-          const panel = getPanelFor(el);
-          PanelUtils.refreshSizeAndPosition(panel);
-          PanelUtils.makeSerializeable(panel);
-          $scope.$root.$broadcast('change:vis');
+        gridster.$widgets.each(function (i, widget) {
+          const panel = PanelUtils.findPanelByPanelIndex(widget.panelIndex, $scope.panels);
+          const panelElement = panelElementMapping[panel.panelIndex];
+          PanelUtils.refreshSizeAndPosition(panel, panelElement);
         });
+
+        $scope.saveState();
       }
 
       // calculate the position and sizing of the gridster el, and the columns within it

src/core_plugins/kibana/public/dashboard/panel/panel.js

@@ -34,7 +34,7 @@ uiModules
        * Used to create a child persisted state for the panel from parent state.
        * @type {function} - Returns a {PersistedState} child uiState for this scope.
        */
-      createChildUiState: '&',
+      createChildUiState: '=',
       /**
        * Contains information about this panel.
        * @type {PanelState}
@@ -69,7 +69,7 @@ uiModules
        * Call when changes should be propagated to the url and thus saved in state.
        * @type {function}
        */
-      saveState: '&'
+      saveState: '='
     },
     link: function ($scope, element) {
       if (!$scope.panel.id || !$scope.panel.type) return;
@@ -89,7 +89,7 @@ uiModules
 
         // create child ui state from the savedObj
         const uiState = $scope.savedObj.uiStateJSON ? JSON.parse($scope.savedObj.uiStateJSON) : {};
-        $scope.uiState = $scope.createChildUiState({ path : getPersistedStateId($scope.panel), uiState });
+        $scope.uiState = $scope.createChildUiState(getPersistedStateId($scope.panel), uiState);
 
         if ($scope.panel.type === savedVisualizations.type && $scope.savedObj.vis) {
           $scope.savedObj.vis.setUiState($scope.uiState);

src/core_plugins/kibana/public/dashboard/panel/panel_utils.js

@@ -1,4 +1,5 @@
 import { DEFAULT_PANEL_WIDTH, DEFAULT_PANEL_HEIGHT } from 'plugins/kibana/dashboard/panel/panel_state';
+import _ from 'lodash';
 
 export class PanelUtils {
   /**
@@ -24,9 +25,10 @@ export class PanelUtils {
   /**
    * Ensures that the panel object has the latest size/pos info.
    * @param {PanelState} panel
+   * @param {Element} panelElement - jQuery element representing the element in the UI
    */
-  static refreshSizeAndPosition(panel) {
-    const data = panel.$el.coords().grid;
+  static refreshSizeAndPosition(panel, panelElement) {
+    const data = panelElement.coords().grid;
     panel.size_x = data.size_x;
     panel.size_y = data.size_y;
     panel.col = data.col;
@@ -34,12 +36,12 @@ export class PanelUtils {
   }
 
   /**
-   * $el is a circular structure because it contains a reference to it's parent panel,
-   * so it needs to be removed before it can be serialized (we also don't
-   * want it to show up in the url).
-   * @param {PanelState} panel
+   * Returns the panel with the given panelIndex from the panels array (*NOT* the panel at the given index).
+   * @param panelIndex {number} - Note this is *NOT* the index of the panel in the panels array.
+   * panelIndex is really a panelId, but is called panelIndex for BWC reasons.
+   * @param panels {Array<Object>}
    */
-  static makeSerializeable(panel) {
-    delete panel.$el;
+  static findPanelByPanelIndex(panelIndex, panels) {
+    return _.find(panels, (panel) => panel.panelIndex === panelIndex);
   }
 }