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

New channel proposition #171

Merged
merged 13 commits into from Feb 27, 2020
Merged

New channel proposition #171

merged 13 commits into from Feb 27, 2020

Conversation

nkolba
Copy link
Contributor

@nkolba nkolba commented Feb 12, 2020

So this is an alternate proposition to where we are with "default" trying to synthesize as much of the feedback and possible. Goals of these changes are to:

  • eliminate the special behavior required for the "default" channel
  • make leaving channels possible
  • provide a backwards compatibility option for FDC3 1.0 broadcast behavior

The changes in a nutshell are:

  • addition of fd3.getCurrentChannel and fdc3.leaveCurrentChannel apis
  • elimination of the 'default' channel
  • addition of a 'global' system channel that can be used as a backwards compatibility option - since a desktop agent could choose to start all apps on 'global'

I've created an implementation here as well: https://github.com/nkolba/desktop-agent

@vmehtafds @nicholasdgoodman

Joining channels in FDC3 is intended to be a behavior initiated by the end user. For example: by color linking or apps being grouped in the same workspace. Most of the time, it is expected that apps will be joined to a channel by mechanisms outside of the app. Always, there SHOULD be a clear UX indicator of what channel an app is joined to.

### The 'global' Channel
The 'system' channels include a 'global' channel which serves as the backwards compatible layer with the 'send/broadcast context' behavior in FDC3 1.0. A desktop agent MAY choose to make membership in the 'global' channel the default state for apps on start up.
Copy link
Contributor

Choose a reason for hiding this comment

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

👏

Choose a reason for hiding this comment

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

... apps will be joined to a channel by mechanisms outside of the app.

What APIs would be used to do this? All proposed, .joinChannel(...), .leaveCurrentChannel(), and .getCurrentChannel() APIs are done from within the calling context; e.g. via the local fdc3: DesktopAgent reference.

If some external color assignment widget wanted to assign "App A" to channel "red", its not exactly sure how the proposed APIs facilitate this; i.e., there is no API provided modify channel assignment from "outside the app".

Whats missing is something like fdc3.getApp("App A").joinChannel("red")...

In lieu of addressing this in the API now, I suggest it might be easier to "say less" and remove the sentence "Most of the time..."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @nicholasgoodman - this raises an excellent point. The API defined here is for apps to interact with FDC3. This does not cover what APIs a desktop agent might use to create default behavior for apps wrt to channels. This omission is purposeful, since this kind of join functionality would be implemented with APIs internal to the desktop agent.

docs/api/api-spec.md Outdated Show resolved Hide resolved
Copy link
Contributor

@rikoe rikoe left a comment

Choose a reason for hiding this comment

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

I really like this suggestion:

  • There is an opposite operation to joinChannel now.
  • There is no longer a slightly confusing default channel, but instead an "off" state, plus a "global" state.
  • There is a backward compatible route for FDC3 1.0 by using joinChannel('global')

Well done @nkolba!

Copy link

@vmehtafds vmehtafds 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 overall this looks pretty good.
My understanding so far is that global is explicitly defining the behavior I was expecting default to do versus previous definition of default left it to the DesktopAgent to define that behavior (which lead to all the confusion).

I think it was enough to just change the language to say the behavior of default is what global is doing and not introduce a new global channel type but I am ok with that if it causes less confusion.

I think the main thing that was missing was leaveCurrentChannel which this includes !

Thanks @nkolba !

@@ -179,7 +179,7 @@ catch (err){
```ts
getSystemChannels() : Promise<Array<Channel>>;
```
Retrieves a list of the System channels available for the app to join.
Retrieves a list of the System channels available for the app to join. This should include the 'global' channel.

Choose a reason for hiding this comment

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

So this is the only difference between global and default ? Currently default is not included in System channels.

Update: I see below default was also supposed to be returned as part of 'system' channels (although OpenFin's current implementation doesnt do that).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vmehtafds The biggest difference between 'global' and 'default' is that you can leave 'global'.

Copy link
Contributor

Choose a reason for hiding this comment

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

@vmehtafds another reason for this is I think that the name default was part of the problem - the default behaviour is now not to be on a channel (unless you call joinChannel). The name global makes it clearer what this channel does, I think.

Choose a reason for hiding this comment

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

the default behaviour is now not to be on a channel

@rikoe: this is a long thread so i may have missed this somewhere, but how does this change not break implementors who were only utilizing DesktopAgent.addContextListener(...) / DesktopAgent.broadcast(...)?

I would question why any change is needed at all. If you never add a context listener, you are de-facto not in any channel, global or otherwise.

The new proposal seems to indicate that using the Channel join APIs are mandatory in order to utilize context; or am I missing something here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nicholasgoodman @rikoe Implementors wishing to maintain backwards compatibility with existing behavior with desktopAgent.addContextListener and desktopAgent.broadcast can choose to start apps on the global channel by default. The key differences from previous proposals are:

  • users can leave the global channel
  • there is no special behavior to implement for the global channel
  • naming is a lot clearer

Choose a reason for hiding this comment

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

I would think that maintaining compatibility with the 1.0 spec would imply that auto-joining the global channel would be a SHOULD instead of a MAY; but either way it seems acceptable to me.

Additionally, not sure what "leaving all channels" provides for that can't be handled within the existing add listener or listener callback code; but similarly, I'm not against it either.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nicholasdgoodman This is backwards compatible with the 1.0 spec. However, 1.0 was very open ended on this topic and many implementations currently work this way. If you're building an implementation today - especially if your platform leans heavily on color (or other) linking - you may not want apps connected by default. So, I'd rather keep this as a MAY.

The "leaving all channels" pattern is a convenience - but it can be a pretty big one. Apps may have multiple handlers set for context and they may be also broadcasting from multiple points. Also, if some controller for linking is operating on the app's behalf (for example, a preload with a titlebar & color selector) it can turn on/off linking without needing knowledge of how listeners and broadcasters are wired up.

docs/api/DesktopAgent.md Outdated Show resolved Hide resolved
docs/api/DesktopAgent.md Outdated Show resolved Hide resolved

#### Examples

An app gets the current context of the `global` as a fallback if no other context is set.

Choose a reason for hiding this comment

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

I am not sure what this means? Can you expand on what you mean by a fallback?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

some platforms may have a tiered approach, where there is a global context (maybe coming from a top-level search) and more localized contexts created with channels. In that case, if an app doesn't have a context coming from a specific channel it is joined to it may want to pull the global context.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am tempted to say this is slightly inconsistent with the rest of the behaviour, and therefore introduces a bit of confusion. I think channels should always work the same way, regardless of what context it is. If you are on no channel, no context will be recorded. If you want context to be recorded, join a channel.

In this particular case I would prefer if the spec doesn't mention this behaviour. Someone can still do this if they like, as the standard wouldn't explicitly forbade it, but I don't personally think it makes much sense.

Choose a reason for hiding this comment

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

I agree with @rikoe here. I feel we are kind of reintroducing a new special behavior for "global" with this wording.

The way I expect - If I join global channel and also add a explicit channel.contextListener for red channel, I am expecting to get messages from both. Your statement seems to suggest you will only receive red channel messages (not sure if that is what you meant) and I will fallback to global when I remove the listener for red channel.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rikoe , @vmehtafds - some clarifications:

  1. in the proposal, the "global" channel would NOT have any special behavior, it would work just like any other channel. The only thing special about it, would be that it is a reserved system channel name so that: platform owners could choose to start apps on global by default, app developers could choose to start their apps on global by default, app developers could choose to listen to and broadcast to the global channel, etc. These would all be opt-in behaviors.
  2. in the code example, yes, if you joined red and added a contextListener on global, you would get messages for both. I am not dictating only using global as fallback - that was just an example of what I think is a probable use-case (I could be wrong)

@rikoe I am not clear on the concerns (I don't see how this is going against channels always working the same) but I am fine changing this example to something else. Do you have any suggestions?

Copy link
Contributor

Choose a reason for hiding this comment

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

@nkolba I see what you mean now - adding an explicit listener to global. No other concerns with what you describe.

However, An app gets the current context of the global as a fallback if no other context is set. is confusing.

The language here indicates special fallback behaviour without having to set up extra explicit listeners, which is unexpected.

If you are joined to global, getCurrentContext should get you the context on global, if not, you will get nothing. I think it will be clearer if we just remove the word "fallback" everywhere.

Of course you can still directly retrieve the channel itself and listen/retrieve the context on it, but that is a separate workflow from the join channel one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rikoe . Yes, if you are joined to global, then fdc3.getCurrentContext only gets you context from global - nothing in the example contradicts that. The example is attempting to show both using fdc3.joinChannel and directly retrieving channels and setting explicit listeners. I'll try to make the language and the example clearer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Change
An app gets the current context of the global as a fallback if no other context is set.
to
An app can retrieve the context on the global channel by accessing it directly, regardless if it is currently "joined" to the global channel or not.
Or something like that...

If you want to make it even more clear:

// retrieve current fdc3 context
const context = await fdc3.getCurrentContext("fdc3.instrument")
// context is null, as not currently joined to a channel

const globalChannel = await fdc3.getSystemChannels.filter(c => c.id === "global")
const globalContext = await fdc3.getCurrentContext("fdc3.instrument")
// context is instrument AAPL on the global channel

fdc3.joinChannel('global')
const context = await fdc3.getCurrentContext('fdc3.instrument')
// top-level context is now instrument AAPL as well because we have joined the global channel

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rikoe just pushed some language cleanup

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rikoe updated example pushed


fdc3.leaveCurrentChannel();
await fdc3.leaveCurrentChannel();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this now asynchronous?

Choose a reason for hiding this comment

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

I suggested that based on the fact that joinChannel being asynchronous. It could be useful to know when the leave operation successfully completed (or not).

Copy link
Contributor

Choose a reason for hiding this comment

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

@vmehtafds @nkolba I don't understand why this requires it to be asynchronous. success/error reporting is not the same thing as long-running operations that need to be asynchronous.

If you call the method without an exception, it worked. If it throws an exception, it didn't. If leaving (or joining) a channel as implemented in anything more then O(1) time, the implementation is doing something very weird.

I think making this operation asynchronous adds unnecessary complexity and ambiguity, and is a mistake.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rikoe @vmehtafds . I'm in favor of keeping things simple.
Given that joinChannel is async - I was thinking symmetry and consistency (most other methods are async). However, there is definitely a good case to be made for keeping it sync.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is joinChannel async? 🤔 I would argue the same applies. I do agree that both join and leave should both be consistent though...

Arguably both join and leaving channels should be instant no-ops, but happy to go with the majority view here.

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 could see wanting to do this:

await fdc3.joinChannel('global');
fdc3.broadcast(context);

@mcleo-d
Copy link
Member

mcleo-d commented Feb 13, 2020

Hey @rikoe - Just testing I can tag @vmehtafds in the PR comment.

Feel free to ignore.

James.

const context = await globalChannel.getCurrentContext("fdc3.instrument");
```

An app wants to respond to all context changes, whether on a joined channel or the `global` channel.

Choose a reason for hiding this comment

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

I also don't understand what is sentence mean and what feature/behavior the below example demonstrates. Can you just expand a bit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vmehtafds . This example is showing an app listening to the global channel outside of whatever channel it may be joined to. This example is meant to illustrate context scopes - i.e. I may have a joined channel scope and a global scope. For example, in a multi-windowed application, I may have a top-level search bar that represents global scope and individual windows may be on their own individual context scopes, or joined to a channel-wide context scope. This may or may not be useful for you. I am happy to get other examples in addition to or instead of this one.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @vmehtafds here: An app wants to respond to all context changes, whether on a joined channel or the global channel.

This is very confusing to me.

  • If you are joined to the global channel, you will get its context changes.
  • If you are joined to the red channel you will get its context changes.
  • If you are joined to no channels you will get no context changes.

I think this idea of scoping etc. is unnecessary complexity at this stage and I don't believe it was something originally in the spec, or proposed before. We should just make channels mutually exclusive.

If you do want this type of behaviour (which I think is unnecessary), it should be done by passing an array of channel names to join, and further API support for figuring out how channels are scoped/which "set" of channels you are in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rikoe @vmehtafds
"Channel scoping" is not something proposing for the spec - its something that someone may (or may not) implement on top of it. This is meant to be an implementation example.

This example seems to be confusing and taking us down a rabbit hole. I'm happy to get other suggestions of how to illustrate the API.

Copy link
Contributor

@rikoe rikoe Feb 19, 2020

Choose a reason for hiding this comment

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

Change An app wants to respond to all context changes, whether on a joined channel or the global channel. to: An app wants to explicitly receive context events on the global channel, regardless of what channel it is currently joined to (if any). (Or something like that. Apart from that the example is fine.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rikoe @vmehtafds - just pushed some clarifications to the language on this example. Please keep the feedback coming!

// context is null, as not currently joined to a channel

const globalChannel = await fdc3.getSystemChannels.filter(c => c.id === "global")
const globalContext = await fdc3.getCurrentContext("fdc3.instrument")

Choose a reason for hiding this comment

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

I think you want await globalChannel.getCurrentContext("fdc3.instrument") here.

const context = await fdc3.getCurrentContext("fdc3.instrument")
// context is null, as not currently joined to a channel

const globalChannel = await fdc3.getSystemChannels.filter(c => c.id === "global")

Choose a reason for hiding this comment

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

What is main reason to have this getSystemChannels.filter example here in addition to getOrCreateChannel example above?
Should you even have to do the filter thing with explicit API to get a channel?

Copy link
Contributor

Choose a reason for hiding this comment

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

As per the APIs, getSystemChannels returns an array of the well-known system channels, e.g. red, global etc.

If it is a system channel, it is just a different way to get the same channel. I just did this to show the alternative.

@nkolba nkolba merged commit 9d0054e into finos:master Feb 27, 2020
@nkolba nkolba deleted the new-channel-prop branch February 27, 2020 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api FDC3 API Working Group cla-present
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants