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.

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

Clean up time range handling in embeddables #17718

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 2 additions & 2 deletions src/core_plugins/kibana/public/dashboard/dashboard_app.js
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ app.directive('dashboardApp', function ($injector) {
courier.fetch(...args);
};
$scope.timefilter = timefilter;
dashboardStateManager.handleTimeChange($scope.timefilter);
dashboardStateManager.handleTimeChange($scope.timefilter.time);

$scope.expandedPanel = null;
$scope.dashboardViewMode = dashboardStateManager.getViewMode();
Expand Down Expand Up @@ -220,7 +220,7 @@ app.directive('dashboardApp', function ($injector) {
$scope.$watch('model.query', $scope.updateQueryAndFetch);

$scope.$listen(timefilter, 'fetch', () => {
dashboardStateManager.handleTimeChange($scope.timefilter);
dashboardStateManager.handleTimeChange($scope.timefilter.time);
// Currently discover relies on this logic to re-fetch. We need to refactor it to rely instead on the
// directly passed down time filter. Then we can get rid of this reliance on scope broadcasts.
$scope.refresh();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -112,10 +112,19 @@ export class DashboardStateManager {

/**
* Time is part of global state so we need to deal with it outside of _pushAppStateChangesToStore.
* @param {Object} newTimeFilter
* @param {String|Object} newTimeFilter.to -- either a string representing an absolute time in utc format,
* or a relative time (now-15m), or a moment object
* @param {String|Object} newTimeFilter.from - either a string representing an absolute or a relative time, or a
* moment object
* @param {String} newTimeFilter.mode
*/
handleTimeChange(newTimeFilter) {
store.dispatch(updateTimeRange(newTimeFilter));
const timeFilter = {
from: FilterUtils.convertTimeToUTCString(newTimeFilter.from),
to: FilterUtils.convertTimeToUTCString(newTimeFilter.to),
mode: newTimeFilter.mode,
};
store.dispatch(updateTimeRange(timeFilter));
}

/**
Expand Down
16 changes: 11 additions & 5 deletions src/core_plugins/kibana/public/dashboard/lib/filter_utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -54,12 +54,18 @@ export class FilterUtils {
}

/**
* Converts the time to a string, if it isn't already.
* Converts the time to a utc formatted string. If the time is not valid (e.g. it might be in a relative format like
* 'now-15m', then it just returns what it was passed).
* @param time {string|Moment}
* @returns {string}
* @returns {string} the time represented in utc format, or if the time range was not able to be parsed into a moment
* object, it returns the same object it was given.
*/
static convertTimeToString(time) {
return typeof time === 'string' ? time : moment(time).toString();
static convertTimeToUTCString(time) {
if (moment(time).isValid()) {
return moment(time).utc();
} else {
return time;
}
}

/**
Expand All @@ -71,7 +77,7 @@ export class FilterUtils {
* @returns {boolean}
*/
static areTimesEqual(timeA, timeB) {
return this.convertTimeToString(timeA) === this.convertTimeToString(timeB);
return this.convertTimeToUTCString(timeA) === this.convertTimeToUTCString(timeB);
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,10 @@ export function updateSavedDashboard(savedDashboard, appState, timeFilter, toJso
savedDashboard.optionsJSON = toJson(appState.options);

savedDashboard.timeFrom = savedDashboard.timeRestore ?
FilterUtils.convertTimeToString(timeFilter.time.from)
FilterUtils.convertTimeToUTCString(timeFilter.time.from)
: undefined;
savedDashboard.timeTo = savedDashboard.timeRestore ?
FilterUtils.convertTimeToString(timeFilter.time.to)
FilterUtils.convertTimeToUTCString(timeFilter.time.to)
: undefined;
const timeRestoreObj = _.pick(timeFilter.refreshInterval, ['display', 'pause', 'section', 'value']);
savedDashboard.refreshInterval = savedDashboard.timeRestore ? timeRestoreObj : undefined;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,7 @@ export class DashboardPanel extends React.Component {
if (this.mounted) {
this.embeddable = embeddable;
embeddableIsInitialized(embeddable.metadata);
this.embeddable.onContainerStateChanged(this.props.containerState);
this.embeddable.render(this.panelElement);
this.embeddable.render(this.panelElement, this.props.containerState);
} else {
embeddable.destroy();
}
Expand Down
24 changes: 16 additions & 8 deletions src/core_plugins/kibana/public/dashboard/selectors/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -128,9 +128,10 @@ export const getHidePanelTitles = dashboard => dashboard.view.hidePanelTitles;
* @return {string|undefined}
*/
export const getMaximizedPanelId = dashboard => dashboard.view.maximizedPanelId;

/**
* @param dashboard {DashboardState}
* @return {string|undefined}
* @return {{ from: {string}, to: {string}, mode: {string} }}
*/
export const getTimeRange = dashboard => dashboard.view.timeRange;

Expand Down Expand Up @@ -160,7 +161,8 @@ export const getDescription = dashboard => dashboard.metadata.description;
* This state object is specifically for communicating to embeddables and it's structure is not tied to
* the redux tree structure.
* @typedef {Object} ContainerState
* @property {Object} timeRange
* @property {String} timeRange.to - either an absolute time range in utc format or a relative one (e.g. now-15m)
* @property {String} timeRange.from - either an absolute time range in utc format or a relative one (e.g. now-15m)
* @property {Object} embeddableCustomization
* @property {boolean} hidePanelTitles
*/
Expand All @@ -171,12 +173,18 @@ export const getDescription = dashboard => dashboard.metadata.description;
* @param panelId {string}
* @return {ContainerState}
*/
export const getContainerState = (dashboard, panelId) => ({
timeRange: _.cloneDeep(getTimeRange(dashboard)),
embeddableCustomization: _.cloneDeep(getEmbeddableCustomization(dashboard, panelId) || {}),
hidePanelTitles: getHidePanelTitles(dashboard),
customTitle: getPanel(dashboard, panelId).title,
});
export const getContainerState = (dashboard, panelId) => {
const time = getTimeRange(dashboard);
return {
timeRange: {
to: time.to,
from: time.from,
},
embeddableCustomization: _.cloneDeep(getEmbeddableCustomization(dashboard, panelId) || {}),
hidePanelTitles: getHidePanelTitles(dashboard),
customTitle: getPanel(dashboard, panelId).title,
};
};

/**
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,13 @@ export class SearchEmbeddable extends Embeddable {
};
}

render(domNode) {
/**
*
* @param {Element} domNode
* @param {ContainerState} containerState
*/
render(domNode, containerState) {
this.onContainerStateChanged(containerState);
this.domNode = domNode;
this.initializeSearchScope();
this.searchInstance = this.$compile(searchTemplate)(this.searchScope);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,13 @@ export class VisualizeEmbeddable extends Embeddable {
}
}

render(domNode) {
/**
*
* @param {Element} domNode
* @param {ContainerState} containerState
*/
render(domNode, containerState) {
this.onContainerStateChanged(containerState);
this.domNode = domNode;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm a little concerned about the this.onContainerStateChanged(containerState); in the render. I know this isn't React, but in general, I think of render as a "Make the DOM look like this" kind of function. If we are doing onContainerStateChanged here, it seems like it might be easy to get into a loop where onContainerStateChanged makes a change, which then makes a render happen, which then calls onContainerStateChanged, etc. Just a thought!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think your concerns are legit but this code is going to change soon with some updates @timroes is making to the loader. I think in the end it will be something like this pseudocode:

onContainerStateChanged(containerState) {
 this.saveState(containerState);
 this.reloadVisualization(getStateForRefresh());
}

render(dom, containerState) {
 this.saveState(containerState);
 this.createVisualization(domNode, getStateForFullRender());
}

Would that alleviate your concerns? It would just refactor the actual storing of state into a separate function both things could call.

this.handler = this.loader.embedVisualizationWithSavedObject(
domNode,
Expand Down
3 changes: 2 additions & 1 deletion src/ui/public/embeddable/embeddable.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,9 @@ export class Embeddable {

/**
* @param {Element} domNode - the dom node to mount the rendered embeddable on
* @param {ContainerState} containerState
*/
render(/*domNode*/) {}
render(/*domNode, containerState*/) {}

destroy() {}
}