Merge TS context isolation branch onto Typescript master branch #598

KiranNiranjan · 2019-03-18T13:09:15Z

Merge Typescript context isolation branch onto Typescript master branch

Unit test result

VishwasShashidhar

Please handle the below points

Surround all if statements with braces.
Update the logger code to include rotation based on date & remove max size
Add comments where the logic is complex

VishwasShashidhar · 2019-03-18T14:15:50Z

demo/index.html

@@ -132,7 +132,9 @@ <h1>Symphony Electron API Demo</h1>

            num++;

-            var notf = new ssf.Notification(title, {


Can we retain this till we completely move to context isolation?

No. In this branch, ssf does not exist

VishwasShashidhar · 2019-03-18T14:16:11Z

demo/index.html


-            notf.addEventListener('click', onclick);
+            /*notf.addEventListener('click', onclick);


remove commented code

VishwasShashidhar · 2019-03-18T14:19:08Z

src/app/window-handler.ts

-                if (this.screenSharingIndicatorWindow
-                    && this.windowExists(this.screenSharingIndicatorWindow)) this.screenSharingIndicatorWindow.close();
+                if (winKey) {
+                    const browserWindow = this.windows[ winKey ];


remove extra spaces inside the array

This is TS recommended

VishwasShashidhar · 2019-03-18T14:20:48Z

src/app/window-handler.ts

+                displayId.includes(d.id.toString()))[ 0 ]) || electron.screen.getPrimaryDisplay();
+
+        const screenRect = indicatorScreen.workArea;
+        let opts = { ...WindowHandler.getScreenSharingIndicatorOpts(), ...{ winKey: streamId } };


can you add a comment above explaining what this logic is about

VishwasShashidhar · 2019-03-18T14:23:04Z

src/common/animation-queue.ts

+     * @param object
+     */
+    public animate(object) {
+        object.func.apply(null, object.args)


can we not use async await?

VishwasShashidhar · 2019-03-18T14:28:48Z

src/renderer/notification-handler.ts

+     */
+    public setWindowPosition(window: Electron.BrowserWindow, x: number, y: number) {
+        if (window && !window.isDestroyed()) {
+            window.setPosition(parseInt(String(x), 10), parseInt(String(y), 10));


what happens if parseInt fails? wouldn't it be better to have defaults?

VishwasShashidhar · 2019-03-18T14:29:10Z

src/renderer/notification-handler.ts

+     */
+    public setupNotificationPosition() {
+        // This feature only applies to windows
+        if (!isMac) {


let's invert the condition. If it is mac, return.

VishwasShashidhar · 2019-03-18T14:33:00Z

src/renderer/notification.ts

+    constructor(opts) {
+        super(opts);
+        ipcMain.on('close-notification', (_event, windowId) => {
+            this.hideNotification(windowId);


does this destroy the browser window or just hide it?

This just hides the browser window

But won't that be waste of resources. What happens if we close it?

Approx 200mb will be reserved based on the display width for about a min. Without this, every notification will be rendered a fresh which looks delayed

Understood, thanks for the explanation.

vrbsm

Please, fix this variable in /.../SymphonyElectron/spec/screenSharingIndicator.spec.ts

 wrapper.setState({ streamId: 'id-123' }); // add this
 const closeIpcRendererMock = {
            cmd: 'close-window',
            windowType: 'screen-sharing-indicator',
            winKey: 'id-123', // add this
        };

this unit test is failing: 'should call close correctly'

vrbsm · 2019-03-18T16:21:34Z

src/common/api-interface.ts

+}
+
+export type LogLevel = 'error' | 'warn' | 'info' | 'verbose' | 'debug' | 'silly';
