-
Notifications
You must be signed in to change notification settings - Fork 617
Standalone windows patch #129
Changes from 8 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 { | |
} | ||
|
||
private static importAliases(shellName: string = process.env.SHELL): 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 shell = pty.spawn(shellName, ['-i', '-c', 'alias'], {env: process.env}); | ||
|
||
var aliases = ''; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,7 +34,10 @@ export default class Buffer extends events.EventEmitter { | |
var current = {attributes: <i.Attributes>null, text: ''}; | ||
var cursorPosition = cursor.getPosition(); | ||
|
||
if (index === cursorPosition.row && this.cursor.getShow()) { | ||
/* 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[] = []; | ||
|
||
for (var i = 0; i !== row.length; ++i) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,8 @@ | ||
import Invocation from "./Invocation"; | ||
import Command from "./Command"; | ||
import Utils from './Utils'; | ||
import * as pty from 'pty.js'; | ||
import * as Path from 'path'; | ||
import * as pty from 'ptyw.js'; | ||
import * as _ from 'lodash'; | ||
|
||
abstract class CommandExecutionStrategy { | ||
|
@@ -63,6 +64,47 @@ class SystemFileExecutionStrategy extends CommandExecutionStrategy { | |
} | ||
} | ||
|
||
class WindowsSystemFileExecutionStrategy extends CommandExecutionStrategy { | ||
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 create the hierarchy
and extract the common logic to |
||
static canExecute(command: string): Promise<boolean> { | ||
return new Promise(resolve => Utils.getExecutablesInPaths().then(executables => resolve(_.include(executables, command)))); | ||
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 guess this should be return new Promise(resolve => resolve(Utils.isWindows())); |
||
} | ||
|
||
startExecution() { | ||
return new Promise((resolve, reject) => { | ||
this.args.unshift(this.command); | ||
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 can call |
||
this.args = ['/s', '/c', this.args.join(' ')]; | ||
this.command = this.cmdPath; | ||
|
||
this.invocation.command = pty.spawn(this.command, this.args, { | ||
cols: this.invocation.dimensions.columns, | ||
rows: this.invocation.dimensions.rows, | ||
cwd: this.invocation.directory, | ||
env: process.env | ||
}); | ||
|
||
this.invocation.command.stdout.on('data', (data: string) => this.invocation.parser.parse(data.toString())); | ||
this.invocation.command.on('exit', (code: number) => { | ||
/* In windows there is no code returned (null) so instead of comparing to 0 we check if its 0 or null with ! */ | ||
if (Number(code) === 0) { | ||
resolve(); | ||
} else { | ||
reject(); | ||
} | ||
}) | ||
}) | ||
} | ||
|
||
private get cmdPath(): string { | ||
if (process.env.comspec) { | ||
return process.env.comspec; | ||
} | ||
else if (process.env.SystemRoot) { | ||
return require('path').join(process.env.SystemRoot, 'System32', 'cmd.exe'); | ||
} | ||
else return 'cmd.exe'; | ||
} | ||
} | ||
|
||
class NullExecutionStrategy extends CommandExecutionStrategy { | ||
static canExecute(command: string): Promise<boolean> { | ||
return new Promise(resolve => resolve(true)); | ||
|
@@ -76,14 +118,17 @@ class NullExecutionStrategy extends CommandExecutionStrategy { | |
export default class CommandExecutor { | ||
private static executors = [ | ||
BuiltInCommandExecutionStrategy, | ||
SystemFileExecutionStrategy, | ||
process.platform === 'win32' ? WindowsSystemFileExecutionStrategy : SystemFileExecutionStrategy | ||
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. Just add two of them unconditionally, maybe we need to rename the old one to |
||
]; | ||
|
||
static execute(invocation: Invocation): Promise<CommandExecutionStrategy> { | ||
var command = invocation.getPrompt().getCommandName(); | ||
|
||
return Utils.filterWithPromising(this.executors.concat(NullExecutionStrategy), executor => executor.canExecute(command)) | ||
.then(applicableExecutors => new applicableExecutors[0](invocation, command).startExecution()); | ||
return Utils.filterWithPromising( | ||
this.executors.concat(NullExecutionStrategy), | ||
executor => executor.canExecute(command)) | ||
.then(applicableExecutors => new applicableExecutors[0](invocation, command).startExecution() | ||
); | ||
} | ||
} | ||
|
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,10 @@ export default class Utils { | |
}); | ||
} | ||
|
||
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.