Skip to content

Watch for changes to row to ensure summary row is up-to-date#11625

Closed
trevan wants to merge 1 commit intoelastic:masterfrom
trevan:listen-in-table-row-for-changes
Closed

Watch for changes to row to ensure summary row is up-to-date#11625
trevan wants to merge 1 commit intoelastic:masterfrom
trevan:listen-in-table-row-for-changes

Conversation

@trevan
Copy link
Copy Markdown
Contributor

@trevan trevan commented May 5, 2017

I've built a docview plugin that allows my users to edit the row. I noticed that when I updated the hit, the table summary wouldn't change. After investigating, all that was needed was to add the row to the watch for createSummaryRow.

@elasticmachine
Copy link
Copy Markdown
Contributor

Can one of the admins verify this patch?

@Bargs
Copy link
Copy Markdown
Contributor

Bargs commented May 5, 2017

Hmmm can you provide a set of steps that reproduces the issue in core Kibana? We fixed a similar issue in the past #10385 so it's odd that your scenario wouldn't work out of the box.

@trevan
Copy link
Copy Markdown
Contributor Author

trevan commented May 6, 2017

This is to actually get around the issue that @lukasolson mentioned in #10385 (review). Updating the doc through a docview causes the docview to disappear if I have the courier refetch the data. So, instead, my docview is just updating the hit in memory (as well as saving it to ES) and this change allows the summary row to see the update.

After re-reading #10385, though, it looks like you do plan a major rewrite of discover that might do this as part of it. So, I can just keep this in my own fork if you don't want it. I filed #11626 which is part of my hack so that could probably be closed as well.

@Bargs
Copy link
Copy Markdown
Contributor

Bargs commented May 8, 2017

Got it, thanks for the details @trevan. @lukasolson @weltenwort what do you guys think? I'm thinking the only risk is possible performance implication, but perhaps because it's a shallow watch it wouldn't be a problem.

@weltenwort
Copy link
Copy Markdown
Member

It is a $watchCollection, so it is not as cheap as a simple $watch. But I don't trust my intuition with regards to angular digest loop performance. Maybe a quick profiler run with a large-ish result set could already add some substance to our speculations.

At least we would get rid of the row.highlight special case this way, I guess.

As @trevan guessed the React refactoring should not have that problem.

@Bargs
Copy link
Copy Markdown
Contributor

Bargs commented Aug 4, 2017

@weltenwort has continued to make steps toward a larger rewrite of the doc table, so I'm going to close this for now.

@Bargs Bargs closed this Aug 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants