[N/A] Hook up event propagation for all browserview-related events by MichaelMCoates · Pull Request #976 · HadoukenIO/core

MichaelMCoates · 2019-10-02T17:11:20Z

Description of Change

This PR adds propagated events up to Application and System for BrowserView.

Tests:
https://testing-dashboard.openfin.co/#/app/sessions/api/completed/5da8b1ceee166d7a14d451bc

Corresponding js-adapter PR:
HadoukenIO/js-adapter#357

@aziz512 trying to add you as reviewer but it won't let me for some reason.

Checklist

PR description included and stakeholders cc'd
npm test passes
automated tests are changed or added
relevant documentation is changed or added
Link to new tests added
PR has assigned reviewers

Release Notes

Notes:

openfin-github-bot · 2019-10-02T17:41:04Z

c7afb49

Git

core: develop <= dev/coates/bv-event-propagation (c7afb49)
js-adapter: develop <= dev/coates/bv-event-propagation (eae221f)
javascript-adapter: develop

Asars used for testing

Test results

✅ Windows 7 (64-bit runtime): Failed 0/665
✅ Windows 7 (32-bit runtime): Failed 0/665
😰 Windows 10 (64-bit runtime): Failed 1/665
😰 Windows 10 (32-bit runtime): Failed 1/665

openfin-github-bot · 2019-10-10T00:49:08Z

⚠️ Failed to build `c55acc4`

openfin-github-bot · 2019-10-10T15:13:05Z

ab57f1c

Git

core: develop <= dev/coates/bv-event-propagation (ab57f1c)
js-adapter: develop <= dev/coates/bv-event-propagation (87dedcf)
javascript-adapter: develop

Asars used for testing

Test results

✅ Windows 10 (32-bit runtime): Failed 0/665
😰 Windows 10 (64-bit runtime): Failed 1/665
✅ Windows 7 (32-bit runtime): Failed 0/665
✅ Windows 7 (64-bit runtime): Failed 0/665

openfin-github-bot · 2019-10-10T18:06:04Z

99ab0e7

Git

core: develop <= dev/coates/bv-event-propagation (99ab0e7)
js-adapter: develop <= dev/coates/bv-event-propagation (f231be5)
javascript-adapter: develop

Asars used for testing

Test results

😰 Windows 7 (32-bit runtime): Failed 1/665
😰 Windows 10 (32-bit runtime): Failed 1/665
✅ Windows 10 (64-bit runtime): Failed 0/665
✅ Windows 7 (64-bit runtime): Failed 0/665

dhamberlin · 2019-10-11T15:32:14Z

src/browser/of_events.ts

        if (tokenizedRoute.length >= 2) {
            const [channel, topic] = tokenizedRoute;
            const uuid: string = (payload && payload.uuid) || tokenizedRoute[2] || '*';
+            const name: string|false = (payload && payload.name);


Mostly irrelevant nitpick: will eval to undefined if there's no payload or no name.
(!!payload && payload.name) will evaluate to false if there's no payload, but I think unidentified is probably actually what we want in this case anyhow.

Agreed. Is there a case in which this could not work as expected?

I don't think so. I would remove the |false typing, since this will never eval to false. But code looks good as is.

Ah, right, good call, missed that. Changed.

src/browser/api/browser_view.ts

dhamberlin

lgtm

pbaize

If we restrict views to targets in the same app do we still need the custom window logic?

openfin-github-bot · 2019-10-16T17:20:51Z

27aee21

Git

core: develop <= dev/coates/bv-event-propagation (27aee21)
js-adapter: develop <= dev/coates/bv-event-propagation (f231be5)
javascript-adapter: develop

Asars used for testing

Test results

😱 Windows 7 (64-bit runtime): Failed 16/665
✅ Windows 10 (32-bit runtime): Failed 0/665
😱 Windows 7 (32-bit runtime): Failed 16/665
😱 Windows 10 (64-bit runtime): Failed 4/665

MichaelMCoates · 2019-10-16T19:05:38Z

The custom window logic is still necessary for view-detached because of the way the routing works in of_events.

openfin-github-bot · 2019-10-16T20:08:04Z

fd30fbd

Git

core: develop <= dev/coates/bv-event-propagation (fd30fbd)
js-adapter: develop <= dev/coates/bv-event-propagation (f231be5)
javascript-adapter: develop

Asars used for testing

Test results

😰 Windows 7 (32-bit runtime): Failed 2/665
😰 Windows 10 (32-bit runtime): Failed 1/665
😱 Windows 10 (64-bit runtime): Failed 21/665
😰 Windows 7 (64-bit runtime): Failed 1/665

openfin-github-bot · 2019-10-17T16:05:04Z

bad0f85

Git

core: develop <= dev/coates/bv-event-propagation (bad0f85)
js-adapter: develop <= dev/coates/bv-event-propagation (f231be5)
javascript-adapter: develop

Asars used for testing

Test results

😱 Windows 7 (32-bit runtime): Failed 3/667
😰 Windows 7 (64-bit runtime): Failed 2/667
😱 Windows 10 (64-bit runtime): Failed 12/667
😰 Windows 10 (32-bit runtime): Failed 2/667

MichaelMCoates · 2019-10-17T18:32:49Z

After talking with Pierre and Tommy, changed the eventing scheme to the following:

Windows can listen to view-attached and view-detached.
Views can listen to target-changed.
Window view events don't propagate up to Application and System.
All view events propagate up to Window, Application, and System.

The tests have been updated to reflect these changes.

openfin-github-bot · 2019-10-17T19:21:04Z

f877a7f

Git

core: develop <= dev/coates/bv-event-propagation (f877a7f)
js-adapter: develop <= dev/coates/bv-event-propagation (2289f59)
javascript-adapter: develop

Asars used for testing

Test results

😱 Windows 7 (32-bit runtime): Failed 4/667
😱 Windows 10 (64-bit runtime): Failed 7/667
😱 Windows 10 (32-bit runtime): Failed 3/667
😱 Windows 7 (64-bit runtime): Failed 3/667

Hook up event propagation for all browserview-related events

c7afb49

MichaelMCoates requested review from datamadic, pbaize and rdepena October 2, 2019 17:11

finos-admin added the cla-present label Oct 2, 2019

MichaelMCoates mentioned this pull request Oct 2, 2019

[N/A] Hook up event propagation for all browserview-related events HadoukenIO/js-adapter#357

Closed

6 tasks

MichaelMCoates changed the title ~~[N/A] Hook up event propagation for all browserview-related events~~ DO NOT REVIEW YET [N/A] Hook up event propagation for all browserview-related events Oct 2, 2019

openfin-github-bot bot added the auto testing started Bot started automated testing label Oct 2, 2019

openfin-github-bot bot added auto testing done Bot completed automated testing and removed auto testing started Bot started automated testing labels Oct 2, 2019

Propagate browserview events up and through window

c55acc4

openfin-github-bot bot added auto testing failed Bot failed to build and removed auto testing done Bot completed automated testing labels Oct 10, 2019

Merge branch 'develop' into dev/coates/bv-event-propagation

ab57f1c

openfin-github-bot bot added auto testing started Bot started automated testing and removed auto testing failed Bot failed to build labels Oct 10, 2019

openfin-github-bot bot added auto testing done Bot completed automated testing and removed auto testing started Bot started automated testing labels Oct 10, 2019

Add test comment

99ab0e7

openfin-github-bot bot added auto testing started Bot started automated testing and removed auto testing done Bot completed automated testing labels Oct 10, 2019

openfin-github-bot bot added auto testing done Bot completed automated testing and removed auto testing started Bot started automated testing labels Oct 10, 2019

MichaelMCoates changed the title ~~DO NOT REVIEW YET [N/A] Hook up event propagation for all browserview-related events~~ [N/A] Hook up event propagation for all browserview-related events Oct 11, 2019

dhamberlin reviewed Oct 11, 2019

View reviewed changes

aziz512 suggested changes Oct 11, 2019

View reviewed changes

src/browser/api/browser_view.ts Outdated Show resolved Hide resolved

openfin-github-bot bot added auto testing started Bot started automated testing and removed auto testing done Bot completed automated testing labels Oct 16, 2019

dhamberlin previously approved these changes Oct 16, 2019

View reviewed changes

pbaize suggested changes Oct 16, 2019

View reviewed changes

openfin-github-bot bot added auto testing done Bot completed automated testing and removed auto testing started Bot started automated testing labels Oct 16, 2019

Remove view-attached custom event emit

fd30fbd

MichaelMCoates dismissed stale reviews from dhamberlin and aziz512 via fd30fbd October 16, 2019 19:05

openfin-github-bot bot added auto testing started Bot started automated testing and removed auto testing done Bot completed automated testing labels Oct 16, 2019

openfin-github-bot bot added auto testing done Bot completed automated testing and removed auto testing started Bot started automated testing labels Oct 16, 2019

Change the way we fetch target for view event propagation

bad0f85

openfin-github-bot bot added auto testing started Bot started automated testing and removed auto testing done Bot completed automated testing labels Oct 17, 2019

openfin-github-bot bot added auto testing done Bot completed automated testing and removed auto testing started Bot started automated testing labels Oct 17, 2019

Change eventing to target-changed and view-attached/detached for windows

a02955f

Unexclude view-target-changed propagation, but don't document it

f877a7f

openfin-github-bot bot added auto testing started Bot started automated testing and removed auto testing done Bot completed automated testing labels Oct 17, 2019

openfin-github-bot bot added auto testing done Bot completed automated testing and removed auto testing started Bot started automated testing labels Oct 17, 2019

Conversation

MichaelMCoates commented Oct 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of Change

Checklist

Release Notes

Uh oh!

openfin-github-bot bot commented Oct 2, 2019

Git

Asars used for testing

Test results

Uh oh!

openfin-github-bot bot commented Oct 10, 2019

⚠️ Failed to build c55acc4

Uh oh!

openfin-github-bot bot commented Oct 10, 2019

Git

Asars used for testing

Test results

Uh oh!

openfin-github-bot bot commented Oct 10, 2019

Git

Asars used for testing

Test results

Uh oh!

dhamberlin Oct 11, 2019

Choose a reason for hiding this comment

Uh oh!

MichaelMCoates Oct 16, 2019

Choose a reason for hiding this comment

Uh oh!

dhamberlin Oct 16, 2019

Choose a reason for hiding this comment

Uh oh!

MichaelMCoates Oct 16, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dhamberlin left a comment

Choose a reason for hiding this comment

Uh oh!

pbaize left a comment

Choose a reason for hiding this comment

Uh oh!

openfin-github-bot bot commented Oct 16, 2019

Git

Asars used for testing

Test results

Uh oh!

MichaelMCoates commented Oct 16, 2019

Uh oh!

openfin-github-bot bot commented Oct 16, 2019

Git

Asars used for testing

Test results

Uh oh!

openfin-github-bot bot commented Oct 17, 2019

Git

Asars used for testing

Test results

Uh oh!

MichaelMCoates commented Oct 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openfin-github-bot bot commented Oct 17, 2019

Git

Asars used for testing

Test results

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

MichaelMCoates commented Oct 2, 2019 •

edited

Loading

⚠️ Failed to build `c55acc4`

MichaelMCoates commented Oct 17, 2019 •

edited

Loading