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(editors/later-binding): Provide later-binding subscriber-oriented view #1187

Conversation

danyill
Copy link
Collaborator

@danyill danyill commented Feb 25, 2023

Closes #1025

This PR provides a subscriber-oriented view and allows the user to use a new button in the header to swap between a publisher-oriented view (the default) and the subscriber-oriented view.

The state of this button is saved in local storage to improve user experience.

It also changes the iconography in all plugins to be consistent in using the link and link_off icons for subscriptions which are present or absent.
It changes the way titles are done in the later binding plugin to be (I hope) a little more intuitive.

It innovates slightly by incrementing the selection in the extref list window after a subscription. I think this is a great workflow enhancement. Others may view it as a crime. WDYT?

It fixes one or two (alleged) bugs along the way.

It adds caching for identifying the supervision instances as this appears to be a costly operation.

It's probably usable for initial feedback.

Things Done and Not Done

  • Fix buggy prefix handling in isSubscribed in subscription/later-binding/foundation.ts. This element is not necessary for a subscription AFAICT. Update relevant tests.

  • Check for loose imports

  • Update snapshots

  • Add the filter for both FCDA and ExtRefs to only those used.

  • Add disabled to FCDAs on the RHS in the subscriber later binding plugins if pXX attributes are present (similar to what we do existing in the fcda-list)

  • Update Subscriber Later Binding (GOOSE) plugin

  • Provide tests

  • Update icons for showing subscriptions in other subscription plugins

  • Generally replicate tests for extref-list for the new subscriber list.

  • Add tests for incrementing, and the change in button

  • SMVSubscriberLaterBinding.test.ts has an interesting failure, where the same FCDA is in two control blocks Daniel thinks issue is with findControlBlocks and incorrect logic in the unsubscribe function. Not really, I used these functions without due care. The problem was me.

  • Abstract out subscribe and unsubscribe into foundation functions for reuse by automation and move event handler into classes.

  • Check that titles are correct between the two extref lists.

  • Why are pXX bindings not working in existing plugin in my PR? Why is there no failing test. If only a snapshot test then add a test. They are working. They just don't work for partial matches.

  • XAT Prot2 Later Binding SMV list looks decidedly odd. But I forget why...

  • ExtRef must not be disabled in subscriber view.

  • Should the auto-increment have a setting cog adjacent to the filter to disable it?

  • If you open the same file twice are the ExtRefs updated? Yes, but the FCDA counts were not.

  • Modify filter as per feat(editors/binding): Allow filtering of subscribed/unsubscribed data in binding editors #1149

  • Add settings option for auto-increment

  • Consider moving common functionality between ext-ref-later-binding-list-subscriber.ts and ext-ref-later-binding-list.ts into foundation (?). My new foundation functions don't feel terribly smooth but I have built on what came before. Perhaps it should be scoped more tightly before FCDAs are processed.

  • Gain user feedback

  • Try to use something like inline flex to put two side-by-side elements for ExtRef/FCDA in the new subscriber list. Decided not to, instead allowed user to resize the list, but happy to take some reviewer assistance. My CSS patience has been exhausted for this PR.

  • Remove the console logs and try that awful ternary operator again in each later binding plugin (which didn't work for mysterious reasons). Have tried, can't figure out why it doesn't work. I will award 1 🍺 to someone who can explain this to me.

  • Can we test local storage? I guess we can, I didn't. But neither did the Substation plugin, so my guilty is not very great.

  • Check for disabled items working correctly as it does now.

  • Offer to the reviewer to go through online to explain choices given monolithic PR.

  • File issue relating to alternative ways to show publisher/subscriber

  • When we moved from Replace to Update I suspect some attributes weren't adequately transferred in my existing code, but we have no test, for example, for pXX attributes. At least checking that a subscription retains these would be nice.

  • Highlight to the reviewer possible deficiency in design which uses Update vs. Replace actions and why this was done.

@danyill danyill marked this pull request as draft February 25, 2023 03:58
@danyill danyill changed the title feat(editors/later-binding): Provider later-binding subscriber-oriented view feat(editors/later-binding): Provide later-binding subscriber-oriented view Feb 26, 2023
@danyill danyill force-pushed the issue-1025-later-binding-subscriber-view-consolidated branch from b679f9f to fb3eb30 Compare March 12, 2023 18:57
@danyill danyill marked this pull request as ready for review March 19, 2023 08:26
@danyill
Copy link
Collaborator Author

danyill commented Mar 19, 2023

This is now substantially complete.

  1. It would be nice to try and do a better job on the CSS for showing the ExtRef and the subscription as separate visual elements. My current resolution is that the user can resize the ExtRef section as required to deal with the spacing issues we anticipated. However, I will be happy to take some suggestions on how to move forward. Christian suggested that Juan might be able to help here.

  2. I will be very pleased to receive a review and would be happy to meet and go through some of the design decisions if that is desirable, as this is quite a large PR.

  3. There is no particular hurry for this review -- I'm working with a local deployment, and no future work directly depends on this. I hope to get some internal feedback on this plugin soon.

  4. Curiously, in each of the LaterBinding plugin files, if a ternary operator is used instead of the if statement, it appears to not correctly behave between views (especially in event handling?), whereas a normal if statement works fine. I am surprised and confused by this.

  5. Some tests were updated during this PR in the isSubscribed function, which I think required a prefix to exist, which I believe was incorrect, so I fixed it in passing.

  6. A couple of tests are failing, but these are unrelated to this work and may be related to the recent migration to Node 18 (failures don't occur locally on my machine).

@jarradraumati
Copy link
Contributor

It would be better if the ExtRefs were ordered as per the XML file rather than listed alphabetically.

@jarradraumati
Copy link
Contributor

GOOSE and SV should not appear in the ExtRef list if pServT is declared (and serviceType is not).

@jarradraumati
Copy link
Contributor

When I map a SMV input and it increments to the next DA, the "Publisher Sampled Value Messages" view doesn't update for the disabled items (see screenshot):

image

@danyill danyill marked this pull request as draft March 20, 2023 18:30
@danyill
Copy link
Collaborator Author

danyill commented Mar 20, 2023

Talking to Christian we have concluded that some further refactoring of the subscriber plugins is required due to the "breaking of the ternary operator" at which stage something has horribly and obtrusively gone wrong...

@danyill
Copy link
Collaborator Author

danyill commented Mar 20, 2023

It would be better if the ExtRefs were ordered as per the XML file rather than listed alphabetically.

Done, perhaps in the fullness of time we can have a sort option here.

GOOSE and SV should not appear in the ExtRef list if pServT is declared (and serviceType is not).

I think done, but may need to add some tests for this.

When I map a SMV input and it increments to the next DA, the "Publisher Sampled Value Messages" view doesn't update for the disabled items (see screenshot):

Done. May also need to add some tests for this

@danyill
Copy link
Collaborator Author

danyill commented Mar 23, 2023

While this PR was mostly finished with tests and awaiting review, Christian has convinced me that for technical reasons and also because we will eventually need to make this a core plugin, we should take a different approach to the structure of this plugin.

So for now, further development will be in https://github.com/danyill/oscd-subscriber-later-binding, and we expect to re-home this within the openscd organisation in the future.

(Technical reasons = especially the use of multiple components for the ExtRef and FCDA list, which is something of an anti-pattern)

@danyill danyill closed this Mar 23, 2023
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.

Provide subscriber-oriented view of later-binding plugins
2 participants