Skip to content
This repository was archived by the owner on Jan 26, 2022. It is now read-only.

Refactor the use of the editor finder #473

Closed
jtpio opened this issue Jun 17, 2020 · 3 comments
Closed

Refactor the use of the editor finder #473

jtpio opened this issue Jun 17, 2020 · 3 comments
Assignees
Milestone

Comments

@jtpio
Copy link
Member

jtpio commented Jun 17, 2020

Glancing at the code, it looks like the EditorFinder (used to find editors matching the temporary cells) is passed all the way down to the EditorHandler here:

this._editorFinder = options.editorFinder;

And used only once here:

void this._debuggerService.updateBreakpoints(
this._editor.model.value.text,
breakpoints,
this._path,
this._editorFinder
);

This makes editor handlers very coupled to the EditorFinder.

Moreover the editor finder is passed to the NotebookHandler here:

debugger/src/handler.ts

Lines 215 to 219 in a056c28

this._handlers[widget.id] = new NotebookHandler({
debuggerService: this._service,
widget: widget as NotebookPanel,
editorFinder: this._editorFinder
});

But not to the ConsoleHandler and FileHandler:

debugger/src/handler.ts

Lines 221 to 232 in a056c28

case 'console':
this._handlers[widget.id] = new ConsoleHandler({
debuggerService: this._service,
widget: widget as ConsolePanel
});
break;
case 'file':
this._handlers[widget.id] = new FileHandler({
debuggerService: this._service,
widget: widget as DocumentWidget<FileEditor>
});
break;

Refactoring this should help fix #471 and #414, as a follow-up to #454 (review)

@jtpio jtpio added this to the 0.3.0 milestone Jun 17, 2020
@jtpio
Copy link
Member Author

jtpio commented Jun 17, 2020

Another point to refactor is the use of the non-optional focus parameter as mentioned in #454 (comment):

find(
debugSessionPath: string,
source: string,
focus: boolean
): IIterator<CodeEditor.IEditor> {

@KrzysztofSikoraCodete KrzysztofSikoraCodete self-assigned this Jun 18, 2020
@KrzysztofSikoraCodete
Copy link
Contributor

Good point. We definitely pass the editor-finder object too deeply.

KrzysztofSikoraCodete added a commit to KrzysztofSikoraCodete/debugger that referenced this issue Jun 18, 2020
afshin pushed a commit to KrzysztofSikoraCodete/debugger that referenced this issue Jul 10, 2020
@jtpio
Copy link
Member Author

jtpio commented Jul 13, 2020

Fixed by #474.

@jtpio jtpio closed this as completed Jul 13, 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

No branches or pull requests

2 participants