Skip to content
This repository has been archived by the owner on Jul 19, 2019. It is now read-only.

CHE-9840 add factory events handling #3

Merged
merged 1 commit into from
Jul 17, 2018
Merged

CHE-9840 add factory events handling #3

merged 1 commit into from
Jul 17, 2018

Conversation

olexii4
Copy link
Contributor

@olexii4 olexii4 commented Jul 6, 2018 •
edited
Loading

What does this PR do?

Add factory events handling. Implement OpenFile action.

What issues does this PR fix or reference?

eclipse-che/che#10162
eclipse-che/che#9840

https://bluejeans.com/s/s@Q@K/

Signed-off-by: Oleksii Orel oorel@redhat.com

@ashumilova ashumilova mentioned this pull request Jul 11, 2018
Closed
20 tasks
AndrienkoAleksandr
AndrienkoAleksandr reviewed Jul 12, 2018
View reviewed changes
che-theia-factory/src/browser/types.ts Outdated
greetingTitle?: string,
greetingContentUrl?: string
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missed line.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

AndrienkoAleksandr
AndrienkoAleksandr reviewed Jul 12, 2018
View reviewed changes
che-theia-factory/src/browser/factory-theia-client.ts Outdated

this.git.clone(
importProjectPromices.push(this.git.clone(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could use "await" here. But up to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am sure about that. We should start in parallel and wait for resolve importProjectPromices.

AndrienkoAleksandr
AndrienkoAleksandr reviewed Jul 12, 2018
View reviewed changes
che-theia-factory/src/browser/factory-theia-client.ts Outdated
this.git.checkout(repo, options);
}
return Promise.resolve();
}
).catch((error) => {
console.error(`Couldn't clone ${source.location} to ${projectPath}... ${error}`);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor, but such case: clone could be done successfully, but than error can be thrown on checkout operation. Message service always displays error message "Couldn't clone...", but in this case it should only notify about checkout fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. Done

benoitf
benoitf reviewed Jul 12, 2018
View reviewed changes
che-theia-factory/src/browser/factory-theia-client.ts Outdated

this.onAppLoaded((event: { actions: Array<IFactoryAction> }) => {
event.actions.forEach((onAppLoadedAction: IFactoryAction) => {
console.log('>>>>>>>>>>> onAppLoaded.action', onAppLoadedAction);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

log trace to be removed ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

benoitf
benoitf reviewed Jul 12, 2018
View reviewed changes
che-theia-factory/src/browser/factory-theia-client.ts Outdated
});
this.onAppClosed(() => {
this.onAppClosedActions.forEach((onAppClosedAction: IFactoryAction) => {
console.log('>>>>>>>>>>> onAppClosed.action', onAppClosedAction);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

log trace to remove ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

benoitf
benoitf reviewed Jul 12, 2018
View reviewed changes
che-theia-factory/src/browser/factory-theia-client.ts Outdated
}

getEnvVariable(name: string): EnvVariable | undefined {
if (!this.envVariables) {
return undefined;
}
return this.envVariables.find(function(envVariable) { return envVariable.name === name });
return this.envVariables.find(function (envVariable) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use of arrow function instead ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

sunix
sunix suggested changes Jul 16, 2018
View reviewed changes
Copy link
Contributor

@sunix sunix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing
in che-theia-factory/package.json ?

"@theia/editor": "0.3.12",

@sunix
Copy link
Contributor

sunix commented Jul 16, 2018

could you also rebase ? (so it works on Theia 0.3.12)

@olexii4
Copy link
Contributor Author

olexii4 commented Jul 17, 2018

Done

benoitf
benoitf reviewed Jul 17, 2018
View reviewed changes
che-theia-factory/src/browser/factory-manager.ts Outdated
@@ -0,0 +1,93 @@
/*
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI license there https://github.com/eclipse/che-theia-factory-extension/blob/master/LICENSE is EPL 2.0

so copyright headers should be 2.0 as well.

benoitf
benoitf reviewed Jul 17, 2018
View reviewed changes
che-theia-factory/src/browser/che-theia-factory-frontend-module.ts Outdated
@@ -1,7 +1,7 @@
/*
* Copyright (c) 2018-2018 Red Hat, Inc.
* All rights reserved. This program and the accompanying materials
* are made available under the terms of the Eclipse Public License v1.0
* are made available under the terms of the Eclipse Public License v2.0
* which accompanies this distribution, and is available at
* http://www.eclipse.org/legal/epl-v10.html
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

link is still pointing to v10.html

example of EPL 2.0 headers:
https://github.com/eclipse/theia-generator-plugin/blob/master/src/app/index.ts#L1-L9

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FAQ being there
https://www.eclipse.org/legal/epl-2.0/faq.php#h.q72cnghf29k0

@benoitf
Copy link
Member

benoitf commented Jul 17, 2018

headers should be

/*********************************************************************
* Copyright (c) 2018 Red Hat, Inc.
*
* This program and the accompanying materials are made
* available under the terms of the Eclipse Public License 2.0
* which is available at https://www.eclipse.org/legal/epl-2.0/
*
* SPDX-License-Identifier: EPL-2.0
**********************************************************************/

https://www.eclipse.org/legal/epl-2.0/faq.php#h.q72cnghf29k0

@olexii4
Copy link
Contributor Author

olexii4 commented Jul 17, 2018

Done

benoitf
benoitf approved these changes Jul 17, 2018
View reviewed changes
Copy link
Member

@benoitf benoitf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thx

@olexii4
CHE-9840 add factory events handling
2de7bb1 
Signed-off-by: Oleksii Orel <oorel@redhat.com>
@olexii4 olexii4 merged commit 0ee74c9 into eclipse:master Jul 17, 2018
@olexii4 olexii4 deleted the CHE-9840 branch July 17, 2018 14:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@benoitf benoitf benoitf approved these changes

@sunix sunix sunix approved these changes

@AndrienkoAleksandr AndrienkoAleksandr AndrienkoAleksandr approved these changes

@akurinnoy akurinnoy akurinnoy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@olexii4 @sunix @benoitf @AndrienkoAleksandr @akurinnoy