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(plugins/Subscription): Create 'Subscription' plugin for GOOSE subscriptions #549

Merged
merged 33 commits into from
Mar 17, 2022

Conversation

Flurb
Copy link
Contributor

@Flurb Flurb commented Feb 21, 2022

Resolves #525

@danyill
Copy link
Collaborator

danyill commented Feb 22, 2022

This is exciting! 🎉

I'm not familiar with ABB IEDs. I thought I'd have a play anyway.

When there are many devices or transmit messages in the left pane, then it would be convenient if the right pane was "sticky" such that when the left-hand side is scrolled, and a new IED is selected then the right-hand side remains in view (the user doesn't have to scroll to the top on the RHS).

Otherwise, if I understand correctly, the user must constantly scroll up and down if working with IEDs at the bottom of the stack.

Here's an example file: ArcFlash_IEC61850_GOOSE_2-section.scd.zip

@Flurb
Copy link
Contributor Author

Flurb commented Feb 23, 2022

This is exciting! 🎉

I'm not familiar with ABB IEDs. I thought I'd have a play anyway.

When there are many devices or transmit messages in the left pane, then it would be convenient if the right pane was "sticky" such that when the left-hand side is scrolled, and a new IED is selected then the right-hand side remains in view (the user doesn't have to scroll to the top on the RHS).

Otherwise, if I understand correctly, the user must constantly scroll up and down if working with IEDs at the bottom of the stack.

Here's an example file: ArcFlash_IEC61850_GOOSE_2-section.scd.zip

Hi @danyill , good suggestion! I can take a quick look at that!

@Flurb
Copy link
Contributor Author

Flurb commented Feb 23, 2022

@danyill I implemented the scrollable lists, I would love to have your feedback!

@danyill
Copy link
Collaborator

danyill commented Feb 24, 2022

Scrollabe lists are the 💣 👍
That makes this feel very usable.

I'm not sure what's in scope for this feature. Should we be showing additional fields? e.g. for all dataset items which are subscribed? For SEL relays it is useful to know the intAddr parameter (and I think ABB too).

Copy link
Collaborator

@danyill danyill left a comment

Choose a reason for hiding this comment

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

I thought I should read the code and as I did I had a few minor comments, use as you wish 😉

src/editors/subscription/publisher-goose-list.ts Outdated Show resolved Hide resolved
src/translations/en.ts Outdated Show resolved Hide resolved
src/editors/Subscription.ts Show resolved Hide resolved
src/editors/subscription/subscriber-ied-list.ts Outdated Show resolved Hide resolved
src/editors/subscription/subscriber-ied-list.ts Outdated Show resolved Hide resolved
src/editors/subscription/subscriber-ied-list.ts Outdated Show resolved Hide resolved
@JakobVogelsang
Copy link
Collaborator

JakobVogelsang commented Feb 24, 2022

I have tried it out for the first time and have some UI remarks before diving into the code:

  1. The two lists are not so visible. Having them in a slightly different color like we do with the action-panes would be a helper already, I think
  2. The header for the source would sound better Publicher: GOOSE or GOOSE publisher. The right side of the sink I would add a of making it Subscriber IED of GOOSE: ....
  3. I also think the headers should not be clipped. If this has to be, you can clip at the beginning not at the end as the valuable information is at the end
  4. Using filtered-list could be a nice option to reduce complexity. You have to imagine that there are project that have 250+ IED in the file. There you see that you don't see.
  5. Maybe reuse the gooseIcon for the list-items that represent a GSEControl :)
  6. This plugin can be a default one from the beginning. When we add other subscriber plugins later, we can decide to change that. But for now!

@Flurb
Copy link
Contributor Author

Flurb commented Feb 24, 2022

I have tried it out for the first time and have some UI remarks before diving into the code:

  1. The two lists are not so visible. Having them in a slightly different color like we do with the action-panes would be a helper already, I think
  2. The header for the source would sound better Publicher: GOOSE or GOOSE publisher. The right side of the sink I would add a of making it Subscriber IED of GOOSE: ....
  3. I also think the headers should not be clipped. If this has to be, you can clip at the beginning not at the end as the valuable information is at the end
  4. Using filtered-list could be a nice option to reduce complexity. You have to imagine that there are project that have 250+ IED in the file. There you see that you don't see.
  5. Maybe reuse the gooseIcon for the list-items that represent a GSEControl :)
  6. This plugin can be a default one from the beginning. When we add other subscriber plugins later, we can decide to change that. But for now!

Point 4, I created an issue: #559. It takes some more work to finish it :)

@Flurb Flurb marked this pull request as draft February 25, 2022 09:06
@Sander3003
Copy link
Member

As @Flurb mentioned small improvements and bugs can be solved in this pull-request. Bigger improvements require a seperate issues. By doing so we have small improvements (more easy to review).

@Flurb Flurb marked this pull request as ready for review February 25, 2022 10:56
Copy link
Collaborator

@JakobVogelsang JakobVogelsang left a comment

Choose a reason for hiding this comment

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

The plugin is nice!! Thank you for that. I found a bug and would love to have the german translation added. Everything else are comments.

src/translations/de.ts Outdated Show resolved Hide resolved
src/translations/de.ts Outdated Show resolved Hide resolved
src/translations/de.ts Outdated Show resolved Hide resolved
src/translations/de.ts Outdated Show resolved Hide resolved
src/translations/de.ts Outdated Show resolved Hide resolved
src/editors/subscription/subscriber-ied-list.ts Outdated Show resolved Hide resolved
src/editors/subscription/subscriber-ied-list.ts Outdated Show resolved Hide resolved
src/foundation.ts Outdated Show resolved Hide resolved
src/foundation.ts Outdated Show resolved Hide resolved
src/foundation.ts Outdated Show resolved Hide resolved
@danyill
Copy link
Collaborator

danyill commented Mar 7, 2022

Thanks for this @Flurb this seems like a very important feature 🎉 🎺

Should I review this or #569?

Off the top of my head from a UI point of view

  • I would like to see a little edit option for each GSEControl so that I can look for further information from the same screen. That doesn't seem too difficult since the Edit GSEControl wizard I think already does this.
  • Perhaps that's sufficient, but I also wonder if we should the description field in a tooltip or beneath the GSEControl in the UI?
  • Should the scroll bars of the GOOSE Publisher and Subscriber be invisible/width zero?
  • How do we show partial subscriptions of a GOOSE dataset?

I'm quite interested in having a subscription plugin for SEL relays where the intAddr would need to be visible/settable for the dataset item which is subscribed.

@Flurb
Copy link
Contributor Author

Flurb commented Mar 7, 2022

Thanks for this @Flurb this seems like a very important feature 🎉 🎺

Should I review this or #569?

Off the top of my head from a UI point of view

  • I would like to see a little edit option for each GSEControl so that I can look for further information from the same screen. That doesn't seem too difficult since the Edit GSEControl wizard I think already does this.
  • Perhaps that's sufficient, but I also wonder if we should the description field in a tooltip or beneath the GSEControl in the UI?
  • Should the scroll bars of the GOOSE Publisher and Subscriber be invisible/width zero?
  • How do we show partial subscriptions of a GOOSE dataset?

I'm quite interested in having a subscription plugin for SEL relays where the intAddr would need to be visible/settable for the dataset item which is subscribed.

I think @JakobVogelsang already reviewed both, am I right Jakob?
For your other suggestions:

I would like to see a little edit option for each GSEControl so that I can look for further information from the same screen. That doesn't seem too difficult since the Edit GSEControl wizard I think already does this.

This was already the plan, I'm not sure if we already have an issue for that. Can you check this? :)

Perhaps that's sufficient, but I also wonder if we should the description field in a tooltip or beneath the GSEControl in the UI?

Good suggestion, create an issue for that :)

Should the scroll bars of the GOOSE Publisher and Subscriber be invisible/width zero?

You mean while still being scrollable? Why do you want that? Or maybe I'm not getting it :)

How do we show partial subscriptions of a GOOSE dataset?

This is already implemented, we have 3 lists on the right: Fully subscribed, partially subscribed and not subscribed. IEDs that are inside the not subscribed/partially subscribed can be fully subscribed. IEDs in the Fully subscribed section can be unsubscribed.

I hope this answers all your questions :)

@danyill
Copy link
Collaborator

danyill commented Mar 7, 2022

Thanks @Flurb I have created some issues. @JakobVogelsang asked me to review... but I may have missed the moment.

Anyway, from a style/user-facing perspective:

Should the scroll bars of the GOOSE Publisher and Subscriber be invisible/width zero?
You mean while still being scrollable? Why do you want that? Or maybe I'm not getting it :)

The height of the GOOSE publisher on Chrome is greater than the contained elements.

image

This gives us a scrollbar on the publisher where we don't need it and a scrollbar on the overall window which is too many scrollbars. They are also the default scrollbars which are intrusive. Perhaps not zero width but less width might be nice.

Probably a CSS change is required which would remove the scrollbars for this case.
Perhaps a calc function should be used to subtract the height of the mwc-top-app-bar-fixed element...

Even so, there is the case with more IEDs/GSEControls it will extend.

In this case:

  • the scrollbar appears to start partway done the publisher pane and abruptly so it doesn't "look right" to my (somewhat unkind) 👁️
  • Scrollbars as shown here are used in OpenSCD so this is consistent with the overall UI. In the wider world there is flamewars around scrollbars (do you need them, are they a good UI element etc.). Material Design suggests there should be scrollbars but the modern take is much more minimalist than the scrollbars we have here.

To be very concrete, what I was thinking of was something less visually intrusive, i.e.

    mwc-list::-webkit-scrollbar {
      width: 5px;
      height: 8px;
      background-color: var(--base3);
    }
    
    mwc-list::-webkit-scrollbar-thumb {
        background: var(--primary);
    }

This way the scrollbars blend into the selection at the top and for the subscribing pane (I may not have the colours exactly right).

image

I accept that I am a pedant and apologise and beg forgiveness 🙏

A reasonable person will simply respond to this feedback with, "You didn't do that in #568 so why should I?"

@danyill
Copy link
Collaborator

danyill commented Mar 7, 2022

How do we show partial subscriptions of a GOOSE dataset?

This is already implemented, we have 3 lists on the right: Fully subscribed, partially subscribed and not subscribed. IEDs that are inside the not subscribed/partially subscribed can be fully subscribed. IEDs in the Fully subscribed section can be unsubscribed.

I"m used to seeing subscriptions at the DO/FCDA level so I can see exactly what items in the dataset are subscribed to and how they are used in the device (the intAddr). Where GOOSE configurations are done on a per data attribute which has to map to an internal address, it's important to manipulate individual items.

Maybe we should be able to expand the partially and full and edit the individual items.

I'm not sure if this is in scope here.

@JakobVogelsang
Copy link
Collaborator

There will be an extra plugin for that, that will be similar but will allow you to connect single FCDA elements to one ExtRef. Combining all approaches people got used to with different vendors is too difficult. At least starting with it. When we see synergies later and think we can merge with one or the other together, we will do that.

@JakobVogelsang
Copy link
Collaborator

Thanks @Flurb I have created some issues. @JakobVogelsang asked me to review... but I may have missed the moment.

Anyway, from a style/user-facing perspective:

Should the scroll bars of the GOOSE Publisher and Subscriber be invisible/width zero?
You mean while still being scrollable? Why do you want that? Or maybe I'm not getting it :)

The height of the GOOSE publisher on Chrome is greater than the contained elements.

image

This gives us a scrollbar on the publisher where we don't need it and a scrollbar on the overall window which is too many scrollbars. They are also the default scrollbars which are intrusive. Perhaps not zero width but less width might be nice.

Probably a CSS change is required which would remove the scrollbars for this case. Perhaps a calc function should be used to subtract the height of the mwc-top-app-bar-fixed element...

Even so, there is the case with more IEDs/GSEControls it will extend.

In this case:

* the scrollbar appears to start partway done the publisher pane and abruptly so it doesn't "look right" to my (somewhat unkind) 👁️

* Scrollbars as shown here are used in OpenSCD so this is consistent with the overall UI. In the wider world there is flamewars around scrollbars (do you need them, are they a good UI element etc.). Material Design [suggests](https://material.io/components/menus#behavior) there should be scrollbars but the modern take is much more minimalist than the scrollbars we have here.
  
  * Browser support for CSS scrollbars is still weak https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Scrollbars
  * But neither do we (at least me) want IE 5.5 scrollbars!
  * And the [webkit option](https://developer.mozilla.org/en-US/docs/Web/CSS/::-webkit-scrollbar) is not on a standards track but is the only viable option

To be very concrete, what I was thinking of was something less visually intrusive, i.e.

    mwc-list::-webkit-scrollbar {
      width: 5px;
      height: 8px;
      background-color: var(--base3);
    }
    
    mwc-list::-webkit-scrollbar-thumb {
        background: var(--primary);
    }

This way the scrollbars blend into the selection at the top and for the subscribing pane (I may not have the colours exactly right).

image

I accept that I am a pedant and apologise and beg forgiveness 🙏

A reasonable person will simply respond to this feedback with, "You didn't do that in #568 so why should I?"

Thanks again for the very detailed review!!! As often, I think you are making a very good point. The scroll bares in windows are a pain. With iOS it is not a problem...don't know how it is with Linux based operating systems.

I also think this should be handled globally in the normalized.ts as you pointed out already, the look and feel is the same in all plugins up until now. I know I am saying this a lot lately, but this should be an extra issue with an extra PR. In the PR especially, the complexity is already to big to be handled properly. I will open an issue with your comments as a starter :)

Copy link
Collaborator

@danyill danyill left a comment

Choose a reason for hiding this comment

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

I reviewed it from a UI/functionality point of view. This is a super important feature which I expect to be looking at in detail in future, thanks! 🎉

Flurb and others added 2 commits March 17, 2022 02:03
* Added event handling

* Intermediate commit

* Intermediate commit

* Small variable fix

* Refactor

* Refactoring

* Create subscribe/unsubscribe actions

* Intermediate commit

* Intermediate commit

* Intermediate commit

* Added correct subscribing/unsubscribing

* Refactor

* Refactoring

* Added goose-message unit tests

* Added ied-element unit tests

* Added publisher goose list unit tests

* Remove console.log

* Remove serviceType

* Remove unused import

* Added add/clear icon toggle to ied-element

* Fixing bug

* Moving newGOOSESelectEvent dispatch

* Subscribe/Unsubscribe now work on both LN0 and LN

* Refactoring

* refactor: use complex actions to subscribe and unsubscribe

* fix(plugins/Subscription): Empty 'Inputs' element after unsubscribing GOOSE should be deleted

Co-authored-by: Jakob Vogelsang <jakob-vogelsang@posteo.de>
@Flurb Flurb dismissed JakobVogelsang’s stale review March 17, 2022 13:11

It's already solved

@Flurb Flurb merged commit 25fae6f into main Mar 17, 2022
@Flurb Flurb deleted the subscriber-plugin branch March 17, 2022 13:12
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.

Create 'Subscription' plugin for GOOSE subscriptions
5 participants