Read custom visualizations #8180
Conversation
The current problem is that we need to somehow know the file path of the project root.
To do: updating lookup for visualizations for type; removing outdated styles
somebody1234
added
CI: No changelog needed
app/gui2/src/components/visualizations/ErrorLoadingVisualizationVisualization.vue
Outdated
Show resolved
Hide resolved
app/gui2/src/stores/visualization.ts
Outdated
There was a problem hiding this comment.
This file became big and is hard to read, moreover has no coverage in unit tests. Could we split it to multiple modules? I would see for example a separate class which keeps synchronized all maps like "typesOfVisualization", "visualizationForTypes" etc. - or maybe we could make use of the structure used by suggestion database? Also, a separate file for communication with compiler worker (applying notifications could be tested I think).
There was a problem hiding this comment.
side note: i think loadScripts fits better in its own file too, but the current logic is too specific to visualization for it to make sense to refactor out
|
most review issues should be addressed, but this may be [x-on-hold] depending on how busy i am? re: (note that removing styles, and reloading visualizations, are still not implemented) |
…izations; code splitting for large viz dependencies; fix bugs
|
things should be mostly addressed, i believe most of what's left should be writing more tests? @farmaazon also, just wanted to know your thoughts on how |
| return JSON.parse(key) | ||
| } | ||
|
|
||
| export class VisualizationMetadataDb extends ReactiveDb<VisualizationId, VisualizationMetadata> { |
There was a problem hiding this comment.
On one hand I really don't like using extends and almost always prefer composition (except for very specific classes like Observable). But on the other hand, in this specific case it does look good, and maybe we should also move SuggestionDb to that same model. Will there ever be a situation where we want more ReactiveDb instances (i.e. tables) in the actual "database" type? If so, then we should really use composition. Otherwise I'm fine with this.
There was a problem hiding this comment.
hmm... fwiw i think it's fine to instead require Db classes to always be single tables - it's not like ReactiveDb supports multiple tables either, even though it is called a Db, right?
but yeah, the intent is for this to always be a single table - the name is supposed to mean, more or less, "a table associating each visualization type with its metadata". (i.e. a single table, where the key is the visualization type, and the value is the metadata)
The list of visualizations is not correct on the user-defined viz story
|
... actually i don't think the mocks are that hacky after some of the newer changes however... i think i might have to leave this PR as it is for now in favor of some stuff that's a bit more urgent. |
|
or perhaps someone would be interested in cleaning it up |
|
requesting one (potentially) final review for the mock impl which is kinda scuffed |
|
@farmaazon just a heads up, i still haven't added tests for compiler messaging. i could maybe? try? but i can't really think of any tests that don't just... test that the implementation is doing exactly what the implementation is doing. i think this may just be something better left for integration and/or e2e tests, but if you do have some ideas on what tests i can add then i'd be more than happy to add them |
|
Incredible work on that FS mock. |
Pull Request Description
Important Notes
Tests are still WIP
Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
The documentation has been updated, if necessary.Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.Scala,
Java,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
Unit tests have been written where possible.If GUI codebase was changed, the GUI was tested when built using./run ide build.