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

Add Debug Output Copy All command : Fixes #27079 #28197

Merged
merged 2 commits into from
Jun 12, 2017

Conversation

cleidigh
Copy link
Contributor

@cleidigh cleidigh commented Jun 7, 2017

@isidorn
Global CopyAll Debug output

@isidorn isidorn self-assigned this Jun 7, 2017
@isidorn isidorn added this to the June 2017 milestone Jun 7, 2017
constructor() {
super({
id: 'repl.action.copyall',
label: nls.localize('actions.repl.copyall', "Debug Copy All"),
Copy link
Contributor

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: {
Copy link
Contributor

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);
}

Copy link
Contributor

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?

@isidorn
Copy link
Contributor

isidorn commented Jun 8, 2017

@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?

@cleidigh
Copy link
Contributor Author

cleidigh commented Jun 8, 2017

@isidorn
made the two tweaks, compiles and executes fine.
I was thinking the same thing about using the same function
I'll work on that.

@cleidigh
Copy link
Contributor Author

cleidigh commented Jun 9, 2017

@isidorn
didn't make much progress yet.
I somehow have to coerce the action registration for the ContextMenu IAction
actions.push(new CopyAllAction(CopyAllAction.ID, CopyAllAction.LABEL, tree));
Should be aligned with the ieditor action call
class ReplCopyAllAction extends EditorAction {
the signatures and call methods are different enough that I have not figured out
the right way to do it other than the action: calling the other one which I don't think is the right answer.

@isidorn
Copy link
Contributor

isidorn commented Jun 9, 2017

@cleidigh great for the two tweaks, feel free to push those.
As for the action duplication, I do not understand why in the replViewer getSecondayActions you can not simply add a new instance of your new EditorAction since EditorActions are also IActions.

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 commandService we can leave as plan b

@cleidigh
Copy link
Contributor Author

cleidigh commented Jun 9, 2017

@isidorn
I tried doing the instantiation. I can't get an acceptable signature.
I think the editor action with the @EditorAction decorator might be the problem.
pushed the other two tweaks. I'll continue to work on the duplication removal.

@cleidigh
Copy link
Contributor Author

@isidorn
well I'm stymied.
I have tried several different approaches,

  • using the service successor in copy all action - type visibility problem - could not get to compile
  • placing the copy all action in repl.ts and it calling the service successor - create some sort of Loop
  • using the command service to call the editor action - compiles and runs, the function is called
    but does not work

export class CopyAllAction extends Action {
static ID = 'workbench.debug.action.copyAll';
static LABEL = nls.localize('copyAll', "Copy All");
private _commandService: ICommandService;

constructor(id: string, label: string,
	@ICommandService commandService: ICommandService
	) {
	super(id, label);
	this._commandService = commandService;
}

public run(): TPromise<any> {
	this._commandService.executeCommand('repl.action.copyall');
	console.log('CopyAll eda');
	return TPromise.as(null);
}

}

bugs me I can't figure it out !!

@isidorn
Copy link
Contributor

isidorn commented Jun 12, 2017

@cleidigh thanks a lot for trying it out, great work! Let's merge this in and then I will try to remove code duplication

@isidorn isidorn merged commit 0ace0d0 into microsoft:master Jun 12, 2017
isidorn added a commit that referenced this pull request Jun 12, 2017
@isidorn
Copy link
Contributor

isidorn commented Jun 12, 2017

@cleidigh this turned out to be more complicated than expected.
I have pushed some minor polish on top of your PR, however I was also running into the same issues as you did while you tried to reduce code duplicaton.
In your case the command does not get exectued from the context menu because it's precondition is not met.

@cleidigh
Copy link
Contributor Author

@isidorn
looks good
so you're just leaving the duplicate code for now?

@isidorn
Copy link
Contributor

isidorn commented Jun 15, 2017

@cleidigh will fix it once I find time, currently working on multi-root features.
Thanks for understandgin

@cleidigh cleidigh deleted the debug-copy-all/cmd branch July 14, 2017 19:16
@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.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants