-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: wizarding #127
base: main
Are you sure you want to change the base?
feat: wizarding #127
Conversation
doc/0003-wizarding-api.md
Outdated
│ │◀───register───│ ┃ │ | ||
│───register────▶│ │ ┃ │ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do these plugins register themselves? Is this a new type of communication from the plugin to open-scd-core?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no nothing new, just by defining them in the array as right now
doc/0003-wizarding-api.md
Outdated
┌─────────────────┐ ┌───────────┐┌─────────────────┐ ┃ | ||
│ Editor Plugin │ │ Core ││ Wizard Plugin │ ┃ LEGEND | ||
└─────────────────┘ └───────────┘└─────────────────┘ ┃ ┌───────────┐ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does "Editor Plugin" in this case include a menu plugin?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no, I was just focusing on the the "editor vs wizard" aspect :)
I could include it if leaving it out only creates confusion
doc/0003-wizarding-api.md
Outdated
│ requests a │ │ ┃ │ | ||
│───wizard for──▶│ │ ┃ | ||
│ a given │ asks wizard │ ┃ | ||
│ │───if it can ─▶│ ┃ ──<desc.> ─▶ Initiated action | ||
│ │ handle given │ ┃ | ||
│ │ │ ┃ | ||
│ │◀ ─ ─answer ─ ─│ ┃ ─ <answer> ▶ Answer to an action | ||
│ │ │ ┃ | ||
│ │ initiates │ ┃ | ||
│ │ wizard │ ┃ ┌─┐ | ||
│ │───with the ──▶│ ┃ │ │ | ||
│ │ given ┌┴┐ ┃ │ │ | ||
│ │ element │ │ ┃ │ │ Internal process |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens when there is still a wizard being displayed while the request comes in? Currently, we push the incoming wizard to the front or back end of the wizard deque (currently called "workflow") depending on whether it's invoked as a subwizard or not. How does this proposal address the common case of a wizard requesting another wizard?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be out of scope, I would say.
Is there an option that would greatly affect the rest, and can we delay the decision?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we delay the decision, would wizard plugins written before we make the decision stop working once we have made it? Is there any way we could grow into this thing in a backwards compatible fashion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is necessary to keep this in mind, but my initial feeling is that we can solve this in a compatible way
doc/0003-wizarding-api.md
Outdated
│ │ closes ┃ | ||
│ │ itself ┃ | ||
│ │ │ │ ┃ | ||
│ │ ◀──┘ ┃ | ||
│ │ │ ┃ | ||
X X X ┃ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For invoking the next wizard on the deque (in order to know when to render a wizard plugin at all, and to move on to the next wizard) and for deciding when to stop rendering the wizard selector button (which will be needed to achieve feature parity with the current legacy open-scd implementation) we will need to know when the wizard is done with its work. An event that says "I'm done" would likely be needed here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would you prefer an event after the closure or an event that asks the core to close the wizard?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have a preference, but it's a decision I would really want us to think about the consequences of. Whether any wizards ever could need any cleanup, or whether "well, you're not being rendered any more now!" is always going to be acceptable as a way for core to close wizards. In the latter case I think I'd prefer that since it's simpler and doesn't require for the wizard itself to do any opening or closing, just to always render its stuff. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree and I have already made changes
doc/0003-wizarding-api.md
Outdated
Wizards must have two public static functions called `CanInspect` and `CanCreate`. | ||
`Core` will call these functions when it tries to figure out which wizard supports | ||
the given element and action. | ||
```ts | ||
type CanInspect: (tagName: string) => boolean | ||
type CanCreate: (tagName: string) => boolean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not name these "Edit" and "Create", respectively? Also, isn't a name that starts with a lowercase letter more usual for methods?
Wizards must have two public static functions called `CanInspect` and `CanCreate`. | |
`Core` will call these functions when it tries to figure out which wizard supports | |
the given element and action. | |
```ts | |
type CanInspect: (tagName: string) => boolean | |
type CanCreate: (tagName: string) => boolean | |
Wizards must have two static methods called `canEdit` and `canCreate`. | |
`Core` will call these methods when it tries to figure out which wizard supports | |
the given element and action. | |
```ts | |
type canEdit: (tagName: string) => boolean | |
type canCreate: (tagName: string) => boolean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yes, I have not mentioned much about the naming.
So with "inspect" I've meant to signal that it could be either a read-only or an edit view. So inspect=edit/view
These are the type definitions so capital letter but I usually capitalise static stuff but I don't insist on it (But the types I would keep capitalised )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't see the "type" keyword in those lines. In general I wouldn't define named types for those, but wherever we do define named types, let's definitely capitalise them, since that's the TS and JS convention. Currently in open-scd, we don't capitalise any static fields, the same way open-wc, lit, and mwc do: They have all properties and methods start with lowercase regardless of whether they're defined on the class or on the instance, just like all constants and functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tried to make the description clearer and already changed them to lowercase.
The types were just intended as a way of description, and I would only include it in the code if they help to make things more understandable.
doc/0003-wizarding-api.md
Outdated
Both event detail can contain an additional `stuff` for flexibility. `stuff` can be something | ||
that the wizard defines for additional configurations (e.g.: display it in readonly mode), | ||
or the editor can provide additional information about the element. | ||
|
||
> **⚠️ WARNING:** Please only use `stuff` if there is no other way. | ||
> It creates a tight coupling with a specific wizard or editor and it cannot be | ||
> guaranteed that your expectation about the `stuff` will be met! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to introduce an extra background hatch to the event that comes with a warning not to use it? What use case are we solving by this? "for flexibility" sounds scarily vague to me. The current problems with the original wizard API came about exactly because we designed it "for flexibility".
The proposal in the Wizard API issue from last year's April was exactly to make the event a pure declaration of intent to edit or create a particular element. For what purpose do we need something other than that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh missing example from my part sorry
I was thinking maybe a plugin wants to display a readonly view so id would send
stuff:{
readonly: true
}
However, I like the idea to leave it out and figure out a solution when we get a problem
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would generally suggest that whether something should be editable or not should be up to the distribution. So for example, my distribution might include a wizard library that gets information about the current stage of the engineering process from the file and information about the user role from, say, an OpenID connect provider, and then shows read-only wizards for those elements which can not be edited by a user with this role at this stage of the engineering process. So if we want e.g. read only views in general, let's include a read only wizard library. This would mesh well with the declaration of user intent "I want to edit/create this element" that was the idea behind the API.
Do we believe "I want to view this element but I would be really angry if I could also edit it, so please don't allow me to do that!" to be a separate possible user intent? I currently don't see the use case for that, but if we do, I think it would be prudent to introduce a third type of wizard request just for viewing. What do you think about all this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you are right. This is more like a UX problem maybe and we have to decide on a case-by-case basis.
doc/0003-wizarding-api.md
Outdated
##### Self Closing | ||
|
||
The wizard decides when it is finished and closes itself. | ||
|
||
`Core` has always the possibility to close the wizard. | ||
For example when clicking on the backdrop of a dialog. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As commented above, we'd likely need some API to let core know when the wizard has done its work.
The advantage is that we can enforce their definition, | ||
what we cannot do with a static functions as of now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by "enforce their definition"? Who would do the enforcing, and how?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TypeScript would, wouldn't it?
If you want to define a wizard plugin you would need to define ´handledTags´
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Plugins don't need to be written using TypeScript at all, I've written them in straght JS before, but a plugin could just as well be written in ClojureScript or Elm, for example.
Also, I think we currently don't enforce anything on plugins, the fact that they have to be ES Modules with an HTMLElement constructor as a default export is just a convention, just like not editing the DOM directly but using the Edit API, or having a run() method if you want to work in the menu. I think as long as we're in the JavaScript world, we can't really do more than come up with easy to follow and strong conventions, so documenting them is important. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true, true. I might reformulate or remove it. Not sure yet
doc/0003-wizarding-api.md
Outdated
Plugin developers may forget that they can get two different types of event details, | ||
but if we have two different events with always the same type of payload, | ||
there is nothing to forget. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean forgetting in case wizard plugin developers do not look at the API and also choose not to use TypeScript? How likely do we think that case is? Do we want to design our API around it?
Do you also see the advantage of more straightforward backwards-compatible API growth in the future with this approach?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you have to type a event yourself when you set a listener.
You can of course reuse the type from the library.
My thinking was that we don't have one "interaction" event and we have to decide depending on the event's structure if it was a click
or a scroll
event
So that is why I've though two event would be clearer
Co-authored-by: cad <[email protected]>
Often plugins need a way to display, edit and create SCD elements. | ||
We want to avoid to put too much effort into implementing the same thing over and over again. | ||
Also we want to speed up plugin development by extracting some common functionalities out of the plugins. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would depend on the wizard's author what content id displayed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oke, any content will work :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The plug-in author will still be responsible on how the dialog looks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If by plug-in you mean the wizard-library plug-in, then yes. If you mean the plugin that calls the wizard, then no: The idea for the API that we've come up with throughout April 2022 is that the plugin sends a pure declaration of intent: in this case that would be "Please allow the user to create a Substation element in this SCL document!" and then the wizard library would be responsible for displaying some dialog that would allow the user to do that. That dialog, in our default wizard library, would look exactly as you have shown there, with the "Guess content" button. Does that make sense?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thinking here. The Wizard-Plugins are 100% responsible of their visuals and content.
- show menu plugin in the diagram to avoid confusion - replace self-closing with a done event and closing by core - clarify registration of plugins - clarify wording of actions - remove "stuff" from the definition - add better (hopefully) argument for the two-event system - define what is out of scope
Rendered version: Wizarding API
The PR is meant for discussion and not to merge it without any code change