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

Should respect pinned filters #13019

Merged

Conversation

thomasneirynck
Copy link
Contributor

Closes #13011.

Not sure if this is the best solution, but wanted something up that we can use to discuss around.

The problem is that Visualize is not refreshing when there are changes in the filters of the globalState. A pinned filter is a global filter, and not present in the appState, so when we pin the filter, it does not create a refresh, and neither are they applied..

the queryFilter object provides a listener on both fitlers in the app and global state, and provides access to both the filter and the query object.

It may be worth considering just to remove the usage of appState in the courier request handler and visualize-directirve to retrieve query/filters and use queryFilters for that purpose instead. But not sure if that is going to have any other impact.

@thomasneirynck thomasneirynck requested a review from ppisljar July 20, 2017 23:15
@thomasneirynck thomasneirynck added Feature:Visualizations Generic visualization features (in case no more specific feature label is available) review v6.0.0 v6.0.0-beta1 labels Jul 20, 2017
const responseHandler = getHandler(responseHandlers, $scope.vis.type.responseHandler);

$scope.fetch = _.debounce(function () {
.directive('visualize', function (Notifier, Private, timefilter, getAppState) {
Copy link
Member

Choose a reason for hiding this comment

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

seems github messed up this comparison ? shows whole file is different where only a couple of lines changed ....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure. maybe a lintroller thing..

const requestHandlers = Private(VisRequestHandlersRegistryProvider);
const responseHandlers = Private(VisResponseHandlersRegistryProvider);
const ResizeChecker = Private(ResizeCheckerProvider);
const queryFilter = Private(FilterBarQueryFilterProvider);
Copy link
Member

Choose a reason for hiding this comment

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

i am not really sure about using queryFilter here ...
what about using globalState service to retrieve the filters from there ?

Copy link
Member

Choose a reason for hiding this comment

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

queryFilter is more more specific, but much easier to use .... appState + globalState are more generic .... but we shouldn't probably use them directly if we already have abstractions on top of it.


// searchSource is only there for courier request handler
requestHandler($scope.vis, $scope.appState, $scope.uiState, $scope.savedObj.searchSource)
requestHandler($scope.vis, $scope.appState, $scope.uiState, $scope.savedObj.searchSource, queryFilter)
Copy link
Member

Choose a reason for hiding this comment

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

put the queryFilter before searchSource (searchSource is kind of BWC parameter which only courier request handler should use

also we should get rid of appState (query filter can retrieve the query and the filters right ?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure if we should. appState has that .linked flag we require. would keep this a spot fix for now.

if (keys[0] === 'uiState') {
$scope.$on('fetch', reload);
queryFilter.on('update', () => {
$scope.fetch();
Copy link
Member

Choose a reason for hiding this comment

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

no need to wrap this function ....
queryFilter.on('update', $scope.fetch);

Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

  • please change the requestHandler as mentioned above
  • other requestHandlers need to be updated as well (timelion, tsvb)

@thomasneirynck thomasneirynck force-pushed the fix/should_respect_pinned_filters branch from 86e3ba0 to a807147 Compare July 27, 2017 13:12
@thomasneirynck thomasneirynck requested a review from ppisljar July 27, 2017 14:38
Copy link
Member

@ppisljar ppisljar left a comment

Choose a reason for hiding this comment

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

LGTM,

but this removes some of the work we tried to do with refactor. When reusing <visualize> you now no longer have control over filters (<visualize> is tightly coupled to queryFilter)

@thomasneirynck thomasneirynck merged commit 5adc2cd into elastic:master Jul 28, 2017
@thomasneirynck
Copy link
Contributor Author

thomasneirynck commented Jul 28, 2017

Backports:
6.0: #13189
6.x: #13188

thomasneirynck added a commit to thomasneirynck/kibana that referenced this pull request Jul 28, 2017
thomasneirynck added a commit to thomasneirynck/kibana that referenced this pull request Jul 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Visualizations Generic visualization features (in case no more specific feature label is available) review v6.0.0-beta1 v6.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants