Update apps for element filtering#577
Conversation
|
whats the curernt plan for this and #578 ? |
If you ignore the changes to DMC_ICE/X23 apps (which were starting points for the full filtering), the main change each app needs is what's in So if you take a look at the NEB app changes + backend changes, and see what you think? I can work through the other app changes reasonably quickly. #578 is where the bulk of the work is left, since we need to save the It's not conceptually that difficult, but often not entirely trivial either. Most of the ones I've changed and pushed work, but a couple e.g. CPOSS209 aren't quite working yet, but the idea should be reasonably clear. |
| """Register callbacks with app.""" | ||
| pass | ||
|
|
||
| def filter_table(self, filter_elements: list[str] | None) -> dict[str, dict]: |
There was a problem hiding this comment.
i think this funciton mutating self.table.data is dangerous for the hosted app with multiple users. i think we should build from deepcopy(self.original_table.data)?
There was a problem hiding this comment.
Hmm yes I think you're right, thanks, I'll fix this soon
There was a problem hiding this comment.
we should also be careful if theres anything else that could have a similar effect, as its less strightforward to spot when we test locally
There was a problem hiding this comment.
Actually I don't think we have a problem with self.original_table.data. We only access specific floats from it, which are immutable, so it shouldn't be changed in this process. If it was, when we applied the filter, wouldn't the metrics all be set to None, and so we'd be unable to unfilter?
Still looking into self.table.data. I get the point, but surely we ultimately do change it through callbacks anyway? We set the user specific store first, but this is still propagated to the table data itself, and that's fine?
There was a problem hiding this comment.
the callbacks are all user-side (client DOM) so theres no issue with those, but self.table.data is server side so its shared. i think its an issue as self.table.data is used for the initial render, so when its edited, later users will not all load the same app. if we build from teh deepcopy then we dont touch the original
There was a problem hiding this comment.
like self.table.data is a python object on the server than exists once, so editing this will edit for any other users
There was a problem hiding this comment.
Oh true, sorry yep, I forgot self.tables aren't the same as the DataTables we modify
| app = entry["app"] | ||
| outputs.extend( | ||
| [ | ||
| Output(f"{app.table_id}-computed-store", "data"), |
There was a problem hiding this comment.
allow duplicate needed here?
2 participants
Pre-review checklist for PR author
PR author must check the checkboxes below when creating the PR.
Summary
Continuation of #559.
This introduces the app changes needed to actually apply the element filter. This needs to be generalised and added to all apps. X23/DMC_ICE are a little out of date currently, and testing won't work until the analysis is updated and re-run.