-
Notifications
You must be signed in to change notification settings - Fork 30k
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
Add Debug Output Copy All command : Fixes #27079 #28197
Conversation
constructor() { | ||
super({ | ||
id: 'repl.action.copyall', | ||
label: nls.localize('actions.repl.copyall', "Debug Copy All"), |
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.
I would prefer the title to be "Debug: Console Copy All", that way it nicely aligns with the other debug actions and has a more clear name
label: nls.localize('actions.repl.copyall', "Debug Copy All"), | ||
alias: 'Debug Copy All', | ||
precondition: debug.CONTEXT_IN_DEBUG_REPL, | ||
kbOpts: { |
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.
I think you can just leave out the kbOpts
and it should nicely compile, please try that
@@ -230,6 +232,19 @@ export class Repl extends Panel implements IPrivateReplService { | |||
this.layout(this.dimension); | |||
} | |||
|
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.
I do not like code duplication, can you please look into removing the CopyAllAction
from electronDebugActions.ts
. I believe that we can now always use this action instead, can you please try that?
@cleidigh great work. I have added some comments directly in the commit which I would like you to address. Apart from that did you try this out, does it work nicely? |
@isidorn |
@isidorn |
@cleidigh great for the two tweaks, feel free to push those. Though after pushing those two twearks we can merge it in and I can look into the last step of combining the two actions. And yeah calling the other one via |
@isidorn |
@isidorn
export class CopyAllAction extends Action {
} bugs me I can't figure it out !! |
@cleidigh thanks a lot for trying it out, great work! Let's merge this in and then I will try to remove code duplication |
@cleidigh this turned out to be more complicated than expected. |
@isidorn |
@cleidigh will fix it once I find time, currently working on multi-root features. |
@isidorn
Global CopyAll Debug output