Refactor Electron's main process #951

2018-01-18T03:21:34+01:00

IGassmann commented

2018-01-18 03:21:34 +01:00

(Migrated from github.com)

This closes #938, closes #298 and fixes #298.

kauffj commented

2018-01-18 19:40:43 +01:00

(Migrated from github.com)

@alexliebowitz I think a decent portion of this code was originally yours, it'd be good to get your eyes on this changeset.

liamcardenas commented

2018-01-18 19:58:18 +01:00

(Migrated from github.com)

I agree, everything looks good here to me but it would be gret to get Alex's take. I am ready to merge if Alex gives the thumbs up

alexliebowitz (Migrated from github.com) reviewed 2018-01-19 16:55:20 +01:00

alexliebowitz (Migrated from github.com) left a comment

Very minor things, can be merged now if needed. Awesome overall.

src/main/createWindow.js

					
				@ -0,0 +92,4 @@

				    }

				  });

				  window.webContents.on('crashed', () => {

alexliebowitz (Migrated from github.com) commented

2018-01-19 16:50:20 +01:00

It doesn't seem like this event is actually sent on the renderer side. Did you mean to build that, or is it something we're going to add later?

src/main/index.js

alexliebowitz (Migrated from github.com) commented

2018-01-19 16:48:09 +01:00

Hmm, now we're doing this same substitution in two places instead of in one function. That feels brittle (what if we find another problem with Windows URI handling and don't realize it's there in two places), and also

We might want to move this back into its own function, or at least add another comment here. Just a thought.

Hmm, now we're doing this same substitution in two places instead of in one function. That feels brittle (what if we find another problem with Windows URI handling and don't realize it's there in two places), and also We might want to move this back into its own function, or at least add another comment here. Just a thought.

IGassmann (Migrated from github.com) reviewed 2018-01-19 16:58:40 +01:00

src/main/createWindow.js

					
				@ -0,0 +92,4 @@

				    }

				  });

				  window.webContents.on('crashed', () => {

IGassmann (Migrated from github.com) commented

2018-01-19 16:58:40 +01:00

It's necessary to handle it: https://electronjs.org/docs/api/web-contents#event-crashed

alexliebowitz (Migrated from github.com) reviewed 2018-01-19 16:59:53 +01:00

src/main/Tray.js

					
				@ -0,0 +2,4 @@

				import path from 'path';

				import createWindow from './createWindow';

				export default class Tray {

alexliebowitz (Migrated from github.com) commented

2018-01-19 16:59:53 +01:00

Does this need to be a whole object? create() is only called once and it doesn't need any state, so maybe just a createTray function that takes an updateAttachedWindow callback?

Does this need to be a whole object? `create()` is only called once and it doesn't need any state, so maybe just a `createTray` function that takes an `updateAttachedWindow` callback?

IGassmann (Migrated from github.com) reviewed 2018-01-19 17:35:27 +01:00

src/main/Tray.js

					
				@ -0,0 +2,4 @@

				import path from 'path';

				import createWindow from './createWindow';

				export default class Tray {

IGassmann (Migrated from github.com) commented

2018-01-19 17:35:27 +01:00

I went for a exported createTray() function initially. However, since it also needs to manage the state of the window, I finally went for using a class.

I went for a exported `createTray()` function initially. However, since it also needs to manage the state of the window, I finally went for using a class.

Sign in to join this conversation.

No reviewers