Connect Breakpoints widget with NoteBook #36

KsavinN · 2019-09-02T14:00:03Z

jtpio · 2019-09-04T13:01:07Z

src/breakpoints/index.ts

+
+  protected removeAllGutterBreakpoints(cell: CodeCell) {
+    const editor = cell.editor as CodeMirrorEditor;
+    editor.editor.getDoc().eachLine(line => {


Thinking out loud: I was wondering whether there was a way to avoid the editor.editor pattern and use the editor wrapper instead (probably not since we need to access the CodeMirror methods directly)

jtpio · 2019-09-04T13:03:00Z

src/breakpoints/index.ts

    super();

-    this.model = new Breakpoints.IModel(MOCK_BREAKPOINTS);
+    this.model = new Breakpoints.IModel([] as Breakpoints.IBreakpoint[]);


Is the type casting necessary here?

jtpio · 2019-09-04T13:04:23Z

src/breakpoints/index.ts

+  notebook: INotebookTracker;
+  previousCell: CodeCell;
+  previousLineCount: number;
+  cellsBreakpoints: any = {};


This needs a proper type instead of any.

jtpio · 2019-09-04T13:06:01Z

src/breakpoints/index.ts

+      }
+      this.previousLineCount = linesNumber;
+    }
+    // eage case for backspace line 2


what does this edge case look like?

If you set a breakpoint in line 1 and after that remove line 2 in editor gutter remove automatically breakpoint from line 1.

jtpio · 2019-09-04T13:11:07Z

src/breakpoints/index.ts

+    );
+
+    if (!breakpoint && !removeGutter) {
+      this.model.breakpoint = {


This looks like the model can only handle 1 breakpoint at a time. Maybe using a more explicit method such as addBreakpoint would be more appropriate?

jtpio · 2019-09-04T13:11:54Z

src/breakpoints/index.ts

+      const breakpoints = this.breakpoints.filter(
+        ele => ele.line !== breakpoint.line
+      );
+      this.breakpoints = [];


Is this line necessary?

jtpio · 2019-09-04T13:12:11Z

src/breakpoints/index.ts

+        ele.line = lineEnter <= ele.line ? ele.line + howMany : ele.line;
+        return ele;
+      });
+      this.breakpoints = [];


Same question as above

jtpio · 2019-09-04T13:16:21Z

src/debugger.ts

@@ -64,6 +70,10 @@ export namespace Debugger {
      return this._isDisposed;
    }

+    get notebook() {


This seems to return a NotebookTracker instead of Notebook (can lead to some confusion for users of this API)

afshin · 2019-09-09T12:25:25Z

src/breakpoints/index.ts

+  noteTracker: INotebookTracker;
+  previousCell: CodeCell;
+  previousLineCount: number;
+  cellsBreakpoints: { [id: string]: Breakpoints.IBreakpoint[] } = {};


Could we change the name of this to cellBreakpoints where cell is singular, please?

afshin · 2019-09-09T12:27:17Z

src/breakpoints/index.ts

+    });
+  }
+
+  removeListner(cell: CodeCell) {


typo: removeListner => removeListener

afshin · 2019-09-09T12:44:26Z

src/debugger.ts

@@ -45,14 +47,16 @@ export class Debugger extends BoxPanel {
 export namespace Debugger {
  export interface IOptions {
    connector?: IDataConnector<ReadonlyJSONValue>;
-
+    noteTracker?: INotebookTracker;


I'm not sure I understand the reasoning here. Why does the Debugger need a reference to any trackers? The pattern I had in mind was the extension to have the trackers and as those trackers change for it to modify the debugger's handle to individual text editors.

In actual branch, i working on a split this. I get your idea before, these changes I made after the suggestion of Jeremy. I guess tomorrow or on Wednesday I will merge changes to this branch.

Ah okay, great; I'll take a look whenever you're ready for that!

merge master

jtpio · 2019-09-26T11:47:45Z

Quickly tested this locally. The "Remove all Breakpoints" button doesn't remove the breakpoints from the cell gutters, but that can be done in a follow-up PR.

… afshin-debugger-ui

jtpio · 2019-09-30T15:26:57Z

src/breakpoints/index.ts

+
+    addBreakpoint(session: string, type: string, lineInfo: LineInfo) {
+      const breakpoint: Breakpoints.IBreakpoint = {
+        line: lineInfo.line,


This seems to return 0 for line number 1?

yes, good point. I will fix that

…er-ui

jtpio · 2019-10-01T12:45:31Z

src/breakpoints/index.ts

+        this.breakpoints = [];
+      } else {
+        const breakpoint = { ...this.breakpoints[0] };
+        var breakpoints: Breakpoints.IBreakpoint[] = [];


let would be better here:

Suggested change

var breakpoints: Breakpoints.IBreakpoint[] = [];

let breakpoints: Breakpoints.IBreakpoint[] = [];

…er-ui

src/index.ts

jtpio · 2019-10-01T15:23:23Z

src/index.ts

@@ -153,7 +220,10 @@ const tracker: JupyterFrontEndPlugin<IDebugger> = {
        }

        const widget = new MainAreaWidget({
-          content: new Debugger({ connector: state, id })
+          content: new Debugger({


Let's add the widget to the main area as well?

For now it's only added to the tracker.

I think we could do that. But I don't quite know what we can now display in that widget.

Not much for now indeed, but it's still good that it shows up somewhere.

Ok. I will change that and try to resolve bug which you found.

jtpio · 2019-10-01T15:58:55Z

Sometimes it seems like the breakpoints can't be set (after running the debugger:create command and refreshing the page).

(here the panel was added to the main area with app.shell.add(widget, 'main'))

afshin · 2019-10-02T17:58:10Z

I propose we merge this and iterate on it so that @jtpio and I can work off this branch.

afshin

Let's merge this and iterate.

JohanMabille · 2019-10-03T07:22:09Z

I can reproduce the bug reported by @jtpio in a deterministic way: I can never set any breakpoint (I have checked that the extension is correctly installed and enabled). Also for an unknown reason, the display of line numbers is disabled by default.

I've tested with this prototype (to see if I could quickly port a fix) I can set breakpoints, but only in cells that have not been executed yet.

NOTE: I'm testing with Google Chrome.

KsavinN · 2019-10-03T09:06:14Z

I can reproduce the bug reported by @jtpio in a deterministic way: I can never set any breakpoint (I have checked that the extension is correctly installed and enabled). Also for an unknown reason, the display of line numbers is disabled by default.

I've tested with this prototype (to see if I could quickly port a fix) I can set breakpoints, but only in cells that have not been executed yet.

NOTE: I'm testing with Google Chrome.

@JohanMabille Thank you for help and opinion. I will try to reproduce issues. "Also for an unknown reason, the display of line numbers is disabled by default." - I was thought that the default state of display line numbers is disabled.

JohanMabille · 2019-10-03T09:42:07Z

@KsavinN The bug I had was my fault, I didn't start the debugger with the "hidden" command. Now I can set breakpoints and reproduce the issue described by @jtpio

jtpio · 2019-10-03T11:33:42Z

package.json

@@ -52,6 +52,7 @@
    "@phosphor/coreutils": "^1.3.1",
    "@phosphor/disposable": "^1.2.0",
    "@phosphor/widgets": "^1.8.0",
+    "@types/codemirror": "0.0.76",


This should go to devDependencies

KsavinN added 4 commits August 29, 2019 15:42

some changes

c5b8d24

add setting breakpoint in UI

833b2cc

add notebooktracker to breakpoint wideget

e60f900

Merge branch 'notebook_lines' into debugger-ui

e6bd0dc

KsavinN added the work in progress label Sep 2, 2019

KsavinN added 4 commits September 2, 2019 16:14

debug for test

b3e1fa7

add method for update breakpoints after changedLines in Notebook

5ad3971

develope toogle remove all breakpoints

484108a

Merge branch 'notebook_lines' into debugger-ui

ec77927

KsavinN requested review from jtpio and afshin September 4, 2019 08:00

jtpio reviewed Sep 4, 2019

View reviewed changes

KsavinN added 3 commits September 6, 2019 16:13

changes

2b0ce1d

refactor , suggest by Jeremy

c37f31c

rename INoteTracker variable

01aa541

afshin reviewed Sep 9, 2019

View reviewed changes

src/breakpoints/index.ts Outdated

});

}

removeListner(cell: CodeCell) {

Copy link

Member

afshin Sep 9, 2019

Choose a reason for hiding this comment

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

typo: removeListner => removeListener

afshin reviewed Sep 9, 2019

View reviewed changes

KsavinN and others added 5 commits September 9, 2019 16:16

Merge pull request #8 from jupyterlab/master

8752e6f

merge master

add DebugerNotebookTracker

061feff

Merge branch 'master' into session

2d676fa

add breakpointservice

7dd3752

add service instance

47d0bb3

KsavinN and others added 6 commits September 27, 2019 13:21

refactoring, still work in progress

a053c95

Export debugger sidebar.

c8c3a85

next refactor

a2d10a6

Merge branch 'debugger-ui' of https://github.com/afshin/debugger into…

e6c6fa9

… afshin-debugger-ui

delete breakpointService , set sidebar in to model of Debuger

933fade

add Tracker for handlers between console/notebook

0297d8d

jtpio reviewed Sep 30, 2019

View reviewed changes

KsavinN and others added 5 commits October 1, 2019 09:30

fix line numbers for breakpoints

2915521

fix line numbers for breakpoints

85d59d9

Merge branch 'debugger-ui' of github.com:KsavinN/debugger into debugg…

f0f1186

…er-ui

fix edge cases

28bdc70

Add sessionChanged signal.

9355956

jtpio reviewed Oct 1, 2019

View reviewed changes

KsavinN added 3 commits October 1, 2019 15:15

set session only in debugger model

1baa7f4

set session only in debugger model

d7071cc

Merge branch 'debugger-ui' of github.com:KsavinN/debugger into debugg…

79cbf93

…er-ui

jtpio reviewed Oct 1, 2019

View reviewed changes

src/index.ts Show resolved Hide resolved

jtpio reviewed Oct 1, 2019

View reviewed changes

add debugger widget to main

6cd90a5

afshin approved these changes Oct 2, 2019

View reviewed changes

KsavinN merged commit 302c3f3 into jupyterlab:master Oct 3, 2019

jtpio reviewed Oct 3, 2019

View reviewed changes

JohanMabille mentioned this pull request Oct 3, 2019

Fixed debugger restore #46

Merged

jtpio removed the work in progress label Oct 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Connect Breakpoints widget with NoteBook #36

Connect Breakpoints widget with NoteBook #36

KsavinN commented Sep 2, 2019 •

edited by afshin

jtpio Sep 4, 2019

jtpio Sep 4, 2019

jtpio Sep 4, 2019

jtpio Sep 4, 2019

KsavinN Sep 9, 2019

jtpio Sep 4, 2019

jtpio Sep 4, 2019

jtpio Sep 4, 2019 •

edited

jtpio Sep 4, 2019

afshin Sep 9, 2019

afshin Sep 9, 2019

afshin Sep 9, 2019

KsavinN Sep 9, 2019

afshin Sep 9, 2019

jtpio commented Sep 26, 2019

jtpio Sep 30, 2019

KsavinN Oct 1, 2019

jtpio Oct 1, 2019

jtpio Oct 1, 2019

jtpio Oct 1, 2019 •

edited

KsavinN Oct 2, 2019

jtpio Oct 2, 2019

KsavinN Oct 2, 2019

jtpio commented Oct 1, 2019

afshin commented Oct 2, 2019

afshin left a comment •

edited

JohanMabille commented Oct 3, 2019 •

edited

KsavinN commented Oct 3, 2019

JohanMabille commented Oct 3, 2019 •

edited

jtpio Oct 3, 2019

	var breakpoints: Breakpoints.IBreakpoint[] = [];
	let breakpoints: Breakpoints.IBreakpoint[] = [];

Connect Breakpoints widget with NoteBook #36

Connect Breakpoints widget with NoteBook #36

Conversation

KsavinN commented Sep 2, 2019 • edited by afshin

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtpio Sep 4, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtpio commented Sep 26, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtpio Oct 1, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtpio commented Oct 1, 2019

afshin commented Oct 2, 2019

afshin left a comment • edited

Choose a reason for hiding this comment

JohanMabille commented Oct 3, 2019 • edited

KsavinN commented Oct 3, 2019

JohanMabille commented Oct 3, 2019 • edited

Choose a reason for hiding this comment

KsavinN commented Sep 2, 2019 •

edited by afshin

jtpio Sep 4, 2019 •

edited

jtpio Oct 1, 2019 •

edited

afshin left a comment •

edited

JohanMabille commented Oct 3, 2019 •

edited

JohanMabille commented Oct 3, 2019 •

edited