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

fixing refresh and auto refresh #12752

Merged
merged 5 commits into from
Jul 12, 2017
Merged

fixing refresh and auto refresh #12752

merged 5 commits into from
Jul 12, 2017

Conversation

ppisljar
Copy link
Member

@ppisljar ppisljar commented Jul 11, 2017

closes #12733
closes #12781

@ppisljar ppisljar added Feature:Visualizations Generic visualization features (in case no more specific feature label is available) review v6.0.0 labels Jul 11, 2017
Copy link
Contributor

@thomasneirynck thomasneirynck left a comment

Choose a reason for hiding this comment

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

Works well, except clicking the magnifying glass next to the query-bar does not refresh the data.

In hindsight, I'm not sure if the should refresh the data to begin with. It would mean we have two buttons (search and refresh) that do exactly the same.

I'm not sure if the search-button from the query-bar should govern the entire page, as it's not related to the time-picker or the filter-bar.

@ppisljar
Copy link
Member Author

i agree, that magnifying glass should only submit a query right ?

@nreese nreese requested review from nreese and removed request for nreese July 11, 2017 20:49
@nreese
Copy link
Contributor

nreese commented Jul 11, 2017

@ppisljar In the visualize directive, the stateMonitor is not getting properly cleaned up. It should be destroyed when the scope gets destroyed.

Also, I think maps_visualization could benefit from this PR. Before the vis refactor, zoomend used to manually trigger a courier.fetch. Now zoomend just updates the state which I assume is used to trigger a refetch. I think it would be clearer if zoomend just triggered reload.

This will help me with Filter geohash_grid aggregation to map view box with collar. I need to trigger a fetch when the map view box is outside of the map collar and having to do this via saving the state is a little more difficult

@ppisljar
Copy link
Member Author

@nreese visualization should never directly trigger a reload ... doing vis.emit('reload') does the same as doing the vis.updateState() ... it just notifies that it should do something ....
in case of updateState it will check if it needs to refetch data and such, in reload it will force refetch data.

@ppisljar
Copy link
Member Author

uiState now triggers normal reload path (calling scope.fetch()) ....
courier response handler already knows uiState does not affect it and will not call es (unless forceReload() is used), but this gives possibility to other request handlers (like tsvb) to handle this differently

@ppisljar
Copy link
Member Author

jenkins, test this

@thomasneirynck thomasneirynck self-requested a review July 12, 2017 13:49
$scope.fetch();
};
$scope.vis.on('reload', reload);
$scope.$on('courier:searchRefresh', reload);
Copy link
Contributor

Choose a reason for hiding this comment

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

pressing the magnifier on the dashboard doesn't work. Consider adding

  $scope.vis.on('reload', reload);
      $scope.$on('courier:searchRefresh', reload);
      $scope.$on('fetch', () => {
        console.log('fetching on command from dashboard.js');
        $scope.vis.forceReload();
      });

@thomasneirynck
Copy link
Contributor

apart from dashboard issue, LGTM.

@ppisljar ppisljar merged commit 574f5ca into elastic:master Jul 12, 2017
thomasneirynck added a commit to thomasneirynck/kibana that referenced this pull request Jul 12, 2017
This was introduced as part of elastic#12752, but needs more review to verify if this is right way forward.
thomasneirynck added a commit that referenced this pull request Jul 12, 2017
This was introduced as part of #12752, but needs more review to verify if this is right way forward.
thomasneirynck added a commit to thomasneirynck/kibana that referenced this pull request Jul 12, 2017
This reverts commit 574f5ca.

This needs more review.
thomasneirynck added a commit that referenced this pull request Jul 12, 2017
This reverts commit dee4f7d.

This needs some more review, as in-tandem with the refresh PR, #12752, it may introduce some unwanted side-effects.
@jimgoodwin
Copy link

@ppisljar Please add a Release Note: paragraph to the description that describes what changed please

@thomasneirynck
Copy link
Contributor

thomasneirynck commented Jul 25, 2017

Removing release_note:fix label. This fixed a regression introduced by the refactor in master, but should not affect any user of v5. So I think we can skip the release note here.

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-rc1 v6.0.0
Projects
None yet
4 participants