Introducing EventBus for the viewer UI. #7254
Conversation
| var eventBus = new EventBus(); | ||
| var count = 0; | ||
| eventBus.on('test', function () { | ||
| count++; |
There was a problem hiding this comment.
Nit: this line needs to be indented with one more space
yurydelendik
force-pushed
the
eventbus
branch
2 times, most recently
from
fd0eaf7
to
ecb023d
Compare
| renderingQueue: pdfRenderingQueue, | ||
| linkService: pdfLinkService, | ||
| downloadManager: downloadManager | ||
| }); | ||
| }, eventBus); |
There was a problem hiding this comment.
PDFViewer takes only one parameter options and you're already passing the event bus in the options object, so this change does not appear to be required.
| }(this, function (exports, uiUtils) { | ||
| var EventBus = uiUtils.EventBus; | ||
|
|
||
| // Attaching to the application event bus to displach events to the DOM for |
There was a problem hiding this comment.
Nit: I think you mean "dispatch events" here
| @@ -384,6 +384,46 @@ function getPDFFileNameFromURL(url) { | |||
| return suggestedFilename || 'document.pdf'; | |||
| } | |||
|
|
|||
| /** | |||
| * Simple event bus for an application. The listener are attached using the | |||
There was a problem hiding this comment.
Nit: listener_s_ are attached
|
It seems that an |
| window.addEventListener('presentationmodechanged', function(evt) { | ||
| if (!evt.detail.active && !evt.detail.switchInProgress && | ||
| self.eventBus.on('presentationmodechanged', function(e) { | ||
| if (!e.active && !e.switchInProgress && | ||
| self.isThumbnailViewVisible) { |
There was a problem hiding this comment.
Nit: I think that this condition will now fit on one line.
Fixed |
|
/botio-linux preview |
From: Bot.io (Linux)ReceivedCommand cmd_preview from @timvandermeij received. Current queue size: 0 Live output at: http://107.21.233.14:8877/14c13b0714ced29/output.txt |
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/14c13b0714ced29/output.txt Total script time: 1.05 mins Published |
|
/botio unittest |
From: Bot.io (Windows)ReceivedCommand cmd_unittest from @timvandermeij received. Current queue size: 0 Live output at: http://107.22.172.223:8877/90540c83462160f/output.txt |
From: Bot.io (Linux)ReceivedCommand cmd_unittest from @timvandermeij received. Current queue size: 0 Live output at: http://107.21.233.14:8877/80648a52743af1e/output.txt |
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/80648a52743af1e/output.txt Total script time: 1.52 mins
|
From: Bot.io (Windows)SuccessFull output at http://107.22.172.223:8877/90540c83462160f/output.txt Total script time: 1.82 mins
|
| eventBus.dispatch('test'); | ||
| expect(count).toEqual(2); | ||
| }); | ||
| it('dispatch event multiple handlers', function () { |
There was a problem hiding this comment.
Nit: 'dispatch event to multiple handlers'.
|
|
||
| eventBus.on('resize', webViewerResize); | ||
| eventBus.on('localized', webViewerLocalized); | ||
| eventBus.on('fileinputchange', webViewerFileInputChange); |
There was a problem hiding this comment.
The function that is referenced here is placed within a GENERIC preprocessor tag, shouldn't this line be too, in order to prevent any possible issues?
There was a problem hiding this comment.
Good catch. Fixed.
| PDFViewerApplication.findBar.dispatchEvent('again', | ||
| cmd === 5 || cmd === 12); | ||
| PDFViewerApplication.eventBus.dispatch('findagain', | ||
| cmd === 5 || cmd === 12); |
There was a problem hiding this comment.
This doesn't look right, and it stops Ctrl+G from working in the preview.
There was a problem hiding this comment.
Thanks. Fixed.
|
/botio-windows preview |
From: Bot.io (Windows)ReceivedCommand cmd_preview from @Snuffleupagus received. Current queue size: 0 Live output at: http://107.22.172.223:8877/f738ff8efdbdcd2/output.txt |
From: Bot.io (Windows)SuccessFull output at http://107.22.172.223:8877/f738ff8efdbdcd2/output.txt Total script time: 1.69 mins Published |
|
/botio unittest |
From: Bot.io (Windows)ReceivedCommand cmd_unittest from @Snuffleupagus received. Current queue size: 0 Live output at: http://107.22.172.223:8877/79bc67cfaae176e/output.txt |
From: Bot.io (Linux)ReceivedCommand cmd_unittest from @Snuffleupagus received. Current queue size: 0 Live output at: http://107.21.233.14:8877/c6991f8c2f789b2/output.txt |
From: Bot.io (Linux)SuccessFull output at http://107.21.233.14:8877/c6991f8c2f789b2/output.txt Total script time: 1.54 mins
|
From: Bot.io (Windows)SuccessFull output at http://107.22.172.223:8877/79bc67cfaae176e/output.txt Total script time: 1.75 mins
|
|
This looks great, thank you for the patch! |
Goal is to reduce amount of callback and dependencies in the application modules. Currently we use DOM events, but they are inconsistent. Replacing all of that with custom EventBus -- we will have one per application. If event bus is not provided, it will use (legacy) global one, which will relay events for the DOM.
Future work will include replacing direct binding between button events with sending DOM events to the event bus, then processing them from app, e.g. toolbar and secondary toolbar will send a presentation mode event, which will be relayed to the presentation mode module (notice that this event shall be handled sync too, which is the reason why we cannot choose async event bus).
Blocks #7204 and #7205