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

Refactor Electron's main process #951

Merged
merged 3 commits into from Jan 19, 2018

Conversation

Projects
None yet
4 participants
@IGassmann
Contributor

IGassmann commented Jan 18, 2018

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

Igor Gassmann added some commits Jan 9, 2018

Igor Gassmann

@IGassmann IGassmann self-assigned this Jan 18, 2018

@IGassmann IGassmann requested a review from liamcardenas Jan 18, 2018

@lbry-bot lbry-bot assigned liamcardenas and unassigned IGassmann Jan 18, 2018

@kauffj

This comment has been minimized.

Show comment
Hide comment
@kauffj

kauffj Jan 18, 2018

Member

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

Member

kauffj commented Jan 18, 2018

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

@liamcardenas

This comment has been minimized.

Show comment
Hide comment
@liamcardenas

liamcardenas Jan 18, 2018

Contributor

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

Contributor

liamcardenas commented Jan 18, 2018

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

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

let URI;
if (process.platform === 'win32' && String(argv[1]).startsWith('lbry')) {
// Keep only command line / deep linked arguments
URI = argv[1].replace(/\/$/, '').replace('/#', '#');

This comment has been minimized.

@alexliebowitz

alexliebowitz Jan 19, 2018

Collaborator

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.

@alexliebowitz

alexliebowitz Jan 19, 2018

Collaborator

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.

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

This comment has been minimized.

@alexliebowitz

alexliebowitz Jan 19, 2018

Collaborator

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?

@alexliebowitz

alexliebowitz Jan 19, 2018

Collaborator

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?

This comment has been minimized.

@IGassmann
@IGassmann
import path from 'path';
import createWindow from './createWindow';
export default class Tray {

This comment has been minimized.

@alexliebowitz

alexliebowitz Jan 19, 2018

Collaborator

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?

@alexliebowitz

alexliebowitz Jan 19, 2018

Collaborator

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?

This comment has been minimized.

@IGassmann

IGassmann Jan 19, 2018

Contributor

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.

@IGassmann

IGassmann Jan 19, 2018

Contributor

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.

@lyoshenka lyoshenka removed the request for review from liamcardenas Jan 19, 2018

@lbry-bot lbry-bot assigned IGassmann and unassigned liamcardenas and IGassmann Jan 19, 2018

@IGassmann IGassmann merged commit 8940942 into master Jan 19, 2018

2 checks passed

Linux (LBRY App) TeamCity build finished
Details
OSX (LBRY App) TeamCity build finished
Details

@IGassmann IGassmann deleted the issue/938 branch Feb 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment