Skip to content
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

Vo/active window changes #15

Merged
merged 4 commits into from Mar 6, 2019
Merged

Vo/active window changes #15

merged 4 commits into from Mar 6, 2019

Conversation

vanesa
Copy link
Contributor

@vanesa vanesa commented Mar 6, 2019

WIP: Use activeWindowChanges to keep track changes and ensure window accessibility.

filter((editor): editor is sourcegraph.CodeEditor => editor !== undefined)
)

// When the configuration or current file changes, publish new decorations.

Choose a reason for hiding this comment

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

Suggested change
// When the configuration or current file changes, publish new decorations.
// When the active editor changes, publish new decorations.

editor.setDecorations(decorationType, decorations)
})
)
}
}
}
sourcegraph.workspace.onDidOpenTextDocument.subscribe(() =>

Choose a reason for hiding this comment

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

You won't be needing this anymore.

const decorationType = sourcegraph.app.createDecorationType()
setTimeout(() => editor.setDecorations(decorationType, decorations), 200)

if (sourcegraph.app.activeWindowChanges) {

Choose a reason for hiding this comment

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

This block looks fine, but it should not be contained in the body decorateEditors(). decorateEditors() is also probably useless now. You probably want something like this:

function decorateEditor(editor: sourcegraph.CodeEditor): void {
    // decoration logic goes here
}

export function activate(context: sourcegraph.extension.Context): void {
    // declare `activeEditor` above
    context.subscriptions.add(
        activeEditor.subscribe(decorateEditor)
    )
}

STATSD_PATTERN.lastIndex = 0 // reset
function decorateEditor(editor: sourcegraph.CodeEditor): void {
const decorations: sourcegraph.TextDocumentDecoration[] = []
const decorationType = sourcegraph.app.createDecorationType()

Choose a reason for hiding this comment

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

You'll want to create the decorationType at the top level, otherwise you may get duplicate decorations.

const decorationType = sourcegraph.app.createDecorationType()
for (const [i, line] of editor.document.text!.split('\n').entries()) {
let m: RegExpExecArray | null
do {

Choose a reason for hiding this comment

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

This do {} while {} feels very unidiomatic. Why do you need it? Can't you simply do the following:

const match = line.match(STATSD_PATTERN)
if (match) {
    // stuff
}

(string.match(regex) returns all matches of a global regExp as an array)

Choose a reason for hiding this comment

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

Also, even if you're working on the assumption that there are several matches per line, the decorations you add on the same line will be indistinguishable from each other (they will all read ' View metric (Datadog) » ')

}

export function activate(context: sourcegraph.ExtensionContext): void {
const activeEditor = from(sourcegraph.app.activeWindowChanges).pipe(

Choose a reason for hiding this comment

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

This will throw if sourcegraph.app.activeWindowChanges is undefined (which, again, will only happen on < 3.1 instances). Move it into the if block below.

@vanesa vanesa marked this pull request as ready for review March 6, 2019 12:21
@vanesa vanesa merged commit 86e66ef into master Mar 6, 2019
@vanesa vanesa deleted the vo/activeWindowChanges branch March 6, 2019 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants