Update doc table when auto refresh contains doc updates #10385
Conversation
There was a problem hiding this comment.
While this has the effect of rendering the new document content, it does so by completely removing the corresponding DOM tree and rendering a new one. In the eyes of ngRepeat the new item has no connection to the old one. In a way this defeats the purpose of track by. Just imagine Kibana having animations for inserting and removing items in the table. An updated document would be animated as being removed and a new one being inserted.
If I read the angular code correctly, ngRepeat in combination with track by is supposed to keep the directive instance constant while updating the properties. The proper way to deal with this would then be to $watch the properties and re-render the row. Our weird rendering code is the reason that this doesn't work automatically, IMHO.
So I would consider this a quick band-aid that we should follow up with proper cleanup. I'd be fine with doing it this way since I plan to perform just that cleanup soon.
|
I totally agree @weltenwort. If we were using standard angular templates to render the rows we wouldn't have this bug, the view would naturally update via angular's binding when the underlying objects updated. However I'm guessing the lodash templates were used to solve performance issues. Watching every property on hundreds of arbitrarily complex objects could have unbounded performance costs. I'm excited to see what you come up with when you tackle the refactoring. Until then I'm happy with this band-aid if you are. |
There was a problem hiding this comment.
Hmm, one case this doesn't solve is highlighting. For example, if I search for something and expand the row, then search again for a different term and the same row remains (and the version is still the same), then the highlighting remains the same in the expanded document. Any ideas on how to address that? (P.S. the corresponding issue is #9729.)
|
@lukasolson I think we'd have to do something similar to what @weltenwort proposed but I haven't dug into it too deeply. I figure it's better to handle that separately since it's lower value, but more complicated. |
| @@ -144,7 +144,8 @@ function discoverController($scope, config, courier, $route, $window, Notifier, | |||
| $scope.indexPattern = resolveIndexPatternLoading(); | |||
| $scope.searchSource | |||
| .set('index', $scope.indexPattern) | |||
| .highlightAll(true); | |||
| .highlightAll(true) | |||
| .version(true); | |||
There was a problem hiding this comment.
Absolutely, good catch. I also realized I needed to update the track by above the first one I changed. Updated.
Bargs
force-pushed
the
autoRefreshDocChange
branch
from
128b4cc to
fe0aefc
Compare
|
@weltenwort @lukasolson any more thoughts on this? |
There was a problem hiding this comment.
So my only comment here is that, while it's not a huge deal, I would prefer it if the behavior were totally consistent. If I have a doc expanded, and then refresh the search, and the doc hasn't been updated, the doc will remain expanded. However, with this change, if I've updated the doc, the doc will collapse and I have to re-expand to see the details again. It's not a huge deal, and I don't think it blocks this PR, it's just my $0.02.
LGTM.
There was a problem hiding this comment.
Now that the context view has been merged, which also uses the doc table, a similar fix could be applied there by adding version: true to the query:
https://github.com/elastic/kibana/blob/master/src/core_plugins/kibana/public/context/api/utils/queries.js#L3
https://github.com/elastic/kibana/blob/master/src/core_plugins/kibana/public/context/api/utils/queries.js#L15
Thanks 😃
Bargs
force-pushed
the
autoRefreshDocChange
branch
from
fe0aefc to
2db1c09
Compare
|
@weltenwort updated! |
|
@weltenwort any more feedback here? |
Backports PR #10385 **Commit 1:** Adds document version to doc table's track_by * Original sha: 747b8ed * Authored by Matthew Bargar <[email protected]> on 2017-02-15T22:13:04Z **Commit 2:** Add version to dashboard search source as well * Original sha: 2cd0f31 * Authored by Matthew Bargar <[email protected]> on 2017-02-17T17:45:39Z **Commit 3:** Add version to context queries * Original sha: 2db1c09 * Authored by Matthew Bargar <[email protected]> on 2017-02-28T16:17:43Z
Backports PR #10385 **Commit 1:** Adds document version to doc table's track_by * Original sha: 747b8ed * Authored by Matthew Bargar <[email protected]> on 2017-02-15T22:13:04Z **Commit 2:** Add version to dashboard search source as well * Original sha: 2cd0f31 * Authored by Matthew Bargar <[email protected]> on 2017-02-17T17:45:39Z **Commit 3:** Add version to context queries * Original sha: 2db1c09 * Authored by Matthew Bargar <[email protected]> on 2017-02-28T16:17:43Z
Fixes #3430
Fixes #7505
The problem: if a doc is already displayed in the doc table and it gets updated in elasticsearch, that change won't be reflected in the doc table via auto-refresh because our ng-repeat
track byonly looks at index, type, and score.I've fixed this by including the
_versionin the search response and then adding it to thetrack by.