Merge pull request #165 from JohanMabille/breakpoints

The model now keeps all the breakpoints of a notebook

azure-pipelines.yml

@@ -16,7 +16,7 @@ steps:
 
 - bash: |
     source activate jupyterlab-debugger
-    conda install --yes --quiet -c conda-forge nodejs xeus-python=0.6.1 ptvsd python=$PYTHON_VERSION
+    conda install --yes --quiet -c conda-forge nodejs xeus-python=0.6.5 ptvsd python=$PYTHON_VERSION
     python -m pip install -U --pre jupyterlab
   displayName: Install dependencies
 

binder/environment.yml

@@ -6,4 +6,4 @@ dependencies:
 - nodejs
 - notebook=6
 - ptvsd
-- xeus-python=0.6
+- xeus-python=0.6.5

package.json

@@ -56,6 +56,7 @@
     "@phosphor/coreutils": "^1.3.1",
     "@phosphor/disposable": "^1.2.0",
     "@phosphor/widgets": "^1.8.0",
+    "murmurhash-js": "^1.0.0",
     "vscode-debugprotocol": "1.35.0",
     "react-inspector": "^2.0.0"
   },
@@ -66,6 +67,7 @@
     "@types/chai": "^4.1.3",
     "@types/codemirror": "0.0.76",
     "@types/jest": "^24.0.17",
+    "@types/murmurhash-js": "1.0.3",
     "chai": "^4.2.0",
     "husky": "^3.0.2",
     "jest": "^24.8.0",

src/breakpoints/body.tsx

@@ -2,7 +2,6 @@
 // Distributed under the terms of the Modified BSD License.
 
 import { ReactWidget } from '@jupyterlab/apputils';
-import { ArrayExt } from '@phosphor/algorithm';
 import { ISignal } from '@phosphor/signaling';
 import React, { useEffect, useState } from 'react';
 import { Breakpoints } from '.';
@@ -22,26 +21,52 @@ export class Body extends ReactWidget {
 }
 
 const BreakpointsComponent = ({ model }: { model: Breakpoints.Model }) => {
-  const [breakpoints, setBreakpoints] = useState(model.breakpoints);
+  const [breakpoints, setBreakpoints] = useState(
+    Array.from(model.breakpoints.entries())
+  );
 
   useEffect(() => {
     const updateBreakpoints = (
       _: Breakpoints.Model,
       updates: Breakpoints.IBreakpoint[]
     ) => {
-      if (ArrayExt.shallowEqual(breakpoints, updates)) {
-        return;
-      }
-      setBreakpoints(updates);
+      setBreakpoints(Array.from(model.breakpoints.entries()));
+    };
+
+    const restoreBreakpoints = (_: Breakpoints.Model) => {
+      setBreakpoints(Array.from(model.breakpoints.entries()));
     };
 
     model.changed.connect(updateBreakpoints);
+    model.restored.connect(restoreBreakpoints);
 
     return () => {
       model.changed.disconnect(updateBreakpoints);
+      model.restored.disconnect(restoreBreakpoints);
     };
   });
 
+  return (
+    <div>
+      {breakpoints.map(entry => (
+        // Array.from(breakpoints.entries()).map((entry) => (
+        <BreakpointCellComponent
+          key={entry[0]}
+          breakpoints={entry[1]}
+          model={model}
+        />
+      ))}
+    </div>
+  );
+};
+
+const BreakpointCellComponent = ({
+  breakpoints,
+  model
+}: {
+  breakpoints: Breakpoints.IBreakpoint[];
+  model: Breakpoints.Model;
+}) => {
   return (
     <div>
       {breakpoints
@@ -50,7 +75,7 @@ const BreakpointsComponent = ({ model }: { model: Breakpoints.Model }) => {
         })
         .map((breakpoint: Breakpoints.IBreakpoint) => (
           <BreakpointComponent
-            key={breakpoint.line}
+            key={breakpoint.source.path + breakpoint.line}
             breakpoint={breakpoint}
             breakpointChanged={model.breakpointChanged}
           />

src/breakpoints/index.ts

@@ -30,10 +30,14 @@ export class Breakpoints extends Panel {
         tooltip: `${this.isAllActive ? 'Deactivate' : 'Activate'} Breakpoints`,
         onClick: () => {
           this.isAllActive = !this.isAllActive;
-          this.model.breakpoints.map((breakpoint: Breakpoints.IBreakpoint) => {
-            breakpoint.active = this.isAllActive;
-            this.model.breakpoint = breakpoint;
-          });
+
+          // TODO: this requires a set breakpoint(bp: Breakpoints.IBreakpoint[]) method in the model
+          /*Array.from(this.model.breakpoints.values()).map((breakpoints: Breakpoints.IBreakpoint[]) => {
+            breakpoints.map((breakpoint: Breakpoints.IBreakpoint) => {
+              breakpoint.active = this.isAllActive;
+              this.model.breakpoint = breakpoint;
+            });
+          });*/
         }
       })
     );
@@ -43,7 +47,7 @@ export class Breakpoints extends Panel {
       new ToolbarButton({
         iconClassName: 'jp-CloseAllIcon',
         onClick: () => {
-          void this.service.updateBreakpoints([]);
+          void this.service.clearBreakpoints();
         },
         tooltip: 'Remove All Breakpoints'
       })
@@ -78,46 +82,23 @@ export namespace Breakpoints {
   }
 
   export class Model implements IDisposable {
-    constructor(model: IBreakpoint[]) {
-      this._breakpoints = model;
+    get changed(): Signal<this, IBreakpoint[]> {
+      return this._changed;
     }
 
-    changed = new Signal<this, IBreakpoint[]>(this);
+    get restored(): Signal<this, void> {
+      return this._restored;
+    }
 
-    get breakpoints(): IBreakpoint[] {
+    get breakpoints(): Map<string, IBreakpoint[]> {
       return this._breakpoints;
     }
 
+    // kept for react component
     get breakpointChanged(): Signal<this, IBreakpoint> {
       return this._breakpointChanged;
     }
 
-    set breakpoints(breakpoints: IBreakpoint[]) {
-      this._breakpoints = [...breakpoints];
-      this.changed.emit(this._breakpoints);
-    }
-
-    set breakpoint(breakpoint: IBreakpoint) {
-      const index = this._breakpoints.findIndex(
-        ele => ele.line === breakpoint.line
-      );
-      if (index !== -1) {
-        this._breakpoints[index] = breakpoint;
-        this._breakpointChanged.emit(breakpoint);
-      } else {
-        this.breakpoints = [...this.breakpoints, breakpoint];
-      }
-    }
-
-    set type(newType: SessionTypes) {
-      if (newType === this._selectedType) {
-        return;
-      }
-      this._state[this._selectedType] = this.breakpoints;
-      this._selectedType = newType;
-      this.breakpoints = this._state[newType];
-    }
-
     get isDisposed(): boolean {
       return this._isDisposed;
     }
@@ -130,13 +111,25 @@ export namespace Breakpoints {
       Signal.clearData(this);
     }
 
-    private _selectedType: SessionTypes;
+    setBreakpoints(id: string, breakpoints: IBreakpoint[]) {
+      this._breakpoints.set(id, breakpoints);
+      this.changed.emit(breakpoints);
+    }
+
+    getBreakpoints(id: string): IBreakpoint[] {
+      return this._breakpoints.get(id) || [];
+    }
+
+    restoreBreakpoints(breakpoints: Map<string, IBreakpoint[]>) {
+      this._breakpoints = breakpoints;
+      this._restored.emit();
+    }
+
+    private _breakpoints = new Map<string, IBreakpoint[]>();
+    private _changed = new Signal<this, IBreakpoint[]>(this);
+    private _restored = new Signal<this, void>(this);
+    // kept for react component
     private _breakpointChanged = new Signal<this, IBreakpoint>(this);
-    private _breakpoints: IBreakpoint[];
-    private _state = {
-      console: [] as Breakpoints.IBreakpoint[],
-      notebook: [] as Breakpoints.IBreakpoint[]
-    };
     private _isDisposed: boolean = false;
   }
 

src/debugger.ts

@@ -116,7 +116,7 @@ export namespace Debugger {
 
   export class Model implements IDisposable {
     constructor(options: Debugger.Model.IOptions) {
-      this.breakpointsModel = new Breakpoints.Model([]);
+      this.breakpointsModel = new Breakpoints.Model();
       this.callstackModel = new Callstack.Model([]);
       this.variablesModel = new Variables.Model([]);
       this.connector = options.connector || null;

src/handlers/cell.ts

@@ -57,6 +57,13 @@ export class CellManager implements IDisposable {
       this.addBreakpointsToEditor(this.activeCell);
     });
 
+    this.breakpointsModel.restored.connect(async () => {
+      if (!this.activeCell || this.activeCell.isDisposed) {
+        return;
+      }
+      this.addBreakpointsToEditor(this.activeCell);
+    });
+
     if (this.activeCell) {
       this._debuggerModel.codeValue = this.activeCell.model.value;
     }
@@ -145,16 +152,7 @@ export class CellManager implements IDisposable {
     }
 
     const editor = cell.editor as CodeMirrorEditor;
-
-    const editorBreakpoints = this.getBreakpointsInfo(cell).map(lineInfo => {
-      return Private.createBreakpoint(
-        this._debuggerService.session.client.name,
-        this.getEditorId(),
-        lineInfo.line + 1
-      );
-    });
-
-    void this._debuggerService.updateBreakpoints(editorBreakpoints);
+    this.addBreakpointsToEditor(cell);
 
     editor.setOption('lineNumbers', true);
     editor.editor.setOption('gutters', [
@@ -184,7 +182,9 @@ export class CellManager implements IDisposable {
     }
 
     const isRemoveGutter = !!info.gutterMarkers;
-    let breakpoints = this.breakpointsModel.breakpoints;
+    let breakpoints: Breakpoints.IBreakpoint[] = this.getBreakpoints(
+      this._activeCell
+    );
     if (isRemoveGutter) {
       breakpoints = breakpoints.filter(ele => ele.line !== info.line + 1);
     } else {
@@ -197,13 +197,16 @@ export class CellManager implements IDisposable {
       );
     }
 
-    void this._debuggerService.updateBreakpoints(breakpoints);
+    void this._debuggerService.updateBreakpoints(
+      this._activeCell.model.value.text,
+      breakpoints
+    );
   };
 
   private addBreakpointsToEditor(cell: CodeCell) {
     this.clearGutter(cell);
     const editor = cell.editor as CodeMirrorEditor;
-    const breakpoints = this._debuggerModel.breakpointsModel.breakpoints;
+    const breakpoints = this.getBreakpoints(cell);
     breakpoints.forEach(breakpoint => {
       editor.editor.setGutterMarker(
         breakpoint.line - 1,
@@ -213,16 +216,10 @@ export class CellManager implements IDisposable {
     });
   }
 
-  private getBreakpointsInfo(cell: CodeCell): ILineInfo[] {
-    const editor = cell.editor as CodeMirrorEditor;
-    let lines = [];
-    for (let i = 0; i < editor.doc.lineCount(); i++) {
-      const info = editor.editor.lineInfo(i);
-      if (info.gutterMarkers) {
-        lines.push(info);
-      }
-    }
-    return lines;
+  private getBreakpoints(cell: CodeCell): Breakpoints.IBreakpoint[] {
+    return this._debuggerModel.breakpointsModel.getBreakpoints(
+      this._debuggerService.getCellId(cell.model.value.text)
+    );
   }
 
   private _previousCell: CodeCell;

src/service.ts

@@ -5,6 +5,8 @@ import { IClientSession } from '@jupyterlab/apputils';
 
 import { ISignal, Signal } from '@phosphor/signaling';
 
+import { murmur2 } from 'murmurhash-js';
+
 import { DebugProtocol } from 'vscode-debugprotocol';
 
 import { Debugger } from './debugger';
@@ -140,6 +142,13 @@ export class DebugService implements IDebugger {
     Signal.clearData(this);
   }
 
+  /**
+   * Computes an id based on the given code.
+   */
+  getCellId(code: string): string {
+    return this._tmpFilePrefix + this._hashMethod(code) + this._tmpFileSuffix;
+  }
+
   /**
    * Whether the current debugger is started.
    */
@@ -180,7 +189,16 @@ export class DebugService implements IDebugger {
     this.clearModel();
     this._stoppedThreads.clear();
     await this.start();
-    await this.updateBreakpoints(breakpoints);
+
+    // No need to dump the cells again, we can simply
+    // resend the breakpoints to the kernel and update
+    // the model.
+    breakpoints.forEach(async (bp, path, _) => {
+      const sourceBreakpoints = Private.toSourceBreakpoints(bp);
+      await this.setBreakpoints(sourceBreakpoints, path);
+    });
+
+    this.model.breakpointsModel.restoreBreakpoints(breakpoints);
   }
 
   /**
@@ -194,8 +212,30 @@ export class DebugService implements IDebugger {
       return;
     }
 
-    await this.session.restoreState();
-    // TODO: restore breakpoints when the model is updated
+    const reply = await this.session.restoreState();
+
+    this.setHashParameters(reply.body.hashMethod, reply.body.hashSeed);
+    this.setTmpFileParameters(
+      reply.body.tmp_file_prefix,
+      reply.body.tmp_file_suffix
+    );
+
+    const breakpoints = reply.body.breakpoints;
+    let bpMap = new Map<string, Breakpoints.IBreakpoint[]>();
+    if (breakpoints.length !== 0) {
+      breakpoints.forEach((bp: IDebugger.ISession.IDebugInfoBreakpoints) => {
+        bpMap.set(
+          bp.source,
+          bp.breakpoints.map(breakpoint => {
+            return {
+              ...breakpoint,
+              active: true
+            };
+          })
+        );
+      });
+    }
+    this._model.breakpointsModel.restoreBreakpoints(bpMap);
 
     if (!this.isStarted() && autoStart) {
       await this.start();
@@ -257,14 +297,18 @@ export class DebugService implements IDebugger {
 
   /**
    * Update all breakpoints at once.
+   * @param code - The code in the cell where the breakpoints are set.
+   * @param breakpoints - The list of breakpoints to set.
    */
-  async updateBreakpoints(breakpoints: Breakpoints.IBreakpoint[]) {
+  async updateBreakpoints(
+    code: string,
+    breakpoints: Breakpoints.IBreakpoint[]
+  ) {
     if (!this.session.isStarted) {
       return;
     }
     // Workaround: this should not be called before the session has started
     await this.ensureSessionReady();
-    const code = this._model.codeValue.text;
     const dumpedCell = await this.dumpCell(code);
     const sourceBreakpoints = Private.toSourceBreakpoints(breakpoints);
     const reply = await this.setBreakpoints(
@@ -274,8 +318,7 @@ export class DebugService implements IDebugger {
     let kernelBreakpoints = reply.body.breakpoints.map(breakpoint => {
       return {
         ...breakpoint,
-        active: true,
-        source: { path: this.session.client.name }
+        active: true
       };
     });
 
@@ -284,10 +327,28 @@ export class DebugService implements IDebugger {
       (breakpoint, i, arr) =>
         arr.findIndex(el => el.line === breakpoint.line) === i
     );
-    this._model.breakpointsModel.breakpoints = kernelBreakpoints;
+    this._model.breakpointsModel.setBreakpoints(
+      dumpedCell.sourcePath,
+      kernelBreakpoints
+    );
     await this.session.sendRequest('configurationDone', {});
   }
 
+  async clearBreakpoints() {
+    if (!this.session.isStarted) {
+      return;
+    }
+
+    this._model.breakpointsModel.breakpoints.forEach(
+      async (breakpoints, path, _) => {
+        await this.setBreakpoints([], path);
+      }
+    );
+
+    let bpMap = new Map<string, Breakpoints.IBreakpoint[]>();
+    this._model.breakpointsModel.restoreBreakpoints(bpMap);
+  }
+
   getAllFrames = async () => {
     const stackFrames = await this.getFrames(this.currentThread());
 
@@ -396,6 +457,21 @@ export class DebugService implements IDebugger {
     return 1;
   }
 
+  private setHashParameters(method: string, seed: number) {
+    if (method === 'Murmur2') {
+      this._hashMethod = (code: string) => {
+        return murmur2(code, seed).toString();
+      };
+    } else {
+      throw new Error('hash method not supported ' + method);
+    }
+  }
+
+  private setTmpFileParameters(prefix: string, suffix: string) {
+    this._tmpFilePrefix = prefix;
+    this._tmpFileSuffix = suffix;
+  }
+
   private _isDisposed: boolean = false;
   private _session: IDebugger.ISession;
   private _sessionChanged = new Signal<IDebugger, IDebugger.ISession>(this);
@@ -403,6 +479,10 @@ export class DebugService implements IDebugger {
   private _eventMessage = new Signal<IDebugger, IDebugger.ISession.Event>(this);
   private _model: Debugger.Model;
 
+  private _hashMethod: (code: string) => string;
+  private _tmpFilePrefix: string;
+  private _tmpFileSuffix: string;
+
   // TODO: remove frames from the service
   private frames: Frame[] = [];
 

src/tokens.ts

@@ -56,6 +56,11 @@ export interface IDebugger extends IDisposable {
    */
   readonly eventMessage: ISignal<IDebugger, IDebugger.ISession.Event>;
 
+  /**
+   * Computes an id based on the given code.
+   */
+  getCellId(code: string): string;
+
   /**
    * Whether the current debugger is started.
    */
@@ -112,9 +117,19 @@ export interface IDebugger extends IDisposable {
   stepOut(): Promise<void>;
 
   /**
-   * Update all breakpoints at once.
+   * Update all breakpoints of a cell at once.
+   * @param code - The code in the cell where the breakpoints are set.
+   * @param breakpoints - The list of breakpoints to set.
+   */
+  updateBreakpoints(
+    code: string,
+    breakpoints: Breakpoints.IBreakpoint[]
+  ): Promise<void>;
+
+  /**
+   * Removes all the breakpoints from the current notebook or console
    */
-  updateBreakpoints(breakpoints: Breakpoints.IBreakpoint[]): Promise<void>;
+  clearBreakpoints(): Promise<void>;
 }
 
 /**
@@ -197,7 +212,7 @@ export namespace IDebugger {
      */
     export interface IDebugInfoBreakpoints {
       source: string;
-      lines: number[];
+      breakpoints: DebugProtocol.Breakpoint[];
     }
 
     /**
@@ -211,6 +226,8 @@ export namespace IDebugger {
         hashMethod: string;
         hashSeed: number;
         breakpoints: IDebugInfoBreakpoints[];
+        tmp_file_prefix: string;
+        tmp_file_suffix: string;
       };
     }
 

tests/src/session.spec.ts

@@ -169,7 +169,10 @@ describe('protocol', () => {
       expect(breakpoints.length).to.equal(1);
 
       const breakpointsInfo = breakpoints[0];
-      expect(breakpointsInfo.lines).to.deep.equal([3, 5]);
+      const breakpointLines = breakpointsInfo.breakpoints.map(bp => {
+        return bp.line;
+      });
+      expect(breakpointLines).to.deep.equal([3, 5]);
     });
   });