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.

Sign up for GitHub

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

Jump to bottom

inline values as decorators when debugging #16129

Merged
merged 1 commit into from
Jan 13, 2017
Merged

inline values as decorators when debugging #16129

merged 1 commit into from
Jan 13, 2017

Conversation

nojvek
Copy link
Contributor

@nojvek nojvek commented Nov 28, 2016 •
edited
Loading

  1. Write Tests.
  2. Figure out how to get words from lines.
  3. Figure out how to stop flashing on stepping because onDidContinue passes in null stack frame everytime
  4. Figure out how to registerDecorator from public API.
  5. setDecorators afterText doesn't work in python on lines with trailing ':'
  6. Merge as single commit
  7. Add screenshot

image

@nojvek
Copy link
Contributor Author

nojvek commented Nov 28, 2016

@isidorn Please take a look at the first iteration.

@isidorn isidorn added this to the November 2016 milestone Nov 28, 2016
@isidorn isidorn mentioned this pull request Nov 28, 2016
Closed
@isidorn
Copy link
Contributor

isidorn commented Nov 28, 2016

I will do a code review and provide feedback in the code a bit later today, let me first try to answer your questions

  1. Awesome - might be a bit tricky though
  2. Just checked and yes there is no nice API to get you all words on a line. As you figured out already you need to use a regex, however you should use a word regex for the particular language you are in. You should get the regex using this method. Here is a sample usage
  3. Instead of using editor.removeDecorations and editor.addDecorations you need to use editor.deltaDecorations and probably keep track on your side of all the decorations from a previous state. DebugEditorModelManager is doing something similar now with breakpoints here
  4. Why do you want to register a decorator from a public api? This feature will live in the vsccode core repo
  5. No clue about that, we can investigate if it is a bug in the editor

@isidorn
Copy link
Contributor

isidorn commented Nov 28, 2016

  1. However if your name regex is more precise than the language word regex then we should probably use that

  2. Here is a public api to set decorations in case you need it for some other use case

@isidorn
Copy link
Contributor

isidorn commented Nov 29, 2016

@nojvek just to let you know that next week we have an endgame week - which means we take no new features. So it would be cool if we would try to land this feature together sometime this week. I have time on Thursday european time so I could try to wrap it up if something is still needed to be done then. Hope that works for you.

@nojvek
Copy link
Contributor Author

nojvek commented Nov 29, 2016 via email

@nojvek
Copy link
Contributor Author

nojvek commented Nov 29, 2016 via email

@isidorn
Copy link
Contributor

isidorn commented Nov 29, 2016

Not sure if we can use it in the core
@jrieken is the right person to answer this

@jrieken
Copy link
Member

jrieken commented Nov 29, 2016

re #16129 (comment) - Where do you need those? In an extension and the main-body of source code?

@isidorn
Copy link
Contributor

isidorn commented Nov 29, 2016

@jrieken main-body of source code

@nojvek
Copy link
Contributor Author

nojvek commented Nov 29, 2016 via email

@jrieken
Copy link
Member

jrieken commented Nov 29, 2016

For time being I've added a map-set.d.ts in the typings folder.

That is not allowed 🙅‍♂️ We have some technical debt there but in essence it would make ES6 Maps/Sets also available in the editor/base layer which we program against older runtime targets that don't support ES6. You might have a local version of those definitions, we have done that in a few places but it is not nice. Also it seems you don't need that.

The reason why I'm having hard time with using {} as map is that, I can't
quite set hasOwnProtype or prototype as keys, since they are already
defined properties. A scope variable could be this special object keys.

The fix for that is to not use {} but Object.create(null) or the utils defined in collections.ts

@nojvek
Copy link
Contributor Author

nojvek commented Nov 29, 2016 via email

@nojvek
Copy link
Contributor Author

nojvek commented Nov 29, 2016 via email

@isidorn
Copy link
Contributor

isidorn commented Nov 29, 2016

That sounds like a decent workaround for now. So +1
I believe tokenization is already dealing with words so it would be cool if you could just get tokens and not do any additional word magic on a line (which you are currently doing).

@nojvek
Copy link
Contributor Author

nojvek commented Nov 29, 2016 via email

@isidorn
Copy link
Contributor

isidorn commented Nov 29, 2016

@nojvek I would not move that to the TextEditorModel before I check with @alexandrudima. Will talk to him tomorrow in the office.

You can add a listener to the editor on changeModel. Example

@isidorn
Copy link
Contributor

isidorn commented Nov 30, 2016

Ok I have discussed this with @alexandrudima and there are concerns

  • The current implementation of putting decorations after text is suboptimal and can hurt performance
  • Going through each word on a line can be very performant for very long lines
  • What do we do for people that have word wrapping enabled. Via that setting they have specifically enabled lines not to overflow over some border.

Due to the above, this feature will be under a setting which by default is false. Once we have no more performance concerns we can lift this flag. @nojvek you do not have to worry about introducing this flag, I can just put it on top once we polish this pr.

Also we need to put a limit on the line size and just do not compute anything for very long lines. Since currently we are matching the words on a line to a set of variables, we could also do it the other way around - match variables across a line. However this strategey does not work best for short lines which are the most common in practice. Thus I believe for starters we should just ignore long line.

As for the word wrap issue - that is something we need to think about. Might not require immediate action.

In a perfect world the debug extension would have a better ast understanding of the file - this knowledge would come from a language server but we currently do not have this. @weinand might know better if we plan to investigate into this any time in the future

Due to all of the above we do not have to rush this feature in for November. But let's see how it goes.

@nojvek
Copy link
Contributor Author

nojvek commented Nov 30, 2016 via email

@isidorn
Copy link
Contributor

isidorn commented Dec 1, 2016

All those constraints make sense but we should introduce another one - the length of the editor line that is being processed. Because if the lenght of that line is quite large you will still go through each word and try to find it in the variables view, so some constraint needs to be added there.
We can handle word wrapping later, not for version 1.

fyi I am on vacation on Friday but I plan to code review, merge this feature and try to wrap it up on Monday.

@nojvek
Copy link
Contributor Author

nojvek commented Dec 1, 2016 via email

@nojvek nojvek changed the base branch from isidorn/inlineValues to master December 2, 2016 22:22
@nojvek
Copy link
Contributor Author

nojvek commented Dec 4, 2016

I've moved most of helper functions to debugInlineDecorators.ts where the core of logic really happens. Added 100% line, branch and function coverage for that file.

I've tested on a bunch of different project types and it looks good so far. I've added a screenshot.

Let me know if you need more changes. Its a nice sunny sunday, so I'll get back to this on Monday evening. Can't wait for this to get into nightly releases.

@nojvek nojvek changed the title Initial working prototype of inline values Working inline values as decorators when debugging Dec 4, 2016
@nojvek nojvek changed the title Working inline values as decorators when debugging inline values as decorators when debugging Dec 4, 2016
@isidorn
Copy link
Contributor

isidorn commented Dec 5, 2016

@nojvek great work! And nice that you tested it in different projects!

I have added comments in the commit directly.

Let's move this feature out to the next release because there is no need for the rush and I would love the insiders to self host on this feature for a couple of weeks. This will also give us time to nicely adress the issues I brought up in the review.

@isidorn isidorn added this to the January 2017 milestone Dec 5, 2016
@nojvek
Copy link
Contributor Author

nojvek commented Dec 5, 2016 via email

@isidorn
Copy link
Contributor

isidorn commented Dec 5, 2016

@nojvek you should be able to see them now, I made a mistake of not publishing them. Sorry for the misunderstanding!

nojvek
nojvek commented Dec 6, 2016
View reviewed changes
Copy link
Contributor Author

@nojvek nojvek left a comment

Choose a reason for hiding this comment

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

Fixed review feedback

@isidorn
Copy link
Contributor

isidorn commented Dec 6, 2016

@nojvek thanks for your replies - we are in the middle of the endgame so I will reply to your comments in a couple of days once the bugfixing fuss is over. I also need to consult with @alexandrudima for the editor side of things concerning some of the questions you have raised.

@nojvek
Copy link
Contributor Author

nojvek commented Dec 6, 2016 via email

@isidorn
Copy link
Contributor

isidorn commented Dec 6, 2016

@nojvek feel free to ignore my comments from couple of days ago which I accidently only released yesterday

@isidorn
Copy link
Contributor

isidorn commented Dec 9, 2016

@nojvek added some comments. Seems like it is in a good state, I would lke to merge it in some time next week and then I can look into improving some of the issues we raised in the discussion (end of line decorations, using timeouts, creating a tokenizer for tests). Most of the team is still busy with the endgame so I will sync with them next week for input on improving implentation of decorations

@nojvek
Copy link
Contributor Author

nojvek commented Dec 9, 2016 via email

@nojvek
Copy link
Contributor Author

nojvek commented Dec 9, 2016 via email

@isidorn
Copy link
Contributor

isidorn commented Dec 28, 2016

Hi @nojvek I was looking into mergin this in, the following would still be needed in order for me to do the merge

  • As @aeschli mentioned here no new editor api needs to be added. So please use existing API
  • Changes in the codeEditorServiceImpl.ts need to be moved to a seperate PR
  • merge your branch with latest from master and resolve conflicts

Other things I can polish up once we merge it in.

Thanks a lot!

@nojvek
Copy link
Contributor Author

nojvek commented Dec 28, 2016 via email

@aeschli
Copy link
Contributor

aeschli commented Dec 30, 2016

My suggestion is that you register your decoration type using ICodeEditorService.registerDecorationType

@isidorn
Copy link
Contributor

isidorn commented Jan 10, 2017

ping @nojvek just to check if there are some updates here? Just fyi If you are busy I can also work on this.

@nojvek nojvek mentioned this pull request Jan 10, 2017
Merged
@nojvek
Copy link
Contributor Author

nojvek commented Jan 10, 2017

@aeschli , codeEditorService is a private variable of codeEditor. It is not exposed. This is exactly what I am trying to do. ICodeEditor.registerDecorationType ends up calling ICodeEditorService.registerDecorationType

Updated PR, as you recommend I am using ICodeEditorService.registerDecorationType (<any>this.editor)._codeEditorService.registerDecorationType. I get a feeling that you are very much against touching the code editor api.

I feel its worse solution than what we had before but gets the job done until vscode exposes a nicer api. I'm really not sure what you want me to do here. 😞

@aeschli
Copy link
Contributor

aeschli commented Jan 11, 2017

@nojvek To get hold of the codeEditorService, use service injection in the class you are:
Add
@ICodeEditorService codeEditorService: ICodeEditorService
to the constructor of the DebugEditorContribution

@nojvek
Copy link
Contributor Author

nojvek commented Jan 11, 2017 via email

@isidorn
Copy link
Contributor

isidorn commented Jan 11, 2017

@nojvek just checked the PR and no other major changes are needed - except the code injection Martin mentioned. Once that is in I will merge this in.
Some polish work will be needed on top (adding setting, some refactoring, trying not to use timeouts) but that we will handle on our side.

Great work! 🍻

@nojvek
Inline values as decorators when debugging
4077947 
For perf reasons we only process max 100 scopes, lines longer than 500 chars are not processed, maximum decorator length is 150 chars and we store a wordRangesMap for current editor model for fast lookup.

Since continue, step over, step in will send null stack frame followed quickly by a valid frame. We debounce removeDecorators.

100% LFB code coverage for debugInlineDecorators.ts.

Tested inline decorators with python and javascript projects.

Adding codeEditorService to debugEditorContribution through dependency injection.
@nojvek
Copy link
Contributor Author

nojvek commented Jan 12, 2017

Done.

Just curious: Any chance someone can give me a brief overview of how vscode uses es6 decorators to make dependency injection work?

		@IDebugService private debugService: IDebugService,
		@IContextMenuService private contextMenuService: IContextMenuService,
		@IInstantiationService private instantiationService: IInstantiationService,
		@IContextKeyService contextKeyService: IContextKeyService,
		@ICommandService private commandService: ICommandService,
		@ICodeEditorService private codeEditorService: ICodeEditorService,
		@ITelemetryService private telemetryService: ITelemetryService

Am I correct to assume that the services are all singletons? i.e each of them is only instantiated once and then stored in a service registry somewhere ?

@editorContribution
export class DebugEditorContribution implements IDebugEditorContribution {

This is like adding the class instance to another registry as editor extensions/contributors ? In this sense if I create another class

@editorContribution
export class SyncEditorContribution implements IDebugEditorContribution {

vscode will instantiate my contribution, add to contributions registry and call its handler functions when editor change happens right? So with @editorContribution its possible to build functionality in vscode that syncs the editor with a peer or web browser for a collaborative interview?

@isidorn
Copy link
Contributor

isidorn commented Jan 13, 2017

@nojvek thanks! I will merge it in today.
As for the code injection, yes every service is instantiated exactly once and more details can be found here

@isidorn isidorn merged commit df2d1d2 into microsoft:master Jan 13, 2017
@isidorn
Copy link
Contributor

isidorn commented Jan 13, 2017

@nojvek added setting, started using set & map (since it is now ok to do this - we no longer support IE10) and did some polish on top. I also plan to look into restructuring the debugInlineValues.ts so it is a class and not a bunch of helper methods. I am also considering not showing inline values for lines after the current line that is focussed.
6091d5d
24c9eba
db334b2

@nojvek
Copy link
Contributor Author

nojvek commented Jan 13, 2017 via email

@isidorn
Copy link
Contributor

isidorn commented Jan 13, 2017 •
edited
Loading

This can be set in settings > "debug.inlineValues": true.
You can set it and try it out in the next insider build. And it will be a part of the january stable release

@luminaxster
Copy link

luminaxster commented Feb 4, 2017 •
edited
Loading

Wow!! Awesome work!
Do you have some feedback(than can be shared) from the users that have used the feature? I highly recommend this reading, if you haven't already, it goes "inline" with this feature. Let me know if I can be help, we are working on researching on and developing programming tools, too.

@weinand
Copy link
Contributor

weinand commented Feb 5, 2017

@luminaxster since this feature made its first appearance in VS Code 1.9 (3 days ago) we do not yet have statistics. In addition this feature is not enabled by default because we are still working on it, so we do not expect widespread acceptance in this release.

@vbfox vbfox mentioned this pull request Aug 26, 2017
Merged
6 tasks
@gregg-miskelly gregg-miskelly mentioned this pull request Apr 21, 2018
Open
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@isidorn isidorn isidorn requested changes

@aeschli aeschli aeschli left review comments

Assignees

@isidorn isidorn

Labels
None yet
Projects
None yet
Milestone
January 2017
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@nojvek @isidorn @jrieken @aeschli @luminaxster @weinand @msftclas