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

Context Clearing #1197

Open
Tracked by #1215
hellosmithy opened this issue Apr 25, 2024 · 12 comments · May be fixed by #1379
Open
Tracked by #1215

Context Clearing #1197

hellosmithy opened this issue Apr 25, 2024 · 12 comments · May be fixed by #1379
Labels
enhancement New feature or request good first issue Good for newcomers needs-pr

Comments

@hellosmithy
Copy link

hellosmithy commented Apr 25, 2024

Enhancement Request

NOTE: Edited to reflect discussion from Standard WG Meeting - May 23rd, 2024 #1215

Use Case:

Enable explicit clearing of individual contexts including within multi-context channels. For instance, removing a filter from a data grid (e.g., by instrument, trade, or time range) while keeping other context filters intact.

Current Limitations:

  • fdc3.nothing: Limited to clearing all contexts on a channel, lacks the ability to target specific contexts.
  • broadcast context with null id field: Requires creating empty versions of each context type, which is cumbersome and inconsistent due to some fields being non-nullable.

Additional Information

The issue of context clearing was most recently discussed at the #1215 Standard WG Meeting and several possible solutions were floated.

  • Extend fdc3.nothing to include an optional subType field to indicate a specific context
  • Addition of a clearContext channel method
  • Addition of a string constant to id fields to indicate a cleared context
    • Not pursued further in this proposal due to not having clear semantics, and also not resolving the issue of contexts having different required fields, and therefore not having a canonical empty context type.
  • Redesign of the broadcast API to support null contexts, e.g. by adding an additional type param or by adding the type to the existing metadata param
    • Whilst this might be the cleanest solution, not pursued further in this proposal due to being the least backwards compatible option. But if there is appetite for broader changes it may be worth considering.

No decision was made, but there was general consensus that any solution would need to address both requirements of:

  • Updating any internal currentContext state in the desktop agent
  • Broadcasting the cleared context to any listeners
    • Accounting for both * and individual context listeners

Proposed Solution

A combination of the first two options would address both the above requirements.

Extend fdc3.nothing context

interface Nothing {
  type: 'fdc3.nothing';
  subType?: string;
}
  • If a subType is not provided it represents a lack of context as per the existing behaviour.
  • If a subType is provided it indicates the lack of the specified context.

E.g.

{
  type: 'fdc3.nothing',
  subType: 'fdc3.timeRange'
}

The above indicates the lack of a time range context

Addition of a Channel.clearContext method

interface Channel {
  ...
  broadcast(context: Context): Promise<void>;  
  getCurrentContext(contextType?: string): Promise<Context|null>;
  clearContext(contextType?: string): Promise<void>;
  addContextListener(contextType: string | null, handler: ContextHandler): Promise<Listener>;
}

Functionality:

  • Clear the specified context type if provided, or all contexts if no type is specified
    • Internal context is updated by the desktop agent
    • Any subsequent joiners to the channel or calls to getCurrentContext will receive a null value for the cleared context type
  • Update all channel listeners that are subscribed to the fdc3.nothing context type
    • If a single context is being cleared then a subType is provided
    • If all contexts are being cleared then subType is omitted

NOTE: Any direct listeners to the cleared subType would not receive any updates as the existing type contract expects that they would only get updates of the context being cleared. As such any consuming application would need to explicitly opt-in to context clearing updates with a separate listener of fdc3.nothing contexts.

E.g.

channel.addContextListener('fdc3.timeRange', (context: Fdc3TimeRange) => {
  // ... update app state
})

channel.addContextListener('fdc3.nothing', (context: Fdc3Nothing) => {
  switch (context.subType) {
    case 'fdc3.timeRange':
      // ... update app state
    default:
      // ... clear all contexts
  }
}

Rationale for Hybrid Approach

  • Internal State Management: The clearContext method directly updates the FDC3 internal context state in a declarative way that matches existing methods.
  • Backward Compatibility: Broadcasting fdc3.nothing with a subType allows context listeners to opt-in to receive specific contexts that have been cleared, without requiring major breaking changes to existing context listeners

Some trade-offs:

  • Ergonomics and Clarity: This solution is not as a discoverable or intuitive as an API redesign might be, but errs on the side of backwards compatibility.
  • API Changes for Desktop Agent Providers whilst backwards compatibility is maintained for consumers, providers would need to make implementation changes to implement the internal context state clearing and broadcasting behaviour

Some edge cases:

  • What happens if an fdc3.nothing with subType is broadcast directly rather than triggered by a call to clearContext? Would the internal context still be cleared?
@hellosmithy hellosmithy added the enhancement New feature or request label Apr 25, 2024
@kriswest
Copy link
Contributor

@hellosmithy Many thanks for the detailed write up. This was previously considered under issue #301, and has subsequently been raised as a question in a meeting at least once. Hence, I believe this is worth revisiting and I propose to do so at the May SWG meeting.

@hellosmithy
Copy link
Author

I've updated the above proposal to reflect the discussion from the last WG

@kriswest
Copy link
Contributor

kriswest commented Jun 4, 2024

Many thanks @hellosmithy!

@kriswest
Copy link
Contributor

@hellosmithy one thing I would update in the write up:

Functionality:

  • Clear the specified context type if provided, or all contexts if no type is specified
    • Internal context is updated by the desktop agent
      - Any subsequent joiners to the channel or calls to getCurrentContext will receive a null value for the cleared context type
  • Update all channel listeners that are subscribed to the fdc3.nothing context type
    • If a single context is being cleared then a subType is provided
    • If all contexts are being cleared then subType is omitted

They wouldn't receive a null context - rather they would:

  • if working through the desktop agent API (fdc3.joinChannel, fdc3.addContextListener): NOT receive the specified contexts on joining or adding a context listener - these are sent automatically if present and would no longer be present
  • if working through the Channel interface: NOT receive the specified contexts when calling theChannel.getCurrentContext - you don't receive context automatically on adding a listener via the channel interface - confusing I know!

From there this needs a PR to implement it that will touch:

I'd be most happy for you to take that on ;-) although we'll need your CLA in place to merge it. Ideally, it gets done before next months SWG meeting so we can get it adopted into the 2.2 scope. If the CLa is likeloy to get in the way of that timeline, let me know and I'll see if I can help get the PR done!

@kriswest kriswest added the good first issue Good for newcomers label Jul 25, 2024
@robmoffat
Copy link
Member

I really like the idea of the clearContext method. I'm less sure about the fdc3.nothing approach, it seems like it is a bit confusing, but I get how this could work right now rather than waiting for a new version of FDC3 to come out.

Backward Compatibility: Broadcasting fdc3.nothing with a subType allows context listeners to opt-in to receive specific contexts that have been cleared, without requiring major breaking changes to existing context listeners

So one thing that isn't really clear then is:

  • is broadcasting fdc3.nothing with no subtype clearing all context?
  • would it mean something to raise an intent with fdc3.nothing with a subtype?

Perhaps this would be better in a specific, new context type, say fdc3.clear?

@kriswest
Copy link
Contributor

@robmoffat by 'the fdc3.nothing approach' do you mean manual broadcast of that triggering a clear? If so I think that very much optional and not something we need to do.

However, if you mean the DA broadcasting fdc3.nothing to indicate that a clear has happened, then I do think we need a form of notification and fdc3.nothing works pretty well for it. Its got few other uses (raising intents without a context) that seem semantically compatible as its just supposed to represent a lack of context.

@hellosmithy
Copy link
Author

hellosmithy commented Nov 8, 2024

I really like the idea of the clearContext method. I'm less sure about the fdc3.nothing approach, it seems like it is a bit confusing, but I get how this could work right now rather than waiting for a new version of FDC3 to come out.

@robmoffat @kriswest after a long wait for CLA and looking at this with fresh eyes I also wonder if the adding of a subType to fdc3.nothing causes some ambiguity to behaviour.

If we went down the explicit clearContext(contextType?: string): Promise<void>; we'd still need to figure out what happens to any context listeners though.

E.g.

const allContextsListener = await channel.addContextListener(null, ...);
const timeRangeListener = await channel.addContextListener('fdc3.timeRange', ...);

await channel.clearContext('fdc3.timeRange');

What value do the two handlers for allContextsListener and timeRangeListener get in this case?

The type ContextHandler = (context: Context, metadata?: ContextMetadata) => void; will need to be expanded somehow to allow for the information about what has been cleared to be added somewhere.

In the case of allContextsListener a non-breaking change could be to broadcast trp.nothing (or a breaking alternative would be Context | null) and add information on what was cleared to ContextMetadata.

But it might be strange to broadcast a trp.nothing to a handler explicitly listening on another context type as in timeRangeListener. Would Context | null be better in that case? (although a breaking change).

I suppose another option might be to change the handler to something like:
type ContextHandler = (contextType: string, context: Context | null, metadata?: ContextMetadata) => void;

@robmoffat
Copy link
Member

@hellosmithy these are excellent questions. Good analysis.

@kriswest
Copy link
Contributor

kriswest commented Nov 8, 2024

Thanks @hellosmithy. I'm not sure there is a perfect or elegant answer here unfortunately.

Re:

type ContextHandler = (contextType: string, context: Context | null, metadata?: ContextMetadata) => void;

You only need the contextType set when context is null and it is (as you say) breaking for all existing app implementations - we'd have to go to an FDC3 3.0 to do it.

With fdc3.nothing we have a potentially awkward situation if an app decides to broadcast it manually for some reason.

An alternative could be to notify apps of the cleared context via the new addEventListener instead... Event objects can provide data fields so the event could tell you what context was cleared. You still need to add an additional handler (as you would if listening for fdc3.nothing), but it will at least only fire if clearContext is called.

(including from @kemerava who has been considering this issue as well)

@kemerava
Copy link
Contributor

kemerava commented Nov 8, 2024

Thanks @hellosmithy.

I would probably vote for not introducing a breaking change here. I think having an event listener like @kriswest suggested can be helpful in this case. We can even introduce a new type like addContextClearingListener to show that is a significant part of functionality. Outside of that, I think documenting that it can happen will allow the apps to decide what to do if they get that type in the handler instead of the type they were waiting for

@hellosmithy
Copy link
Author

Thanks @kriswest @kemerava - I'm not too familiar yet with the semantics of the new addEventListener. Can this be connected to specific channel? The examples in the docs seems to be more for general system-wide events. Whereas I think clearing a context might be associated more with a specific channel.

@kriswest
Copy link
Contributor

Good point @hellosmithy. addEventListener would more naturally associate with the current user channel, unless it came with data in the event pointing tothe channel. This is quite disconnected from the Channel object and hence inelegant/unintuitive.

Another approach to think about is the use of the optional ContextMetadata object passed to the ContextHandler See https://fdc3.finos.org/docs/api/ref/Types#contexthandler.
But again we'd need to fire the handler with a null context (which is a breaking change) and use ContextMetadata to clarify the type cleared.

Which brings me back to the use of fdc3.nothing to indicate a cleared context. That at least offers an opt-in (you need to be listening to all types or specifically the fdc3.nothing type to received the clear message) and is non-breaking (we're only delivering context messages and not changing any API types).

The only inelegant part of that proposal is that someone could manually broadcast an fdc3.nothing. To handle that we could require that DA either reject the broadcast of fdc3.nothing or interpret it as a clearContext call...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers needs-pr
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants