neighbour-hoods / sensemaker-lite

11 stars 1 forks source link

Assessment Widget Tray configuration API #67

Closed adaburrows closed 1 year ago

adaburrows commented 1 year ago

There should an assessment widget tray configuration API. It should:

pospi commented 1 year ago

See linked PR for where I ended up with this. I made some revisions to the types to use a Rust enum, which Serde can output in a "higher-kinded union type" sort of way. Each block is now stored as a DimensionBinding; which may be either a DimensionWidgetBinding or a DimensionAppletFeatureBinding.

This means that the outer ResourceDefAssessmentWidgetTrayConfig & AssessmentWidgetTrayConfig data structures can be constructed purely from DHT links from an originating ResourceDefEH.

AssessmentWidgetBlockConfig is no longer needed, either. In this format there is no real need to differentiate (input|output)_assessment_widget nor to limit how many AssessmentWidgetBlockConfigs may be bound to a ResourceDefEH.

There is currently no agreement about having multiple components, one for assessment and one for pure output of computed assessments.

We could configure this simple case by associating an 'input' Widget followed by an 'output' one; resulting in two DHT links from a ResourceDefEH to two different DimensionBindingEHs.

pospi commented 1 year ago

Data structures and minimal read logic implemented and compiling.

Still to do:

adaburrows commented 1 year ago

AssessmentWidgetBlockConfig is no longer needed, either. In this format there is no real need to differentiate (input|output)_assessment_widget nor to limit how many AssessmentWidgetBlockConfigs may be bound to a ResourceDefEH

This is incorrect. We still need to differentiate between input and output assessment widgets because they have different purposes in the UX. The UI must be able to differentiate between them in order to properly associate them with their slots in the UI. While theoretically, we could just look it up in the configuration of the app, IMO it's better to just return all the data the rendering code needs. This also allows us to ensure that each application is only getting the data it needs and nothing more.

The AssessmentWidgetConfig is specifically for when applications register a widget so that our backend knows about it instead of it residing purely in the client code (and requiring all apps to have been loaded before that data is available).

The AssessmentWidgetBlockConfig is specifically for the AssessmentWidgetTray to know which blocks are available to render. Yes, there's still some rough edges here because our design team hasn't come up with a coherent design for where everything goes, but if we do keep the current UX of the Todo app, then we need an input only assessment widget tray on the right side and an output only assessment widget tray on the left. This means we need to filter on that specific field.

While you're linking approach bypasses having multiple AssessmentWidgetTrayConfigs for different ResourceDefEHs, your code links to the wrong object, because you omitted the AssessmentWidgetBlockConfig. Additionally, in the future, we may also need to filter by context_eh, which would add another link type to this structure. I'm not exactly sure what the performance characteristics are of filtering across two to three link types (see below for why this might happen).

This means that your code does not comply with the requirements, and you should not have checked off the third item, because the methods you implemented do not satisfy the exact requirements for the UI for that item.

Possible ways to rectify this are to:

adaburrows commented 1 year ago
  • validation rules (do we want these features restricted to CAs, or..?)

Do we currently have a role for who's the CA? I don't believe we do. So validation on that aspect if not possible.

pospi commented 1 year ago

Made a quick pass at adding the wrapper struct, which I think resolves most of the above.

I'm still unsure on the last bullet point. Is that not the (get|set)_assessment_widget_tray_config endpoints? If so, I think this is addressed upon updating the inner data structures in 1fe6ca0 and 66c0b07. If not I have misunderstood the original API contract.

Do we currently have a role for who's the CA? I don't believe we do. So validation on that aspect if not possible.

The other integrity zomes are all governed by validation logic in zomes/integrity/sensemaker/src/lib.rs. A lot of them call through to validate_author_as_ca() which does some weird numeric EntryDefIndex matching which feels brittle, but is probably only dependent upon the definition order in YAML files and so perfectly adequate. That then calls through to Properties::is_community_activator() to match against the AgentPubKey given as part of the DNA properties.

If it is a good idea to add it, then I would advise splitting the integrity zome lib file properties.rs and the validate_author_as_ca() helper out into a shared crate for pulling into zomes/integrity/widgets so that a similar validation callback can be declared there. Let me know if I should go ahead with that?

Tests coming tomorrow but have to sign off for the day in a moment.

pospi commented 1 year ago

Thanks also for the more detailed description on the background to these APIs, although apologies also for making you feel as though it needed to be documented in further detail. The code comments in the OP lead me to see this as a more exploratory API contract than I think was intended :pray:

Feel free to leave answering for another time if you prefer, but I wanted to clarify this detail because I'm not sure I understand the different delivery mechanisms and it may be a point of misunderstanding in mine (and perhaps consequently others) knowledge of the proposed architecture:

AssessmentWidgetConfig is specifically for when applications register a widget so that our backend knows about it instead of it residing purely in the client code (and requiring all apps to have been loaded before that data is available)

I'm unsure what "residing purely in the client code" might entail. But my guess is it relates to the two different configuration types currently declared as DimensionWidgetBinding and DimensionAppletFeatureBinding in the zome code? Is there some sort of "direct upload" functionality envisaged for Widgets that enables ESModule code to be directly loaded into a storage zome, or something?

I was under the impression that Applet installation was the only avenue toward adding new features to a Neighbourhood. In the case of how this relates to https://github.com/neighbour-hoods/nh-launcher/issues/43 I had imagined that the process would be that an Applet bundle file is parsed by a background Tauri thread, CustomElement ESModules extracted from some manifest, and those then stored individually into a storage zome. The Applet bundle itself would then be discarded and replaced by some index that links the provided CustomElements together as being part of the same install.

If we are also seeing this as an opportunity to open up development possibilities into more modular and discrete contributions then this is very cool! Not something I had thought about much.

...if I am getting this right then a simple thumbs up reaction is the only necessary response :wink:

pospi commented 1 year ago

Yes, there's still some rough edges here because our design team hasn't come up with a coherent design for where everything goes

Also wondering & having trouble remembering: is there a 1:1 relationship between a Resource Def and a CustomElement? (I didn't think so, I thought these were currently broken down by "renderer" types.)

If not, in a future iteration it might make sense to bind AssessmentWidgetBlockConfig on a per Resource Def, per CustomElement basis; as this would allow the templates to define their own slot IDs which would make a raw hashmap or vector style of flexibly configuring DimensionBindings a semantically appropriate means of assigning UI elements to specific visual positions as dicatated by component authors.

pospi commented 1 year ago

Tests are implemented and passing. And yeah, I see what I've done: there are multiple AssessmentWidgetBlockConfig per ResourceDefEh but I only have one now. Should mostly be handled by reverting 66c0b07 which is closer to the desired behaviour.

I will need to do a little work on ensuring ordering as there will be some shuffling of multiple configs as they persist through updating the whole array. Probably the easiest way to manage is just to delete all links and recreate them in order each time.

adaburrows commented 1 year ago

Also wondering & having trouble remembering: is there a 1:1 relationship between a Resource Def and a CustomElement? (I didn't think so, I thought these were currently broken down by "renderer" types.)

Currently, there's only a 1:1 relationship between components that render resources and the ResourceDef. In the future, that might change. Because there might be different kinds of views of things for different view context settings. Like maybe an image could have a square view or a rectangular block for preview image and textual info next to it. Then a context could be configured to show objects with either of those views. But that has nothing to do with this API.

As far as this API is concerned, each time a Resource is displayed, there needs to be an AssessmentWidgetTrayConfig associated with a ResourceDef that has a specific set of mappings between input dimensions to input widgets along with a computed dimension to show for each widget (and possibly output widgets, but the design team did vote to nuke that today, so it's possible that will change shortly, but just keep it in there). In the future, we may need to have an AssessmentWidgetTrayConfig that is chosen both by ResourceDef and ContextEH, so that should be considered in this implementation.

adaburrows commented 1 year ago

I'm unsure what "residing purely in the client code" might entail. But my guess is it relates to the two different configuration types currently declared as DimensionWidgetBinding and DimensionAppletFeatureBinding in the zome code? Is there some sort of "direct upload" functionality envisaged for Widgets that enables ESModule code to be directly loaded into a storage zome, or something?

Yes, so the bit about "residing purely in the client code" is that we need to download the whole applet from the applet store and load it in the browser in order to get to the components that it provides, and that point the code effectively is only ever residing in memory in the client side. So yeah, my choice of words was a little confusing.

In the future, I see us being able to just add in components as standalone things. Need a new component to view a particular resource? Just install it. Need a new widget to assess with? Just install the component. Need a new resource? Just install it. Need a full screen interactive app to do something complicated? Just install it along with the resource types and other views it needs.

Who knows, maybe even the types can be abstracted out a bit into RDF schemas then everything can use JSON-LD as the main format and we can extend out past Holochain. Or auto generate zome code from the RDF schema, and let people add custom validation logic for the zome. There's paths to explore in the future.

adaburrows commented 1 year ago

If it is a good idea to add it, then I would advise splitting the integrity zome lib file properties.rs and the validate_author_as_ca() helper out into a shared crate for pulling into zomes/integrity/widgets so that a similar validation callback can be declared there. Let me know if I should go ahead with that?

Yeah, go for it.

adaburrows commented 1 year ago

Whoops! Didn't mean to close the issue when commenting.

adaburrows commented 1 year ago

Probably the easiest way to manage is just to delete all links and recreate them in order each time.

Go ahead and delete and recreate the links. If it becomes a performance issue, we'll likely just have to change the approach.

adaburrows commented 1 year ago

Just for reference, the nh-launcher has a task that will use this: Assessment widget tray configuration screen