-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Add comments and inline docs for visualization saving and editing process. #7968
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -66,16 +66,27 @@ uiModules | |
location: 'Visualization Editor' | ||
}); | ||
|
||
// Retrieve the resolved SavedVis instance. | ||
const savedVis = $route.current.locals.savedVis; | ||
|
||
// Instance of vis.js. | ||
const vis = savedVis.vis; | ||
|
||
// Clone the _vis instance. | ||
const editableVis = vis.createEditableVis(); | ||
|
||
// NOTE: Why is this instance method being overwritten? | ||
vis.requesting = function () { | ||
const requesting = editableVis.requesting; | ||
// Invoking requesting() calls onRequest on each agg's type param. | ||
// NOTE: What is the structure of an agg_config, and the role of type | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @spalger Question There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The When a vis is marked as being requested, the bounds of that vis are updated and the corresponding data can be resolved based on new bounds. |
||
// and type.params? | ||
requesting.call(vis); | ||
requesting.call(editableVis); | ||
}; | ||
|
||
// Boolean by default, but then something else later | ||
// NOTE: What is the role of searchSource? Why does it default to a boolean? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @spalger Question There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is kind of a scattershot description, I'll probably come back and edit it to be more coherent later
Because filters/queries in Kibana have different levels of persistence and come from different places, it is important to keep track of where filters come from for when they are saved back to the At query time, all of the That set of query parameters is then sent to elasticsearch. This is how the filter hierarchy works in Kibana. Visualize, starting from a new search:
Visualize, starting from an existing search:
Dashboard search sources:
|
||
const searchSource = savedVis.searchSource; | ||
|
||
$scope.topNavMenu = [{ | ||
|
@@ -104,8 +115,12 @@ uiModules | |
docTitle.change(savedVis.title); | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This seemed like a mistake, so to clarify the role of this variable within the function I gave it a unique name and scoped it to the function. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, wow, confused why eslint didn't complain about that. |
||
// Instance of app_state.js. | ||
let $state = $scope.$state = (function initState() { | ||
// Extract visualization state with filtered aggs. | ||
// Consists of: aggs, params, listeners, title, and type. | ||
const savedVisState = vis.getState(); | ||
|
||
const stateDefaults = { | ||
uiState: savedVis.uiStateJSON ? JSON.parse(savedVis.uiStateJSON) : {}, | ||
linked: !!savedVis.savedSearchId, | ||
|
@@ -114,19 +129,22 @@ uiModules | |
vis: savedVisState | ||
}; | ||
|
||
$state = new AppState(stateDefaults); | ||
// This is used to generate a 'uiState' PersistedState instance, and | ||
// implictly add 'change' and 'fetch_with_changes' event handlers. | ||
const appState = new AppState(stateDefaults); | ||
|
||
if (!angular.equals($state.vis, savedVisState)) { | ||
// NOTE: Why would appState.vis not equal the savedVisState? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @spalger Question There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
if (!angular.equals(appState.vis, savedVisState)) { | ||
Promise.try(function () { | ||
editableVis.setState($state.vis); | ||
editableVis.setState(appState.vis); | ||
vis.setState(editableVis.getEnabledState()); | ||
}) | ||
.catch(courier.redirectWhenMissing({ | ||
'index-pattern-field': '/visualize' | ||
})); | ||
} | ||
|
||
return $state; | ||
return appState; | ||
}()); | ||
|
||
function init() { | ||
|
@@ -137,8 +155,14 @@ uiModules | |
$scope.indexPattern = vis.indexPattern; | ||
$scope.editableVis = editableVis; | ||
$scope.state = $state; | ||
|
||
// Create a PersistedState instance. | ||
$scope.uiState = $state.makeStateful('uiState'); | ||
// Associate PersistedState instance with the Vis instance, so that | ||
// `uiStateVal` can be called on it. Currently this is only used to extract | ||
// map-specific information (e.g. mapZoom, mapCenter). | ||
vis.setUiState($scope.uiState); | ||
|
||
$scope.timefilter = timefilter; | ||
$scope.opts = _.pick($scope, 'doSave', 'savedVis', 'shareData', 'timefilter'); | ||
|
||
|
@@ -239,6 +263,9 @@ uiModules | |
kbnUrl.change('/visualize', {}); | ||
}; | ||
|
||
/** | ||
* Called when the user clicks "Save" button. | ||
*/ | ||
$scope.doSave = function () { | ||
savedVis.id = savedVis.title; | ||
// vis.title was not bound and it's needed to reflect title into visState | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,14 @@ | ||
|
||
/** | ||
* @name Vis | ||
* | ||
* @description This class consists of aggs, params, listeners, title, and type. | ||
* - Aggs: Instances of AggConfig. | ||
* - Params: Are these the settings in the Options tab? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @spalger Question There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yep |
||
* | ||
* Not to be confused with vislib/vis.js. | ||
*/ | ||
|
||
import _ from 'lodash'; | ||
import AggTypesIndexProvider from 'ui/agg_types/index'; | ||
import RegistryVisTypesProvider from 'ui/registry/vis_types'; | ||
|
@@ -24,7 +35,6 @@ export default function VisFactory(Notifier, Private) { | |
|
||
this.indexPattern = indexPattern; | ||
|
||
// http://aphyr.com/data/posts/317/state.gif | ||
this.setState(state); | ||
this.setUiState(uiState); | ||
} | ||
|
@@ -36,6 +46,7 @@ export default function VisFactory(Notifier, Private) { | |
|
||
let schemas = type.schemas; | ||
|
||
// NOTE: What is this doing? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @spalger Question There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Basically, this was put in place to do migrations at runtime. I'm not certain when it went in, but I'm pretty sure we can remove it for 5.0 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it was around 4.0 that this went in, and it was only to support people who had saved visualizations during the 4.0 betas. I would be pretty surprised if someone expects visualizations created in the 4.0 betas to open fine in 5.0... but they might not realize how long it's been since they saved their visualizations, and we have done everything possible to avoid writing real migrations. |
||
let aggs = _.transform(oldState, function (newConfigs, oldConfigs, oldGroupName) { | ||
let schema = schemas.all.byName[oldGroupName]; | ||
|
||
|
@@ -119,6 +130,7 @@ export default function VisFactory(Notifier, Private) { | |
}; | ||
|
||
Vis.prototype.requesting = function () { | ||
// Invoke requesting() on each agg. Aggs is an instance of AggConfigs. | ||
_.invoke(this.aggs.getRequestAggs(), 'requesting'); | ||
}; | ||
|
||
|
@@ -149,6 +161,11 @@ export default function VisFactory(Notifier, Private) { | |
Vis.prototype.getUiState = function () { | ||
return this.__uiState; | ||
}; | ||
|
||
/** | ||
* Currently this is only used to extract map-specific information | ||
* (e.g. mapZoom, mapCenter). | ||
*/ | ||
Vis.prototype.uiStateVal = function (key, val) { | ||
if (this.hasUiState()) { | ||
if (_.isUndefined(val)) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@spalger Question
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
editableVis
is intended to be a mirror of the vis object, and as such it needs to go through the same lifecycle methods that vis does (in this case#requesting()
). I'm not certain what benefits from the requesting method being called on the editable vis though...