Conversation
✅ Deploy Preview for fdc3 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
Actually a single commit - the rest is from the release branch which we need to merge first |
|
|
kriswest
left a comment
kriswest
left a comment
There was a problem hiding this comment.
I don't think we should be changing the message schema to resolve this issue - there is already state in this class containing the user channel defs that should be used, rather than re-transmitting it with every event.
I want to spend a bit more time with the client code to confirm that we're not listening to this event elsewhere - I could have sworn we had this handled already.
| } | ||
| ] | ||
| }, | ||
| "userChannels": { |
There was a problem hiding this comment.
I don't think this should be here - the event should not have to carry all the user channel definitions, which are supposed to remain constant (there are no event s for changes in the user channel definitions which are supposed to remain fixed). These should already have been retrieved by the client code and hence this duplication is inefficient.
At most the definition of the channel being selected could be included - but even that should not be necessary.
Also this change to the message schema will need to be approved by the SWG, where a change to the implementation only can be approved by the maintainers.
Changed the base to point at that release branch (and filled in summary/issue) - if that base is merged first the base will automatically change itself to main. |
|
Would you prefer if I a) reverted the change to b) changed The advantage of this is that we wouldn't need to change the event message, but I do want to query the user channels in case they have changed, hence |
|
|
|
There is a local cache of the User Channels data User channels are not generally updated - there's no provision for it in either the API or the communication protocol (i.e. no events - you can only poll for the list) and no built-in way to update injected channel selectors in FDC3 for the web. But its not explicitly stated that they don't change, so someone could implement a feature in a DA that does it. Hence, we should (IMHO) support the edge-case, but not focus on it (by querying the channels every time). I've had a go at implementing that and pushed to your branch. Note that the tests needed a slight tweak after switching to an async handler for the event - I added a 100ms wait for the known channel case (n.b 0-1ms would probably have done the job). I added more for the unknown case due to the extra round trip. When testing I noted that:
|
|
|
|
|
kriswest
left a comment
kriswest
left a comment
There was a problem hiding this comment.
LGTM, will need another approver (or for me to not be the last push + reapprove)
The problem with this is the channel selector: unless you call getUserChannels() or you are changing to a new channel, then it will be displaying the old user channels. Thus, if the desktop agent adds channel 5, but you're on channel 2, you won't know about it. You'll have to restart the app to see the correct channels again, which is a workaround but not ideal. Although thinking about this it might be possible to do in the channel selector itself with a web socket, maybe.
nice, thanks
yes, great idea. |
Just retrieving the channel data from the the DA doesn't push it into the injected channel selectors - something extra would be needed for that. Also the channel being set shouldn't be the thing triggering such an update as by that point a channel has already been selected in an external channel selector (which wouldn't normally be used alongside an injected one). The user channel set changing has never been a thing in FDC3 which is why there are no events telling apps that the set of channels has changed. It is something we could do - but its really a new feature and we need to do more than we are here (e.g. an event from DA saying channels have changed). Hence, if we want to talk about doing it a different issue should be raised and discussed for 2.3 IMHO. |
The reason I ended up with my original commit (with the change to the message etc.) was because of this line: ... in which we are pushing potentially state data (this.userChannels) out to the injected channel selectors. Now, as I said in the previous comment, I think I can work around this by ignoring the user channel information and passing it separately with web sockets, but it still seems like we missed a step by not adding this to the incoming message or making sure it's correct before we send this out. Shall I raise another issue for this? |
I think you should. The description on the message schema for FDC3UserInterfaceChannels: Describes it as being used for initial set-up, not for handling changes to the set of channels. It's also being used to set the channel when the app calls As I said, the standard doesn't explicitly say that the set of User channels doesn't change, just that they "are created and named by the desktop agent". However, it also makes no provision for changes (I suspect because no one actually thought that they would get changed) and if we start doing that it should be done properly (i.e. discussed and approved by the SWG) and with a complete solution (e.g. include an event for app's to know that the set of channels has changed, create a message for the DA to notify the proxy - which would be shared with the aforementioned event, what's the behaviour if the selected channel ceases to exist etc.). The issue that this PR addresses is related but different and, IMHO, is just an implementation issue we can resolve through the maintainers, as channels being changed by the DA (with an external channel selector or similar feature) is an existing use case and easily solvable with the proposed code tweaks - it can go out in a 2.2.x NPM release. Hence, if raising an issue I'd call it something like "Support changes to the set of User Channels in the Desktop Agent API" and we can confirm (or refute) that it is thing you should be able to do as part of that. |
|
ok will do. I am baffled by the idea that no one has ever wanted to change the user channels. |
|
Would like to merge this but I need to merge v2.2.0-beta.3 PR first. |
They're simply a set of predefined colour channels for a user to select from, the use case has always been changing the selected channel rather than the set you select from. I'm aware of multiple products that will accept a configuration for the set (and of course the standard recommends a particular, fixed set of channel defs), but to my knowledge no one has expressed a use case or need to change them at runtime. Where you have the ability to save or share a workspace or layout of apps, with channel selections, a changeable set of channels presents a problem as the links between apps in the layout might be broken by a channel used in the layout not existing in a particular user's set-up. Hence, I've mostly seen people change the set via config applied to all users on start-up. I can envisage someone coming up with a use case for adding to the set of user channels for some specific purpose (although such use cases are often served by app channels - which are not administrated by the user but handled by the apps instead), but changing or removing channels I mostly see as creating challenges for little gain. |
kriswest
left a comment
kriswest
left a comment
There was a problem hiding this comment.
Still LGTM, @robmoffat does it test up ok on your end?
I mean, looks ok but I'm going to need to do another beta release to really test this out. |
|
Feel free to set another release branch up and I'll review for ya - perhaps we should change the package.yml workflow back to running on merge into main at the same time... That said, don't forget that you could just |
|
@novavi could you take a look and approve this please? |
|
Can someone from the maintainer team review this please? |
@julianna-ciq I know you guys will need this to work... |
mistryvinay
left a comment
mistryvinay
left a comment
There was a problem hiding this comment.
Approve these additions.
|
|
3 participants
Describe your change
DA proxy code needs to listen for channelChanged events from the DA and action them (if necessary) in order to handle channel changes performed outside of the window (e.g. in DAs that load apps into iframes and display their own channel selector, rather than an injected one, outside of the window).
Related Issue
resolves #1540
Contributor License Agreement
Review Checklist
DesktopAgent,Channel,PrivateChannel,Listener,Bridging)?JSDoc comments on interfaces and types should be matched to the main documentation in /docs
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
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
npm run build) run and the results checked in?Generated code will be found at
/src/api/BrowserTypes.tsand/or/src/bridging/BridgingTypes.tsBaseContextschema applied viaallOf(as it is in existing types)?titleanddescriptionprovided for all properties defined in the schema?npm run build) run and the results checked in?Generated code will be found at
/src/context/ContextTypes.ts