-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Visual Mode + Rudimentary Operators #144
Changes from all commits
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,4 +1,4 @@ | ||
out | ||
node_modules | ||
typings | ||
*.swp | ||
*.sw? |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,140 @@ | ||
"use strict"; | ||
|
||
import * as _ from 'lodash'; | ||
|
||
import { ModeName, Mode } from './mode'; | ||
import { Motion} from './../motion/motion'; | ||
import { Position } from './../motion/position'; | ||
import { Operator } from './../operator/operator'; | ||
import { DeleteOperator } from './../operator/delete'; | ||
import { ModeHandler } from './modeHandler.ts'; | ||
import { ChangeOperator } from './../operator/change'; | ||
|
||
export class VisualMode extends Mode { | ||
/** | ||
* The part of the selection that stays in the same place when motions are applied. | ||
*/ | ||
private _selectionStart: Position; | ||
|
||
/** | ||
* The part of the selection that moves. | ||
*/ | ||
private _selectionStop : Position; | ||
private _modeHandler : ModeHandler; | ||
|
||
private _keysToOperators: { [key: string]: Operator }; | ||
|
||
constructor(motion: Motion, modeHandler: ModeHandler) { | ||
super(ModeName.Visual, motion); | ||
|
||
this._modeHandler = modeHandler; | ||
this._keysToOperators = { | ||
// TODO: use DeleteOperator.key() | ||
|
||
// TODO: Don't pass in mode handler to DeleteOperators, | ||
// simply allow the operators to say what mode they transition into. | ||
'd': new DeleteOperator(modeHandler), | ||
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. 👍 Awesome, this is the pattern in which I was envisioning as well but never got around to it. 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. And then later we turn on |
||
'x': new DeleteOperator(modeHandler), | ||
'c': new ChangeOperator(modeHandler) | ||
}; | ||
} | ||
|
||
shouldBeActivated(key: string, currentMode: ModeName): boolean { | ||
return key === "v"; | ||
} | ||
|
||
async handleActivation(key: string): Promise<void> { | ||
this._selectionStart = this.motion.position; | ||
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. This is great first cut. Would it be worthwhile to move selection to it's own class? That is what I had envisioned for the 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. Hmm... Well, to start, I still don't fully grasp the concept of This seems like a good place to discuss some of my thoughts about structuring this code. My idea is to pass around a state object which could look like this (very rough)
All motions would just take a state and return a new state object. When you pressed a key the object would be passed through some transformations and eventually you'd get a new one. When the process is done you would then update VSCode to show the new selections and cursor positions. The idea is to make stuff like macros/number commands easier - if, say, the user goes into visual mode, makes 2w into a macro and does 100@@, we don't redraw the selection 200 times. This would also make proper undo (e.g., stepping back through full actions rather than what we have now) way more straightforward, since actions are nicely discretized. Anywho, I still have to dig more into the source of Vintageous and VSVim before I feel confident about making sweeping changes like this, but those are my general thoughts. What do you think? (I also am a fan of separating out the motions into classes like VSVim and Vintageous do - otherwise the Motion and Position classes would eventually become monstrous!) 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. Yeah, I think that's what I had initially intended for 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. Ah... that makes more sense now! |
||
this._selectionStop = this._selectionStart; | ||
|
||
this.motion.select(this._selectionStart, this._selectionStop); | ||
} | ||
|
||
handleDeactivation(): void { | ||
super.handleDeactivation(); | ||
|
||
this.motion.moveTo(this._selectionStop.line, this._selectionStop.character); | ||
} | ||
|
||
/** | ||
* TODO: | ||
* | ||
* Eventually, the following functions should be moved into a unified | ||
* key handler and dispatcher thing. | ||
*/ | ||
|
||
private async _handleMotion(): Promise<boolean> { | ||
let keyHandled = false; | ||
let keysPressed: string; | ||
|
||
for (let window = this.keyHistory.length; window > 0; window--) { | ||
keysPressed = _.takeRight(this.keyHistory, window).join(''); | ||
if (this.keyToNewPosition[keysPressed] !== undefined) { | ||
keyHandled = true; | ||
break; | ||
} | ||
} | ||
|
||
if (keyHandled) { | ||
this._selectionStop = await this.keyToNewPosition[keysPressed](this._selectionStop); | ||
|
||
this.motion.moveTo(this._selectionStart.line, this._selectionStart.character); | ||
|
||
/** | ||
* Always select the letter that we started visual mode on, no matter | ||
* if we are in front or behind it. Imagine that we started visual mode | ||
* with some text like this: | ||
* | ||
* abc|def | ||
* | ||
* (The | represents the cursor.) If we now press w, we'll select def, | ||
* but if we hit b we expect to select abcd, so we need to getRight() on the | ||
* start of the selection when it precedes where we started visual mode. | ||
*/ | ||
|
||
// TODO this could be abstracted out | ||
if (this._selectionStart.compareTo(this._selectionStop) <= 0) { | ||
this.motion.select(this._selectionStart, this._selectionStop); | ||
} else { | ||
this.motion.select(this._selectionStart.getRight(), this._selectionStop); | ||
} | ||
|
||
this.keyHistory = []; | ||
} | ||
|
||
return keyHandled; | ||
} | ||
|
||
private async _handleOperator(): Promise<boolean> { | ||
let keysPressed: string; | ||
let operator: Operator; | ||
|
||
for (let window = this.keyHistory.length; window > 0; window--) { | ||
keysPressed = _.takeRight(this.keyHistory, window).join(''); | ||
if (this._keysToOperators[keysPressed] !== undefined) { | ||
operator = this._keysToOperators[keysPressed]; | ||
break; | ||
} | ||
} | ||
|
||
if (operator) { | ||
if (this._selectionStart.compareTo(this._selectionStop) <= 0) { | ||
await operator.run(this._selectionStart, this._selectionStop.getRight()); | ||
} else { | ||
await operator.run(this._selectionStart.getRight(), this._selectionStop); | ||
} | ||
} | ||
|
||
return !!operator; | ||
} | ||
|
||
async handleKeyEvent(key: string): Promise<void> { | ||
this.keyHistory.push(key); | ||
|
||
const wasMotion = await this._handleMotion(); | ||
|
||
if (!wasMotion) { | ||
await this._handleOperator(); | ||
} | ||
} | ||
} |
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.
Nit: I'd prefer
forEach
:)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.
Why? IMO,
for of
is much better thanforEach
- it reads way nicer, and you can dofor (const ... of)
which is not possible withforEach
. (Are you sure you haven't confused it withfor in
?)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.
🙆♀️ Not that strongly attached to it so let's just go with
for of
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.
Haha. The closer I get to bike shedding, the more argumentative I get, or something. 🌈