Skip to content
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

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

feat: wizarding #127

wants to merge 6 commits into from

Conversation

trusz
Copy link
Member

@trusz trusz commented Jul 10, 2023

Rendered version: Wizarding API

The PR is meant for discussion and not to merge it without any code change

@trusz trusz marked this pull request as draft July 10, 2023 19:40
Comment on lines 51 to 52
│ │◀───register───│ ┃ │
│───register────▶│ │ ┃ │
Copy link
Contributor

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?

Copy link
Member Author

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

Comment on lines 46 to 48
┌─────────────────┐ ┌───────────┐┌─────────────────┐ ┃
│ Editor Plugin │ │ Core ││ Wizard Plugin │ ┃ LEGEND
└─────────────────┘ └───────────┘└─────────────────┘ ┃ ┌───────────┐
Copy link
Contributor

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?

Copy link
Member Author

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

Comment on lines 54 to 66
│ 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
Copy link
Contributor

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?

Copy link
Member Author

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?

Copy link
Contributor

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?

Copy link
Member Author

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

Comment on lines 74 to 79
│ │ closes ┃
│ │ itself ┃
│ │ │ │ ┃
│ │ ◀──┘ ┃
│ │ │ ┃
X X X ┃
Copy link
Contributor

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.

Copy link
Member Author

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?

Copy link
Contributor

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?

Copy link
Member Author

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

Comment on lines 91 to 96
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
Copy link
Contributor

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?

Suggested change
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

Copy link
Member Author

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 )

Copy link
Contributor

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.

Copy link
Member Author

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.

Comment on lines 120 to 126
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!
Copy link
Contributor

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?

Copy link
Member Author

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

Copy link
Contributor

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?

Copy link
Member Author

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.

Comment on lines 165 to 170
##### 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.
Copy link
Contributor

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.

Comment on lines +187 to +188
The advantage is that we can enforce their definition,
what we cannot do with a static functions as of now.
Copy link
Contributor

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?

Copy link
Member Author

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´

Copy link
Contributor

@ca-d ca-d Jul 12, 2023

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?

Copy link
Member Author

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

Comment on lines 205 to 207
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.
Copy link
Contributor

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?

Copy link
Member Author

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

doc/0003-wizarding-api.md Outdated Show resolved Hide resolved
Co-authored-by: cad <cad@sdf.org>
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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this limited to SCL editing only? or could it e.g. also contain buttons for functions e.g. auto button e.g. Guess content:

image

Copy link
Member Author

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

Copy link
Member

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 :)

Copy link
Member

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?

Copy link
Contributor

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?

Copy link
Member Author

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants