feat(editors/cleanup) Cleanup editor

[danyill]

This deals with one request of #290 and is a foundation to eventually incorporate the rest.

I have questions and problems which will need resolution before I can progress so this is very much WIP.

• GSEControl with no or invalid datSet
• SampledValueControl with no or invalid datSet
• DataSet that are not referenced by any control block
• LNodes that are invalid reference
• LNodeType, DOType, DAType and EnumType that are not referenced

I've decided that a datatable would be a good way to address these. I want the ability to filter across different IED names, manufacturers, IED types and (e.g.) DataSets because I think a cleanup may deal with existing scd files with many IEDs or historical issues. It needs to be easy for the user to select large numbers of IEDs with fairly simple screening criteria.

Regrettably there is no material-web-component for a datatable and very little evidence that it might be arriving soon (issue has been open for three years).

There is a very excellent article on the greatness of a good data-table element and features it might have here:

https://uxdesign.cc/design-better-data-tables-4ecc99d23356

The issue regarding data tables led me to look at vadin-grid which offers (I think) lit-compatible Javascript front-end web components with a material theme. The particular "killer feature" I like is filter columns.

However snowpack (3.8.6) has an allergic reaction to this (unless I have done my imports or dependencies incorrectly which is ever so possible).
Interestingly, so does 3.8.8 but it does provide better error messages.
Intriguingly, 3.2.1 "just works". Some issues I found associated with snowpack suggest some recent regressions which (may) be related.

FredKSchott/snowpack#3724
FredKSchott/snowpack#3004
FredKSchott/snowpack#3395
mui/material-ui#26568

I am by no means finished with this, but I think it (mostly) works with the older version of snowpack.

cleanup_datasets.mp4

vaadin-grid is license compatible (Apache 2) with Open-SCD but I don't want to simply assume we should add a dependency.
I think a data table is important for OpenSCD but there may be other (or better) (or lighter) options.

So... I am up for gentle mockery, advice and instruction 😉

• I'm not very good with type handling.
• I haven't written tests (yet) but plan to.
• I haven't fully researched whether the logic is sound.
• We need to be able to click on the dataset and open up the existing wizard to show the dataset.
• I am muddled most of the time 😕

[danyill]

However snowpack (3.8.6) has an allergic reaction to this (unless I have done my imports or dependencies incorrectly which is ever so possible).
Interestingly, so does 3.8.8 but it does provide better error messages.
Intriguingly, 3.2.1 "just works". Some issues I found associated with snowpack suggest some recent regressions which (may) be related.

The error messages in 3.8.6 are typically when I click on the plugin I get a browser console error regarding cyclic references.

Sometimes instead I receive the following:

But it is not always the same error mesage.

[JakobVogelsang]

The first impression is very good. The extra dependency is not nice, we were hoping to use the mwc-data-table that is on the roadmap for the component library we are using. vaadin is powerful, but adds I think something like 8 more dependencies to the repository. One thing that is important is: we have not merged to lit2.0 and both the component libraries have this as a dependency. So you have to choose a version of vaadin-grid that still uses lit-element 2.xx and lit-hatm 1.xx.

[JakobVogelsang]

Another solution would be to have a list for each of the things you want to clean up: In the list you can then use the secon span to add more information that you want to filter for something like:

dataset_name
IEDname (manufactorer) - iedType

When you put the identity string in the value attribute of the list-item, you even can filter for the device and other parents of the dataset.

Same list can be create for empty GSE,SMV and Report control blocks. Then you have all lists next to each other on a big screen. You have a lot of code where we use this type all the time and you can use filtered-list out of the box.

[JakobVogelsang]

Don't get me wrong, I don't wont to restrict you to not work with tables. But I really think list might work better here :)

[danyill]

👍 Thanks for the feedback, I think it is fair.

Let me have a go at doing as you suggested. This is a useful learning experience for me.

(:newspaper_roll: Material Web 3 is hopefully coming late in 2022 and perhaps that will deliver something. ☺️ I guess I should be honest, my usage here is a little bit of a pretext - I ultimately want data tables for other things, here are some incomplete ramblings from a little while ago)

[danyill]

I have done something and now seek a preliminary review before I try to scale for the remaining tasks:

Some incomplete things which I would like some help with or to receive comments on:

1. 
   When I have no datasets, my button is not disabled. Fixing this currently escapes me.
2. Should the button be on the left-hand side? Or the right-hand side?
3. The button styling doesn't look correct but I'm unsure how best to improve. I think there should be more margin.
4. All German is courtesy of Google Translate and likely to be unintelligible.
5. I have not written tests before here and mocha is fairly new to me, I will take many comments. I have tried to generate a snapshot.

A few thoughts:

• I wanted to use a snackbar to indicated to the user that datasets were removed. But the validation snackbar interferes. Do we really need the validation snackbar to prevent any other snackbars? I think putting a number of errors on top of the validation icon might be an alternative to allow this to be used by other UI elements.
• I haven't carefully read the standard to see if this logic is correct but I'm confident others will be able to confirm/deny.

[JakobVogelsang]

I fuc...ing love it!! Great work from the first look and feel, it completely fits to the over all look and feel in OpenSCD, it is clear and intuitive. From UI, there is nothing for me to complain. To your questions:

1. You initialized with false and this is why it is not disabled at the beginning.
2. Right hand would fit more to the wizard handling
3. This is how we use it in the wizards. But I would say outlined could be a solution.
4. I will have a look on those.

[danyill]

Apparently I can't request changes on my own PR!

I note I've somehow deleted some snapshots erroneously which I must fix.
I have posed one or two questions.

[danyill]

I have made (I think) some improvements to the user interface:

• The header says the number of unused datasets
• The button the user clicks indicates the number of unused datasets
• The datasets are ordered according to the identity string (i.e. sorted by IED first)
• Button disabling works properly and allowed logic simplification
• The checklist should be close to the item which means on the left, not the right. We don't want mistakes in deleting things so the visual distance between checklist and item should be minimised. It's either this or the container size must be reduced (although I've done that too and it still seems too big - I am not sure on the best approach with this). This does mean the select all is on the opposite side (in my mind that is OK, a slight discordance in the UI when you decide to do something "severe" is not a bad thing, it reminds one that with great power should come greater fear). I haven't considered consistency with other UI elements.

We must not have a clean function which instead of sweeping the house eats the house instead!

Question: Should we include the dataset description here in some way?

Also:

• Ids converted to classes
• I hadn't taken into account LogControl blocks, so I've added them. I wonder if I missed anything else.
• The selectors are (now, I hope) improved since a DataSet can be under an LN and more work was needed to ensure uniqueness.

The next thing will be some work on the tests.

[danyill]

I think I am mostly done but I have a few problems:

First, when I click to look at a dataset I lose allow selection within the editor when I return. I think it's probably a change in context around this but I'm not sure how to correct it.

Second, styling:

Should the button be on the left-hand side? Or the right-hand side?

Right hand would fit more to the wizard handling

The button styling doesn't look correct but I'm unsure how best to improve. I think there should be more margin.

This is how we use it in the wizards. But I would say outlined could be a solution.

I have added some padding because there was zero. Alas, despite a valiant effort, my CSS skills have not allowed me to move this button to the right 😠

Because I've moved the checklists to the left this doesn't feel so problematic but I am interested...

Other than that, I've added some tests and no doubt more could be done. I would be grateful for a review.

[JakobVogelsang]

Hi @Danyll, to be honest with you, I was satisfied with the first version you have pushed UI wise, although I am glad now that you have pushed yourself a little further. E.g that the amount of to be deleted DataSets are displayed on the remove button and other little things that make the feature close to production ready :)

The two question you have I could solve

1. Use this additional style attribute on the mwc-button ittself style="float:right; margin-bottom: 10px"
2. I tested the outlined property on the button and I like it a lot. Would look something like that
   
3. The responsive width of the whole thing does not work, but I don't know why. I will have a look with my review.

Thanks a lot for your effort :)

[danyill]

If you are 🤒 🤮 🙏 please not rise from your 🛌. I am most grateful for this 🥇 review and have strived to make improvements.

[JakobVogelsang]

This is already very good. So please don't take all the comments too seriously. The only thing I would ask you to change are the import statements.

Over all LGTM!! Thank you for your big effort!!!

[danyill]

OK, I'm gonna push the rebase. I have a backup branch up my sleeve in case this ends poorly but all tests pass I believe.

For the remaining cleanup tasks, would you prefer to:

• merge this to main and then add to it via separate PRs?
• leave this PR open and have me add PRs against this branch?

Thank you for reviewing this and providing guidance 👏. I hope to need fewer review comments for additional tasks 🥀

[JakobVogelsang]

OK, I'm gonna push the rebase. I have a backup branch up my sleeve in case this ends poorly but all tests pass I believe.

For the remaining cleanup tasks, would you prefer to:

* merge this to main and then add to it via separate PRs?

* leave this PR open and have me add PRs against this branch?

Thank you for reviewing this and providing guidance 👏. I hope to need fewer review comments for additional tasks 🥀

The first would be better. What you have has clearly an added value for users and the other PRs would be simpler that ways :)

[danyill]

I think I've dealt with all of these, although after the force push I can't see, perhaps one wasn't resolved...

[JakobVogelsang]

LGTM