Cleanup unused control blocks

[danyill]

This is part of #290 (comment) and deals with the initial request.
It is the second of at least three discrete panels for the Cleanup editor and resolves:

• ReportControl with no or invalid datSet
• LogControl with no or invalid datSet
• GSEControl with no or invalid datSet
• SampledValueControl with no or invalid datSet

Bearing in mind that for ReportControl having no or an invalid datSet may be fine in an ICD file or if datasets are dynamically allocated.

What I did:

• Add to Cleanup editor a new section: "Control Blocks with a missing or invalid dataset" which covers GSEControl, LogControl, ReportControl and SampledValueControl (why did the standard not make this SVControl?!)
• Provide the optional ability to remove SMV/GSE addresses at the same time (as a default).
  The goal is that each atomic operation should if possible result in a conforming file that can be validated (?hmmm... there are problems here but this margin is too small to explain them)
• Provide the ability to edit ReportControl, SMVControl and GSEControl elements from each list item.
• Provide a caution/warning icon if a GSEControl or SampledValueControl is mapped to an address in Communication > ConnectedAP SCL section
• Made this editor more responsive for small form factor devices or very large screens using flex.
  In particular, flex made it easier to ensure that the rows are the same height - I think this gives the best "organisation" for the different cleanup functions.

Unsolved:

• My CSS-fu is weak but is weaker with CSS Grid so we're using flex now. I would prefer the checkbox above the button on small screens but have struggled to make flex do my bidding for all form factors.
• I am not sure how to eliminate the scrollbars associated with the filtered-list. They shouldn't be there and yet they are... ❓
  I think this is due to a mwc-ripple element (from the Firefox scroll button) but I am not sure how to diagnose further.
• I thought about using a WizardCheckBox for the bottom checkbox but initialising to a checked item and the ability to disable it weren't there. Perhaps I just didn't understand them. Things which were @state I thought should be @property. Conscious that I understand almost nothing I did not let this hold me up but I can go back to it on request... 😕
• For removing the relevant sections in the Communication > ConnectedAP section, Figure 22 of IEC 61850-6 Ed 2.1 shows a link between tAddress and tControlBlock. As far as I can tell, this is an error 🐛 or at least I can't see where those semantics are defined.
  

Things I'm still working on:

• Tests
• Seeing if my flex and CSS have too much cruft
• Contemplating form and style 👔

As always, any reviews and comments are gratefully accepted.

(FWIW after this the templates section and deal with:

• LNodes that are invalid reference
• LNodeType, DOType, DAType and EnumType that are not referenced)

[danyill]

I think the scrollbars caused by the ripple is similar to this issue and may be caused by improper semantics or using elements in a way that material-web does not like

[JakobVogelsang]

Looks promising. Thank you for the contribution. I have only one UI remark: The elements Ropert, GSEControl and SampledValuesControl do look very alike in the list. May the icons we have for those might be usefull? Or having three seperate lists?

[danyill]

Looks promising. Thank you for the contribution. I have only one UI remark: The elements Ropert, GSEControl and SampledValuesControl do look very alike in the list. May the icons we have for those might be usefull? Or having three seperate lists?

Thanks 😉
I need to let it gestate for a little and then I can make some more improvements.

1. I recall that you can filter by the control block type in the list because of the marvellous filter and the identity function.
2. I feel like three (four if we include LogControl) lists is too much here when the function performed is identical.
3. I'd be happy to add the icons to each element. I think you mean these ones:
   
4. I guess another option is to have a set of tick boxes or radio buttons up the top to filter on the control block type?
   I'll add the icons first and then let's see what we think. Since in most other places in the UI we expect the user to search with the filter (e.g. in the unreferenced datasets for manufacturer, device type or device name) I think we should reinforce that pattern. It also keeps the UI uncluttered.

[JakobVogelsang]

I like the fourth point. We have actually a very nice code example with the log how this filtering can be done with CSS. Also, a good learning experience - it was for me !!!

[JakobVogelsang]

To the SCL related question above:

Be careful as the name tControlBlock is missleading. This is a complex type purely used in the Communication section. You are mixing it with tConntrolWithTriggerOpt and tControl that are complex types used to define commons of control blocks in the IED section

[danyill]

The elements Ropert, GSEControl and SampledValuesControl do look very alike in the list. May the icons we have for those might be usefull? Or having three seperate lists?

I like the fourth point. We have actually a very nice code example with the log how this filtering can be done with CSS. Also, a good learning experience - it was for me !!!

Thanks for your feedback. 👍

I have mocked up an interface as I think you suggested:

1. For the icon to exist the checkboxes must return to the right. OK, I'll choose a max-width of about 700px to prevent visual drifting.
2. I am having a small battle with the CSS to the filtering but I hope to soon be victorious. The purple is selected in the picture above.
3. I think the existing log interface is a little buggy. Partly because the colours don't change for me in Chrome as I click on the buttons but I can work around that here. More bothersome is that sometimes the "hover shadow" doesn't leave when the mouse cursor goes away from the button. I may try to figure this out.

To the SCL related question above:

Be careful as the name tControlBlock is missleading. This is a complex type purely used in the Communication section. You are mixing it with tConntrolWithTriggerOpt and tControl that are complex types used to define commons of control blocks in the IED section

OK. My line of thought is that following this diagram I expected to find a definition of tAddress which contained or referenced tControlBlock which I think must have tCBName and yet it does not.

Incidentally, on that topic should this function also be removing the items in the red square in the picture below if the tick box at the bottom is checked?

In which case the tick box would say: "Remove dependent elements" or similar. WDYT. Are there other elements we should consider?

I currently think the default action, even in the cleanup should be to result in a conforming file which means if the following reference the control block they should be removed:

• tServiceSettings
• tReportSettings
• tGSESettings
• tSMVSettings

Should this also include relevant ExtRef elements in the Inputs section?

Hmm... I think I need to do some refactoring so this won't be ready for code review for a little while yet.

[danyill]

I currently think the default action, even in the cleanup should be to result in a conforming file which means if the following reference the control block they should be removed:

• tServiceSettings

• tReportSettings

• tGSESettings

• tSMVSettings

Should this also include relevant ExtRef elements in the Inputs section?

Interestingly, removeReportControlAction in the Wizard reportcontrol.ts does not do any of this either.
Perhaps it doesn't need to? Perhaps "undefined references" should be its own stage of cleanup/validation.
While a file may pass schema validation without this it seems messy to me.

[JakobVogelsang]

Hi @danyill,

you got it right, we do not check everything when adding, editing and removing things. This is why I am so careful to not call it a system configuration tool. OpenSCD is at this time and stage an incomplete SCL editor. We are heading towards the goal to become a complete SCL editor, which I think can be done in a couple of months. There are two things we decided to do it like that. 1. There are usecase where global validity checks are not needed, or in other works were a pure SCL editor makes sence to have already. 2. I did not have confidence/knowledge at the beginning to do all the needed checks. Those checks I know not need to be installed globally so they are usable by all plugin authors. If you want to see some first things we check globally have a look at Editing, where we check duplicate name attribute within the same parent and for global unique id attributes.

As the plugin author, you can decide how you want to deliver the Cleanup editor. If you want to make it complete from the beginning, I can offer you to add some IEC 61850 requirements on the issues you are working on. For me, it is approvable if you don't violate the schema.

Best regards,
Jakob

[danyill]

Thank you for this feedback.

1. I have now finished the UI work associated with the feedback above (icons and filters). I hope you like it!

2. I have refactored each cleanup into a separate file (possibly introducing regressions or cruft particularly for the CSS)

3. I would be grateful for a quick usability review to see how the filters work/feel with the checklist (in the particular the check all button). The test/testfiles/cleanup.scd should give a flavour. For a more difficult case, I'm using this:

   IOP_2019_HV_v9_ed2.1.scd.zip

   I think the UI is more interesting and better (but also more complex). The CSS filtering is neat.

   I am now too close to it to understand whether it is easy to use or clean or intuitive.

[JakobVogelsang]

Thank you for this feedback.

1. I have now finished the UI work associated with the feedback above (icons and filters). I hope you like it!

2. I have refactored each cleanup into a separate file (possibly introducing regressions or cruft particularly for the CSS)

3. I would be grateful for a quick usability review to see how the filters work/feel with the checklist (in the particular the check all button). The `test/testfiles/cleanup.scd` should give a flavour. For a more difficult case, I'm using this:
   [IOP_2019_HV_v9_ed2.1.scd.zip](https://github.com/openscd/open-scd/files/8337160/IOP_2019_HV_v9_ed2.1.scd.zip)
   I think the UI is more interesting and better (but also more complex). The CSS filtering is neat.
   I am now too close to it to understand whether it is easy to use or clean or intuitive.

Hi @danyill,

I had a look at the UI and like it a lot. The buttons work very intuitively, and you would get my approval on this already. I had an idea how to solve the discussion with dynamic reporting: why not filter our report control blocks per default. I think this way the tool would signalize that removing those are not very common.

Furthermore, I had a first glimpse at the code and this is looking promising too! Isn't it crazy how performant the CSS filtering works! This stuff is great, unfortunately harder to test as well. We need visual regression test at the end.

What do you think can I start with code review? Or do you want to add/edit/remove something first?

[JakobVogelsang]

The elements Ropert, GSEControl and SampledValuesControl do look very alike in the list. May the icons we have for those might be usefull? Or having three seperate lists?

I like the fourth point. We have actually a very nice code example with the log how this filtering can be done with CSS. Also, a good learning experience - it was for me !!!

Thanks for your feedback. 👍

I have mocked up an interface as I think you suggested:

1. For the icon to exist the checkboxes must return to the right. OK, I'll choose a max-width of about 700px to prevent visual drifting.

2. I am having a small battle with the CSS to the filtering but I hope to soon be victorious. The purple is selected in the picture above.

3. I think the existing log interface is a little buggy. Partly because the colours don't change for me in Chrome as I click on the buttons but I can work around that here. More bothersome is that _sometimes_ the "hover shadow" doesn't leave when the mouse cursor goes away from the button. I may try to figure this out.

To the SCL related question above:
Be careful as the name tControlBlock is missleading. This is a complex type purely used in the Communication section. You are mixing it with tConntrolWithTriggerOpt and tControl that are complex types used to define commons of control blocks in the IED section

OK. My line of thought is that following this diagram I expected to find a definition of tAddress which contained or referenced tControlBlock which I think must have tCBName and yet it does not.

Incidentally, on that topic should this function also be removing the items in the red square in the picture below if the tick box at the bottom is checked?

In which case the tick box would say: "Remove dependent elements" or similar. WDYT. Are there other elements we should consider?

I currently think the default action, even in the cleanup should be to result in a conforming file which means if the following reference the control block they should be removed:

* `tServiceSettings`

* `tReportSettings`

* `tGSESettings`

* `tSMVSettings`

Should this also include relevant ExtRef elements in the Inputs section?

Hmm... I think I need to do some refactoring so this won't be ready for code review for a little while yet.

For this feature I would should as a minimum consider the followings:

1. Removing DataSet: nothing as there cannot be any valid ExtRef to a DataSet that is not used by a control block
2. Removing GOOSE/SMV: as a minimum, remove the GSE and the SMV element in the Communication section.
3. Removing ReportControl: as a minimum nothing

We do not need to remove anything from the Service section. The settings there tell you what you can and cannot do in the IED the Service section resides in.

A SCT should in addition consider the ReportSettings, LogSettings, GSESettings and SMVSettings. If any of the control block setting and the control block name there is Fix !! which is also the default!! the SCT may not be able to remove those from the SCL file.

[danyill]

I had a look at the UI and like it a lot. The buttons work very intuitively, and you would get my approval on this already. I had an idea how to solve the discussion with dynamic reporting: why not filter our report control blocks per default. I think this way the tool would signalize that removing those are not very common.

Thank you.
That is a good idea 💡 It shall be as you wish. In fact it already is 🧚‍♂️

Furthermore, I had a first glimpse at the code and this is looking promising too! Isn't it crazy how performant the CSS filtering works! This stuff is great, unfortunately harder to test as well. We need visual regression test at the end.

It is quite performant although I have to iterate over all items and add them to a class. Hmmm there may be an easier way to do that all in one go?

I think it may be beyond my ability to add visual tests at this time at least as part of this PR.

What do you think can I start with code review? Or do you want to add/edit/remove something first?

I am sorry for the delay ⏳, I am but a 🐌 who dreams of being a 🐇

What I want to do:

1. split apart the cleanup containers in the code (done now)
2. make the tests separate (done now)
3. add some tests for this new set of control block cleanup (this weekend)
4. leave it two days for contemplation (I really best study the flex: clamp statement to see if what I've done is in any way defendable for instance)

but if you want to review, at any time I will be most pleased 🙏 . I think it is now in a half reasonable state...

I've done the first two and pushed them now so at least it is not all horribly munged into one file. I think that will improve review and maintenance.

For this feature I would should as a minimum consider the followings:

Removing DataSet: nothing as there cannot be any valid ExtRef to a DataSet that is not used by a control block
Removing GOOSE/SMV: as a minimum, remove the GSE and the SMV element in the Communication section.
Removing ReportControl: as a minimum nothing

We are in luck, I think that is what I have done 🎉

Incidentally:

• I do not know what is happening with tests.
• CI on Github sometimes takes more than an hour
• The snapshots are also doing something a bit odd, SMV and subscription sld-drawing seem to refresh even though I'm very confident I haven't touched these...

[danyill]

Just a few questions from me.

[danyill]

Things not done:

• When we have checks on the checklist items, they disappear if we click on the "detail view" icon to take a peek which is not great (and as a user would enrage me 😡). I played with some localStorage to try and find a workaround but was not successful so far.

• I haven't written tests for the "detail view" icon associated with each list item for either the dataset or control block cleaning.

• I haven't written tests for the enable and disable of the Remove Address button, I think this is mainly cosmetic.

[JakobVogelsang]

Close to be merged. Most comments are just a hint, except the

• variable names
• property definition
• import order

I did not see any bugs!! GREAT work on this one, and thank you very much for your contribution!!

[JakobVogelsang]

• f

On a global scale, this is the same issue as the performance issues we discuss at the moment. We need to have a closer look, when do we re-render what. As a dirty quick solution, you could use a global variable. An object, probably to save the control block identity as the object key and the selected status as the value of the object.

[danyill]

👍 Thank you for this feedback, I hope to respond and improve over the weekend.

[danyill]

As a dirty quick solution, you could use a global variable. An object, probably to save the control block identity as the object key and the selected status as the value of the object.

I think we'll skip the dirty quick solution for now or address this in a separate issue/PR.

[JakobVogelsang]

LGTM