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

GPII-4218: PSPChannel read API cannot read UIO+ setting #877

Merged
merged 14 commits into from
Jun 30, 2020

Conversation

klown
Copy link
Member

@klown klown commented Jun 5, 2020

This pull request addresses two issues. First, it solves the original problem where the PSPChannel read API could not read a UIO+ setting. Secondly, it adds the ability for the BrowserChannel/WebSocketsSettingsHandler to write UIO+ settings, allowing the UIO+ chrome extension for morphic to send and receive messages from a running Morphic instance.

PSPChannel read API

The inverseCapabilitiesTransformations block in the solutions registry for UIO+ was an empty disallowing any transformation from settings to preferences. That meant any pspChannel request for a UIO+ preference was bound to fail. All but one of the inverse transformations could be automatically generated by gpii.getInverseRules(), meaning that no inverse transformations would be used. The problematic transform is for the contrastTheme setting. There was a inversion specified in an earlier commit on this branch, but that resulted in a solutions registry validation error when compared with the associated capabillitiesTransform, and so it was removed. Also, it looks like some other settings handler than that used by UIO+ handles contrast themes. UIO+'s settings handler is gpii.settingsHandlers.webSockets.component.

Possible ways forward:

  • figure out how to set up the contrastTheme transformation and its inverse so the registry validates,
  • remove contrastTheme from the inverted transformations since it is not invertible
  • remove it and let some other settings handler deal with it.

UIO+ Extension for Morphic changeSettings Requests Using the BrowserChannel

The steps suggested in the JIRA were implemented, modifying the BrowserChannel and WebSocketsSettingsHandler:

  • the BrowserChannel's receiveMessage handler was modified to accept changeSettings message types,
  • the WebSocketsSettingsHandler:
    • addClient() was modified to attach a listener for the changeSettings message type to BrowserChannel clients,
    • notifySettings() was modified to send a settingsChangedReceived message back to the source of the changeSettings message, and an onSettingsChanged message to all other interested clients.

JIRA: https://issues.gpii.net/browse/GPII-4218

klown added 4 commits June 5, 2020 15:36
Also tweaked the conole messagee for the PSP client and Browser
client examples.
…heme'

Removed because it does not validate:
"ERROR: The capabilities transforms for win32.json5 -> net.gpii.uioPlus -> contrastTheme may result in data corruption."

Also, it might actually be handled by another settings handler.
@gpii-bot
Copy link

gpii-bot commented Jun 5, 2020

CI job passed: https://ci.gpii.net/job/universal-tests/2349/

@gpii-bot
Copy link

CI job passed: https://ci.gpii.net/job/universal-tests/2368/

When a browser channel client sends a request to change settings, instead
of silence, they receive a "changeSettingsReceived" message type as a response.
All other clients are sent the "onSettingsChanged" message type, as before.
@gpii-bot
Copy link

CI job passed: https://ci.gpii.net/job/universal-tests/2369/

@gpii-bot
Copy link

CI job passed: https://ci.gpii.net/job/universal-tests/2372/

@gpii-bot
Copy link

CI job passed: https://ci.gpii.net/job/universal-tests/2373/

@klown
Copy link
Member Author

klown commented Jun 16, 2020

Ready for review @jobara @amb26

@@ -32614,7 +32614,43 @@
}
}
},
"inverseCapabilitiesTransformations": {}
"inverseCapabilitiesTransformations": {
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like there isn't a high contrast inverse transformation. From what I recall you had some issues with that one. I think you can do the inverse transform by mapping "default" to "http://registry\\.gpii\\.net/common/highContrast/enabled" with value set to false. For rest you'd follow the theme mapping e.g. "bw" maps to "http://registry\\.gpii\\.net/common/highContrastTheme" with the value set to "black-white". If you can, you could also set "http://registry\\.gpii\\.net/common/highContrast/enabled" with value set to true, however looking at the capability transform, it will take a theme value or the enabled preference set to mean there is a defined contrast.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I had an inverse transformation in a previous commit, see lines 32580 (enabled) and 32595 (contrastTheme). It worked. However, when running the solutions registry unit tests, the inverse for contrastTheme was found invalid with the message:

ERROR: The capabilities transforms for win32.json5 -> net.gpii.uioPlus -> contrastTheme may result in data corruption.

I'll compare your suggestions with what I did.

Copy link
Contributor

Choose a reason for hiding this comment

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

@klown I don't know what that error message means, so if you're still getting that we may need to check with @amb26 or maybe @the-t-in-rtf.

Copy link
Member Author

@klown klown Jun 17, 2020

Choose a reason for hiding this comment

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

@jobara, I managed to get past the validation error by having the inverse transformation for http://registry\\.gpii\\.net/common/highContrastTheme output a value of regular-contrast when contrastTheme is anything but default. That makes sense because the inbound transformation for contrastTheme checks only a value of regular-contrast. I'll push that soon.

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 this is remarkably similar to what @sgithens did for desktop contrast in #874

Copy link
Member Author

@klown klown Jun 17, 2020

Choose a reason for hiding this comment

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

Note that this is remarkably similar to what @sgithens did for desktop contrast in #874

Yes, it looks like whoever wrote the capabilitiesTransformations transform of highContrast and highContrastTheme for the com.microsoft.windows.highContrast solution used the same rules for the net.gpii.uioPlus solution. There is even a comment to that effect:
// Use the same condition for applying a high contrast theme to windows

I took the capabilitiesTransformations for UIO+ as given and worked on providing the missing inverseCapabilitiesTransformations.

Seeing as @sgithens modifications have been merged into master, I will bring the logic of the contrast transformation here into line with that, and, as appropriate, the inverse transformation.

However, I just noticed that UIO+ does not send any kind of "enabled" setting in its request with respect to high contrast, only a contrastTheme setting. See the example clip from @jobara. That suggests a simplification of the transformation rules here. Or, perhaps, that UIO+ should be sending a highContrast: true/false/default setting.

Copy link
Member Author

@klown klown Jun 23, 2020

Choose a reason for hiding this comment

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

Okay, I've modified the net.gpii.uioPlus transformation for high contrast similar to what @sgithens has done for com.microsoft.windows.highContrast, but I've left the inverse transformation as is. The inverse for the windows solution is a straightforward mapping of a windows on/off value to the enabled common term. The inverse for UIOPlus is from its contrast theme setting (e.g., "by") to both the enabled common term and the common term for the theme (e.g., "black-yellow").

Let me know if this is correct.

// When the connection is done, the client tells to the flow manager its id

socket.on("open", function () {
console.log("## Socket connected");
console.log("## browserChannelClient: Socket connected");
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it common for GPII to use console.log instead of fluid.log?

Copy link
Member Author

Choose a reason for hiding this comment

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

:-) GPII, in general, uses fluid.log(). But, some examples and other demo code do not. This particular example doesn't require infusion, or kettle for that matter, and only needs the ws WebSocket package.

I'm inclined to re-write it as an infusion component and possibly use kettle for the requests. That way, it would resemble how UIO+ or Morphic clients access the /browserchannel endpoint. But, I'm not sure that's a high priority.

Copy link
Contributor

Choose a reason for hiding this comment

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

@klown, yes I'm not sure either. I'll defer to @amb26 for that.

Copy link
Member

Choose a reason for hiding this comment

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

Leaving this with console.log is fine

…astTheme'

Modified the inverse transformation so that it validates.
@gpii-bot
Copy link

CI job passed: https://ci.gpii.net/job/universal-tests/2375/

@gpii-bot
Copy link

CI job passed: https://ci.gpii.net/job/universal-tests/2382/

@gpii-bot
Copy link

CI job passed: https://ci.gpii.net/job/universal-tests/2383/

@gpii-bot
Copy link

CI job passed: https://ci.gpii.net/job/universal-tests/2384/

@amb26 amb26 merged commit 5cdb9ec into GPII:master Jun 30, 2020
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.

4 participants