-
Notifications
You must be signed in to change notification settings - Fork 617
Standalone windows patch #129
Changes from 6 commits
d698363
ee93490
74412b4
f554789
8d81865
e4810ae
6a7961c
73b6403
01ef745
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
import * as pty from 'pty.js'; | ||
import * as _ from 'lodash' | ||
import * as pty from 'ptyw.js'; | ||
import * as _ from 'lodash'; | ||
|
||
export default class Aliases { | ||
static aliases: _.Dictionary<string>; | ||
|
@@ -14,6 +14,8 @@ export default class Aliases { | |
} | ||
|
||
static importAliasesFrom(shellName: string): void { | ||
if (process.platform === 'win32') return; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about creating an enum with platforms in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have you noticed my comment about an enum? #129 (comment) |
||
|
||
var zsh = pty.spawn(shellName, ['-i', '-c', 'alias'], {env: process.env}); | ||
|
||
var aliases = ''; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,6 +34,9 @@ export default class Buffer extends events.EventEmitter { | |
var current = {attributes: <i.Attributes>null, text: ''}; | ||
var cursorPosition = cursor.getPosition(); | ||
|
||
/* Quick fix for the strange character at the end (https://github.com/black-screen/black-screen/pull/56#issuecomment-139653723) */ | ||
if (process.platform === 'win32' && row.length < 1) return; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Triple equal is useless in TypeScript. In JS it compares with types, but, you know, TypeScript compares types out of the box. :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not a mistake to use strict equality (triple equals) in TypeScript. When you read a piece of code that contains strict equality it's obvious to the reader that no type coercion will happen there. TypeScript transpiles equality operators as is to the JavaScript code, so the meaning is still clear in js. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right, my bad. |
||
|
||
if (index == cursorPosition.row && this.cursor.getShow()) { | ||
var rowWithCursor: Char[] = []; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,7 @@ | ||
import Invocation from "./Invocation"; | ||
import Command from "./Command"; | ||
import Utils from './Utils'; | ||
import * as pty from 'pty.js'; | ||
import * as pty from 'ptyw.js'; | ||
import * as _ from 'lodash'; | ||
|
||
export default class CommandExecutor { | ||
|
@@ -51,6 +51,12 @@ class BuiltInCommandExecutionStrategy extends CommandExecutionStrategy { | |
class SystemFileExecutionStrategy extends CommandExecutionStrategy { | ||
startExecution() { | ||
return new Promise((resolve, reject) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The purpose of the strategy classes is to replace conditions with polymorphism. Please create a separate strategy for Windows. You might want to extract the common pieces into an intermediate super class. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've refactored the way we choose a strategy to make it easier to extend. Sorry, if it created conflicts. |
||
if (process.platform === 'win32') { | ||
this.args.unshift(this.command); | ||
this.args = ['/s', '/c', this.args.join(' ')]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Have you tried executing a command with multiple parameters? I'm not sure it works as expected. |
||
this.command = Utils.getCmdPath(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please do this in the constructor. |
||
} | ||
|
||
// TODO: move command to this class. | ||
this.invocation.command = pty.spawn(this.command, this.args, { | ||
cols: this.invocation.dimensions.columns, | ||
|
@@ -61,7 +67,8 @@ class SystemFileExecutionStrategy extends CommandExecutionStrategy { | |
|
||
this.invocation.command.stdout.on('data', (data: string) => this.invocation.parser.parse(data.toString())); | ||
this.invocation.command.on('exit', (code: number) => { | ||
if (code === 0) { | ||
/* In windows there is no code returned (null) so instead of comparing to 0 we check if its 0 or null with ! */ | ||
if (!code) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about converting |
||
resolve(); | ||
} else { | ||
reject(); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,11 +16,11 @@ export default class Terminal extends events.EventEmitter { | |
history: History; | ||
gitBranchWatcher: fs.FSWatcher; | ||
|
||
private stateFileName = `${process.env.HOME}/.black-screen-state`; | ||
private stateFileName = `${Utils.getHomeDirectory()}/.black-screen-state`; | ||
|
||
// The value of the dictionary is the default value used if there is no serialized data. | ||
private serializableProperties: _.Dictionary<any> = { | ||
currentDirectory: `String:${process.env.HOME}`, | ||
currentDirectory: `String:${Utils.getHomeDirectory()}`, | ||
history: `History:[]` | ||
}; | ||
|
||
|
@@ -37,14 +37,17 @@ export default class Terminal extends events.EventEmitter { | |
|
||
createInvocation(): void { | ||
var invocation = new Invocation(this.currentDirectory, this.dimensions, this.history); | ||
invocation.once('end', () => { | ||
if (app.dock) { | ||
app.dock.bounce('informational'); | ||
} | ||
this.createInvocation(); | ||
}).once('working-directory-changed', (newWorkingDirectory: string) => | ||
this.setCurrentDirectory(newWorkingDirectory) | ||
); | ||
|
||
invocation | ||
.once('clear', _ => this.clearInvocations()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We'll remove events in the future, so try to avoid them. Although, I'll accept this PR even with the new event, because it might be hard to implement it in a different way. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I haven't noticed the previous time: what is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It clears the invocations, like pressing CTRL + L but typing the command clear. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I understand, but there is no command typing part of the story in this PR, right? And shouldn't be for that matter, because it's not about Windows support. |
||
.once('end', _ => { | ||
if (app.dock) { | ||
app.dock.bounce('informational'); | ||
} | ||
this.createInvocation(); | ||
}) | ||
.once('working-directory-changed', (newWorkingDirectory: string) => this.setCurrentDirectory(newWorkingDirectory)); | ||
|
||
this.invocations = this.invocations.concat(invocation); | ||
this.emit('invocation'); | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -123,6 +123,20 @@ export default class Utils { | |
}); | ||
} | ||
|
||
static getCmdPath(): string { | ||
if (process.env.comspec) { | ||
return process.env.comspec; | ||
} | ||
else if (process.env.SystemRoot) { | ||
return Path.join(process.env.SystemRoot, 'System32', 'cmd.exe'); | ||
} | ||
else return 'cmd.exe'; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since it's Windows-only, please move it to the strategy class mentioned above. |
||
|
||
static getHomeDirectory(): string { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can see you use the |
||
return process.env[(process.platform === 'win32') ? 'USERPROFILE' : 'HOME']; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Remember about |
||
} | ||
|
||
private static delegate(name: string, args: Array<any>): void { | ||
if ((typeof window != 'undefined') && (<any>window)['DEBUG']) { | ||
(<any>console)[name](...args); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please just change the version to point to your master on Github instead of changing the name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ShockOne If I point to http://github.com/iiegor/pty.js (specifically I'm pointing to the win-fix branch), the git submodule of the repo doesn't initialize, so node-gyp rebuild fails 😕
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Understood, let's leave it as is.