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

Adding .NET docs for Events to API reference #1441

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

kemerava
Copy link
Contributor

@kemerava kemerava commented Nov 15, 2024

Describe your change

Adding .NET docs for Events to API reference
@bingenito hope I did not steal your issue! :) I really wanted to try to give it a go, could you please review it, as I was not sure if there are better ways of implementing it in some places.

Related Issue

resolves #1318

Contributor License Agreement

  • I acknowledge that a contributor license agreement is required and that I have one in place or will seek to put one in place ASAP.

Review Checklist

  • Issue: If a change was made to the FDC3 Standard, was an issue linked above?
  • CHANGELOG: Is a CHANGELOG.md entry included?
  • API changes: Does this PR include changes to any of the FDC3 APIs (DesktopAgent, Channel, PrivateChannel, Listener, Bridging)?
    • Docs & Sources: If yes, were both documentation (/docs) and sources updated?

      JSDoc comments on interfaces and types should be matched to the main documentation in /docs
    • Conformance tests: If yes, are conformance test definitions (/toolbox/fdc3-conformance) still correct and complete?

      Conformance test definitions should cover all required aspects of an FDC3 Desktop Agent implementation, which are usually marked with a MUST keyword, and optional features (SHOULD or MAY) where the format of those features is defined
    • Schemas: If yes, were changes applied to the Bridging and FDC3 for Web protocol schemas?

      The Web Connection protocol and Desktop Agent Communication Protocol schemas must be able to support all necessary aspects of the Desktop Agent API, while Bridging must support those aspects necessary for Desktop Agents to communicate with each other
      • If yes, was code generation (npm run build) run and the results checked in?

        Generated code will be found at /src/api/BrowserTypes.ts and/or /src/bridging/BridgingTypes.ts
  • Context types: Were new Context type schemas created or modified in this PR?
    • Were the field type conventions adhered to?
    • Was the BaseContext schema applied via allOf (as it is in existing types)?
    • Was a title and description provided for all properties defined in the schema?
    • Was at least one example provided?
    • Was code generation (npm run build) run and the results checked in?

      Generated code will be found at /src/context/ContextTypes.ts
  • Intents: Were new Intents created in this PR?

@kemerava kemerava requested a review from a team as a code owner November 15, 2024 23:21
Copy link

netlify bot commented Nov 15, 2024

Deploy Preview for fdc3 ready!

Name Link
🔨 Latest commit 6c06bb3
🔍 Latest deploy log https://app.netlify.com/sites/fdc3/deploys/67a515d1bf2b4f0008798b2e
😎 Deploy Preview https://deploy-preview-1441--fdc3.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

docs/api/ref/Events.md Outdated Show resolved Hide resolved
docs/api/ref/Events.md Outdated Show resolved Hide resolved
docs/api/ref/Events.md Outdated Show resolved Hide resolved
docs/api/ref/Events.md Outdated Show resolved Hide resolved
docs/api/ref/Events.md Outdated Show resolved Hide resolved
docs/api/ref/Events.md Outdated Show resolved Hide resolved
docs/api/ref/Events.md Outdated Show resolved Hide resolved
docs/api/ref/Events.md Outdated Show resolved Hide resolved
docs/api/ref/Events.md Outdated Show resolved Hide resolved
docs/api/ref/Events.md Outdated Show resolved Hide resolved
docs/api/ref/Events.md Outdated Show resolved Hide resolved
docs/api/ref/Events.md Outdated Show resolved Hide resolved
bingenito
bingenito previously approved these changes Nov 29, 2024
@kemerava
Copy link
Contributor Author

kemerava commented Dec 3, 2024

Sorry @bingenito, had to resolve the merge conflicts, could please re-review?

bingenito
bingenito previously approved these changes Dec 3, 2024
Copy link
Contributor

@kriswest kriswest left a comment

Choose a reason for hiding this comment

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

The tab selection headers are not appearing on Events.md, e.g.:
image
https://deploy-preview-1441--fdc3.netlify.app/docs/next/api/ref/Events
vs.

image
https://deploy-preview-1441--fdc3.netlify.app/docs/next/api/ref/DesktopAgent

That's probably because that page needs the following to be added at the top, right after the metadata block:

import Tabs from '@theme/Tabs';
import TabItem from '@theme/TabItem';

See DesktopAgent.md for an example.

Regarding the definitions contained in the PR, all looks good to me, but I wonder if the Details class might need a better-scoped name such as EventDetails or Fdc3EventDetails. There are already two different sub-classes (both called Details currently) for the channel changed event and Private Channel addContextListener/unsubscribe events, should they be better namespaced? Happy to defer to you and @bingenito on that, but thought to ask.

CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Kris West <[email protected]>
@kemerava
Copy link
Contributor Author

kemerava commented Dec 4, 2024

Thanks for the review @kriswest! Fixed the imports. I like the EventDetails idea, however, would love to hear also what @bingenito thinks! Thanks both

@bingenito
Copy link
Member

I favor Fdc3EventDetails but this is also a placeholder. I've started the implementation of this (copying these as the definition) and will work out where it makes sense to change once we write tests against it and get a feel for naming. We can then circle back and update the docs to match?

@kemerava
Copy link
Contributor Author

kemerava commented Dec 4, 2024

Thanks, @bingenito. Works for me, do you want to keep this open until then, or should we merge as is and it will be updated later?

@bingenito
Copy link
Member

bingenito commented Dec 4, 2024

@kemerava let's hold off a bit. As I'm implementing this I'm thinking of mirroring context types and having IApiEvent<T> where T is the type for the Details property. I'll create a draft PR over in fdc3-dotnet in the next few days and we can discuss there.

bingenito
bingenito previously approved these changes Dec 4, 2024
@bingenito
Copy link
Member

@kemerava I worked on and off over break for weeks on this and can't get it to work intuitively using generics. The design simply just doesn't fit into .NET that well and my current implementation is a back to a greatly simplified IFdc3Event with string type and object? Details. IApiEvent is abandoned right now for me as there would be no difference between the two interfaces given .NET can not restrict on a string enum like we see in TypeScript.

@bingenito
Copy link
Member

Draft fdc3-dotnet implementation can be found at finos/fdc3-dotnet#170. As noted above this is significant simplification of design to simply have a nullable object in the base type. As we can't have restriction on enum string values it also didn't make sense to have separate ApiEvent and Fdc3Event definitions. Let's have the discussion over there and then we can circle back here.

@kemerava
Copy link
Contributor Author

@bingenito @kriswest updated the code based on the .NET PR, please review - thanks!

docs/api/ref/Events.md Outdated Show resolved Hide resolved
docs/api/ref/Events.md Outdated Show resolved Hide resolved
bingenito
bingenito previously approved these changes Feb 3, 2025
Copy link
Contributor

@kriswest kriswest left a comment

Choose a reason for hiding this comment

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

I think we should explain the lack of an ApiEvent definition in .NET - see inline comments

<TabItem value="dotnet" label=".NET">

```
Not implemented
Copy link
Contributor

Choose a reason for hiding this comment

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

@bingenito @kemerava I found this confusing initially... It exists in TypeScript to provide a common ancestor for the FDC3 API and PrivateChannel events allowing a common event handler type to be used across them but separate enums for the event types - which I believe you can't replicate in C# (but haven't taken the time to understand yet ;-) ).

I realise now that the naming of said handler type is inelegant (FDC3EventHandler is based on ApiEvent which has two subtypes FDC3Event and PrivateChannelEvent), I suspect we'll need to live with that...

From what I see in the fdc3-dotnet PR the private channel events extend FDC3Event. Perhaps it would make sense to document IFdc3Event event here instead, otherwise include an explanation of the difference here to forestall a future reader's confusion?

Copy link
Contributor

Choose a reason for hiding this comment

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

P.S. I'd be open to changing the section heading from ApiEvent to something that would better represent the differing definitions.

Copy link
Member

@bingenito bingenito Feb 6, 2025

Choose a reason for hiding this comment

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

@bingenito @kemerava I found this confusing initially... It exists in TypeScript to provide a common ancestor for the FDC3 API and PrivateChannel events allowing a common event handler type to be used across them but separate enums for the event types - which I believe you can't replicate in C# (but haven't taken the time to understand yet ;-) ).

If you want consumer to use ApiEvent across all cases then that base class would need a common type which can only be string in this case (not an actual enum in C#). As such, it would have been the same exact definition for Fdc3Event or Fdc3Event would be empty and redundant. I felt that ApiEvent can always be added in based on demand, but later removing would be difficult.

From what I see in the fdc3-dotnet PR the private channel events extend FDC3Event. Perhaps it would make sense to document IFdc3Event event here instead, otherwise include an explanation of the difference here to forestall a future reader's confusion?

From a dev standpoint, the events are providing IFdc3Event. The implementations for private channel are helpers and can therefore be concrete implementations. If it were just implementing IFdc3Event instead of Fdc3Event we would be duplicating all code into each of them. Note that Fdc3Event implements IFdc3Event so in turn the concrete events also implement and can be cast to IFdc3Event.

Copy link
Member

Choose a reason for hiding this comment

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

Note that the .NET implementation is ignoring typical C#/.NET design for the sake of consistency of FDC3 across each platform. Ideally, in fdc3-dotnet, there would be typed event handlers for every one of these events using the += syntax such as desktopAgent.ChannelChanged += new ChannelChangedEventHandler....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added the explanation on the ApiEvent, @kriswest @bingenito maybe that would work for the explanation side of things?

@kemerava kemerava requested a review from kriswest February 6, 2025 21: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.

Add .NET docs for Events to API reference
3 participants