This change to package.json needs to be reverted. Before making any changes to package.json, you need to make sure to undo the changes that occur when re-write the package.json with the runtime version of the file. The make your changes and stage the changes before running the extension again.

Thanks for catching that! Is there any way we can automate this process for the future? I would like to investigate the possibility if it's worthwhile.

I'm going to add a new option to our CMakeLists.txt for the language server that will point to your local clone of this instance and copy it into a disposable location so you don't have to run the extension on top of the local repository. (this has been bugging me as well) :)

Excellent! I will revert the changes as per Sean's comment and keep my eyes peeled for that change in the future.

What does this TODO mean?

This is a TODO that I should have investigated earlier. Most operations within client.ts operate via vscode.Uri, whereas this method only compares the absolute file path via string. @bobbrow opinion on the matter?

You should probably send the uri in the JSON message instead of the filename.

rgba(255,0,255,0.5) [](start = 20, length = 19)

it looks like we still have the test color in here. We need to remember to make a light-themed and dark-themed gray color here,

Most definitely. I will add different coloration based on light/dark, and I will keep note of it as a discussion point when we talk as a group, and when we find out more about the possibility of opacity.

let textEditorDecoration = vscode.window.createTextEditorDecorationType(renderOptions); [](start = 8, length = 87)

We probably only need to create this object once when the class is first initialized.

editor.setDecorations [](start = 8, length = 21)

with each notification, I think we'll need to save the ranges that come in. This way we can clear out the old decorations before setting the new ones.

It looks like you might also be able to call dispose on the TextEditorDecorationType to remove all decorations associated with that object. If that's the case, then my previous comment about creating the TextEditorDecorationType only once would be invalid. You might be able to just keep a reference to the previous TextEditorDecorationType and dispose it before creating a new one and assigning it to the new ranges.

In reply to: 163304083 [](ancestors = 163304083)

That sounds like a fairly tidy way to manage the decoration types. I will make sure to investigate that possibility first!

This is only needed for OSX. Please do not include this in the check-in.

I think it would be ok to add this to the .gitignore. @pieandcakes?

@grdowns Was this added on purpose because you have a requirement for Mono now?

No, I'm pretty sure we don't have a requirement for Mono with this change.

This license was auto-generated. I believe Sean is correct and it will be removed.

git diff says this is a whitespace difference, specifically a carriage return (^M). Undo.

I will make sure to undo this change. Do we have any tools for Git to automatically ignore carriage return changes? I feel that checking for carriage returns manually with each commit is wasteful.

You could try git diff -w | git apply --cached --ignore-whitespace. However, I would do a quick glance atgit diff before creating the PR.

The common things to look out for with this repo is any changes that you did not make or files that you did not add should not be included with your changes.

Thanks! I appreciate the advice + tip. You are totally right about the automatically generated changes and I will make sure to be diligent in scrutinizing these diffs pre-PR.

Is this a required change?

This should go in at some point though it's not required to be part of this change. This repo is out of sync with the language server.

@grdowns Was this added on purpose because you have a requirement for Mono now?

I think you need to undo this file change. I dont' think we should be changing our activation from *.

Please move this up to the top of the class definition with the other member variables. You might also want to rename it something more specific so we don't lose track of it. e.g. inactiveRegionsDecoration.

Thinking about this a bit more... Since the inactiveRegionsDecorarion is linked to the Client, this will end up accidentally deleting highlights from files when you have more than one file open in the editor. You're going to want to have one of these for each uri that you encounter.

And then you will want to add disposal code to DefaultClient.dispose

Reading the helptext on outFiles, looks like its a glob pattern to specify js files. do we want to set this to "${workspaceRoot}/out/src/**/*.js" ?

nit: Can you make this an interface?
valuePair[0].dispose(); is not as readable as valuePair.decoration.dispose();
and valuePair[1] = params.ranges; vs valuePair.ranges = params.ranges;

Absolutely! I considered doing so, but decided against it as it didn't seem worth polluting the file + namespace. I think that someone else requesting an interface is enough to rationalize doing it, however. I'll put it through!

need to guard against valuePair being undefined here.

Guard has been brought back in + using if (valuePair) instead of if (valuePair !== undefined)

nit: personal preference (being a Light user 😄): I built this change and am playing with it. This is a bit too dark to easily distinguish the inactive regions for me. Can you bump it up a bit? 175,175,175 looks a bit better to me.

Definitely! I don't expect these numbers to survive until the feature is complete

nit: Please move it closer to the other group of interfaces.

Captitalize

Why is this commented out?

it should be uncommented.

@bobbrow is correct. Apologies while I get used to the code review cycle. All tips and advice and nitpicks are welcome. 😄

nit: move comment above

Either do !valuePair or this.inactiveRegionsDecorations.has(params.uri)

!valuePair changes the meaning of the statement, and this.inactiveRegionsDecorations.has(params.uri) is (assumedly) as expensive as calling this.inactiveRegionsDecorations.get(params.uri) which while minor it would be best to avoid it. Is that the case?

no need to call .has. You can change it to if (valuePair) or just leave it as if (valuePair !== undefined). I think the 'truthy' compare argument for this case has been bolstered a bit by the recent regression, but I believe the API contract here is relatively well defined.

interface Map<K, V> {
    clear(): void;
    delete(key: K): boolean;
    forEach(callbackfn: (value: V, key: K, map: Map<K, V>) => void, thisArg?: any): void;
    get(key: K): V | undefined;
    has(key: K): boolean;
    set(key: K, value: V): this;
    readonly size: number;
}

Whoops, should have looked at the return values of get(). 🙈

-resolved-

I believe if (valuePair) is more idiomatic and as it does not change the meaning of the statement, I will substitute it in. Thanks

This should not be imported here. Only vscode should ever call this method.

That snuck in via autocomplete. Removed + noted for the future

return true

🤦‍♂️

Did you also want a reference compare or actual equality?
https://github.com/Microsoft/vscode/blob/69f9a3b8080f5e90a47743dda1a6dc3f55972be4/src/vs/vscode.d.ts#L422

Good catch! I was ignorant of the reference vs primitive comparison difference in this case. if (!e1 == e2) should be if (!e1.isEqual(e2)) as it will compare the ints inside each range's positions.

nit: comparing two things. Use "are" instead of "is" for the naming. :)

It's a stylistic choice for a method returning a boolean. But I see your point and I'll concede. 😆

This seems wrong. It will compare r1[0] against r2[1] and fail. If we are expecting the arrays to have the same ranges in the same order, this should be a single for-loop iterating from 0 to r1.length-1 (or r2.length-1 since they are equal at this point). Then it checks if r1[n] is equal to r2[n].

Are the arrays always going to have the Range in the same order?

If we are not guaranteedsame order, I think it should be e1 exists in e2, if it does not return false.