Graph visualization - initial code #451

franekp · 2017-10-10T13:58:25Z

No description provided.

berggren · 2017-10-10T14:19:33Z

timesketch/ui/graphs/graphs.module.ts

@@ -0,0 +1,42 @@
+/*
+Copyright 2015 Google Inc. All rights reserved.


s/2015/2017

We-St

First half of the review. I'll continue later.

We-St · 2017-10-11T08:47:10Z

timesketch/ui/api/attach-csrf-token.interceptor.ts

+import {HttpEvent, HttpInterceptor, HttpHandler, HttpRequest} from '@angular/common/http'
+
+export class AttachCsrfTokenInterceptor implements HttpInterceptor {
+  intercept(request: HttpRequest<any>, httpHandler: HttpHandler): Observable<HttpEvent<any>> {


If you do not rely on the type but need to provide one anyway, use "{}" rather "any". "any" deactivates the type checking completely, while "{}" is the empty type.

We-St · 2017-10-11T08:51:11Z

timesketch/ui/api/attach-csrf-token.interceptor.ts

@@ -0,0 +1,12 @@
+import {Observable} from 'rxjs'


This file here (and most others I have seen so far), rely on JavaScript's automatic semicolon insertion. This is quite dangerous and I would recommend moving away from it. There are some nasty edge-cases (e.g. around the return statement).

We-St · 2017-10-11T09:03:27Z

timesketch/ui/api/attach-csrf-token.interceptor.ts

+
+export class AttachCsrfTokenInterceptor implements HttpInterceptor {
+  intercept(request: HttpRequest<any>, httpHandler: HttpHandler): Observable<HttpEvent<any>> {
+    const csrfToken = document.getElementsByTagName('meta')[0]['content']


document.getElementsByTagName('meta')[0] is quite fragile: if anyone inserts a new meta tag, requests will start failing.

What about giving the meta tag a pseudo-attribute (e.g. "xsrf-token") and use document.querySelector('[xsrf-token]') to get it? Reference: https://developer.mozilla.org/en-US/docs/Web/API/Document/querySelector

We-St · 2017-10-11T09:06:39Z

timesketch/ui/api/graph.service.spec.ts

+    })
+  })
+
+  it('.search() should return a proper response for a query', async () => {


it() typically forms sentence, e.g. it('should do something'). The it() refers to the describe block. The output in case of error will be: Graphservice should do something.

You can have nested describe blocks (one for 'GraphService', one for 'search()') if you want or you can rename the first param of it.

We-St · 2017-10-11T09:10:19Z

timesketch/ui/api/graph.service.ts

+
+@Injectable()
+export class GraphService {
+  constructor(private sketchService: SketchService, private http: HttpClient) {}


optional: it's good practice to make all injected services "readonly" in addition to being private. However, no pressing need for this.

We-St · 2017-10-11T10:48:21Z

timesketch/ui/graphs/cypher-query.component.spec.ts

+  it('should emit the "cypherSearch" event when user hits enter key', async () => {
+    const textbox: HTMLInputElement = fixture.debugElement.query(By.css('input[type="search"]')).nativeElement
+    const form: HTMLFormElement = fixture.debugElement.query(By.css('form')).nativeElement
+    const cypherSearchPromise = new Promise((resolve) => fixture.componentInstance.cypherSearch.subscribe(resolve))


toPromise(). I'll leave it up to you to change it everywhere.

Ok, here it has a chance to work.

We-St · 2017-10-11T10:54:35Z

timesketch/ui/graphs/cypher-query.component.spec.ts

+    textbox.value = 'MATCH (a) RETURN (a)'
+    textbox.dispatchEvent(new Event('input'))
+    fixture.detectChanges()
+    form.dispatchEvent(new Event('submit'))


This doesn't test the enter key as the description says.

Ok, I will change the description. Testing enter key is not quite possible with PhantomJS.

Can you not simply do:

const event = new KeyboardEvent('keyup', {'key': 'Enter'});
someElement.dispatchEvent(event);

?

PhantomJS says that KeyboardEventConstructor is not a constructor, I would rather not put time into investigating this.

We-St · 2017-10-11T10:55:42Z

timesketch/ui/graphs/cypher-query.component.spec.ts

+    expect(cypherSearch).toEqual('MATCH (a) RETURN (a)')
+  })
+
+  it('should emit the "cypherSearch" event when user presses search button', async () => {


optional: this is nearly a duplicate of the previous test. You can nest this in another describe() block and have another beforeEach() to unify test setup.

Interesting.

We-St · 2017-10-11T10:57:05Z

timesketch/ui/graphs/cytoscape-settings.component.html

+    }
+  </style>
+  <!-- interaction options -->
+  <label>minZoom = <input [(ngModel)]="settings.minZoom" type="number" /></label><br />


Why not make this a list (ul and li) rather than use brs? You'd have more styling flexibility that way.

We-St · 2017-10-11T11:00:51Z

timesketch/ui/graphs/cytoscape-settings.component.spec.ts

+
+@Component({
+  template: `
+    <ts-graphs-cytoscape-settings [settings]="settings">


franekp · 2017-10-11T17:34:00Z

@berggren Ready to merge.
EDIT: just noticed this was just first half of the review. Let's wait for Stefan to finish second half.

We-St

Some more comments.

We-St · 2017-10-13T08:27:25Z

timesketch/ui/api/models.ts

@@ -0,0 +1,91 @@
+type DateTime = string


Since this file reflects server-side resources, please add a link to that documentation to this file.

Done. You can check if the format of the comment is acceptable.

Looks really good!

We-St · 2017-10-13T08:29:59Z

timesketch/ui/api/sketch.service.spec.ts

+      const fakeSketchId = 42
+
+      sketchService.sketchId = fakeSketchId
+      const sketchPromise = sketchService.getSketch().toPromise()


I think you could restructure this code a bit. What do you think about:

http.expectOne('/api/v1/sketches/42/').flush({objects: ['FAKE_SKETCH']})

sketchService.sketchId = 42
const sketch = await sketchService.getSketch().toPromise()
expect(sketch).toEqual('FAKE_SKETCH')
http.verify()

And (optionally) remove fakeResult and fakeSketchId.

sounds good.

We-St · 2017-10-13T08:32:51Z

timesketch/ui/css/ts.css

@@ -623,3 +623,65 @@ ul.content-list>li:last-child { border-bottom: none; }
  font-weight: 400;
  src: local('Roboto'), local('Roboto-Regular'), url('/static/fonts/Roboto/Roboto-Regular.ttf') format('truetype');
 }
+
+ts-graphs-cytoscape div {


We should think about moving CSS to SCSS at some point.

Sure. But later.

We-St · 2017-10-13T08:35:25Z

timesketch/ui/graphs/cytoscape-settings.component.spec.ts

+      )
+      const label: HTMLLabelElement = labelDebugElement.nativeElement
+      const input: HTMLInputElement = labelDebugElement.query(By.css('input')).nativeElement
+      if (typeof value === 'boolean') {


I think it would make sense to split this test into several, based on this if. E.g. one test for all boolean values, one for numbers, one for additive plus one to check that all fields were checked.

You could also move headless and styleEnabled into a separate one.

We-St · 2017-10-13T08:36:24Z

timesketch/ui/graphs/cytoscape-settings.component.ts

@@ -0,0 +1,37 @@
+import {Component, Input} from '@angular/core'
+
+export type CytoscapeSettings = {


Are these server-side resources again? I'd recommend either adding a link to the docs or jsDoc here.

No, these are not server-side resources. I can add a link to Cytoscape documentation.

We-St · 2017-10-13T09:18:21Z

timesketch/ui/graphs/graph-view.component.html

+    [style]="style"
+    [layout]="layout"
+
+    [minZoom]="settings.minZoom"


At some point, it might make sense to pass in a whole settings object rather than all sections individually.

Then the change detection will not detect changes to individual fields.

There are multiple ways to do this. Either you can use immtuables for this or override ngDoCheck (https://angular.io/guide/lifecycle-hooks#lifecycle-sequence). It's also ok to leave as-is for now, since this is a bigger change.

We-St · 2017-10-13T09:19:24Z

timesketch/ui/graphs/graph-view.component.ts

+    wheelSensitivity: 1,
+    pixelRatio: ('auto' as any),
+  }
+  layout = {


Do we have an interface for layout as well?

There is one in type definitions of Cytoscape library.

I will add it here, you are right.

We-St · 2017-10-13T09:19:55Z

timesketch/ui/graphs/graph-view.component.ts

+    minTemp: 1.0,
+    useMultitasking: true,
+  }
+  style = [


Can this be moved to CSS?

No, this is Cytoscape-specific.

I will add type annotation for that too.

We-St · 2017-10-13T09:21:59Z

timesketch/ui/graphs/main.component.html

@@ -0,0 +1,6 @@
+<div>


Angular2+ components do not need single template root components, so you can remove this div (if it doesn't serve any other purpose).

It is a flexbox container with fixed height, so it has a purpose. But this is good to know.

Then I wonder why not make the component element itself the flexbox container?

We-St · 2017-10-13T09:23:45Z

timesketch/ui/graphs/graph-view.component.ts

+
+import {CytoscapeSettings} from './cytoscape-settings.component'
+
+export type GraphViewState = {


I wonder if it wouldn't just be easier to write

export interface GraphViewState {
type: 'empty'|'loading'|'ready';
element?: ...
}

But this will no longer be semantically correct, right? I imagine later on this could become something like:

type GraphViewState = { type: 'empty' | 'loading' | 'ready' | 'error', spinnerUrl?: string, message?: string, elements?: Cy.ElementsDefinition, elementTypes?: ElementType[], }

How do you figure out which fields are available for which type?

Related: http://blog.jenkster.com/2016/06/how-elm-slays-a-ui-antipattern.html

This would not be semantically identical to what is written now. It's ok to keep the discriminator columns and be more precise on types, but in such a scenario, I would split this single type definition into multiple definitions, e.g.

export type Empty = { ... }; export type Loading = {...}; export type GraphViewState = Empty | Loading | ... ;

We-St · 2017-10-17T13:18:47Z

timesketch/ui/core/upload.directive.ts

@@ -46,8 +44,8 @@ export const tsCoreFileModel = ['$parse', function ($parse) {
            const model = $parse(attrs.tsCoreFileModel)
            const modelSetter = model.assign

-            element.bind('change', function (){
-                scope.$apply(function (){
+            element.bind('change', function () {


These could be changed to arrow functions.

I would rather create a separate PR which changes it everywhere.

Sounds good!

We-St · 2017-10-17T13:19:04Z

timesketch/ui/css/ts.scss

+  div {
+    @include ts-graphs-fill-space
+  }
+  @include ts-graphs-fill-space


Missing semicolons here.

We-St · 2017-10-17T13:22:20Z

timesketch/ui/graphs/cypher-query.component.ts

+  @Output() cypherSearch = new EventEmitter<string>()
+  query = 'MATCH (a)-[e]->(b) RETURN *'
+
+  onSubmit(event: Event) {


The event is unused.

We-St · 2017-10-17T13:24:41Z

timesketch/ui/graphs/cytoscape-settings.component.spec.ts

+  })
+
+  it('should have tests covering all cytoscape options', () => {
+    expect(Object.keys(remainingOptions).length).toEqual(0)


This makes your tests depend on each other and easier to break. If someone adds another test or moves them around, they will start to fail. I proposed a different solution in a comment further up.

Yes, this makes them order-dependent, but they are guaranted to execute in order they are defined. Solution from your comment is less dry, so it is not clear which one is better.

Having your tests depend on each other is probably a lot worse than minimal repetition. Especially because you guarantee with your test that the duplicated information stays in sync. (Note that Jasmine is not the problem here. It guarantees execution order, but you cannot be sure that new code will not be added after your test).

If you choose to leave it like this, please add a striking comment to the last test, something like:

// -------------------- THIS TEST NEEDS TO BE LAST --------------------
// This test verifies that all options were properly represented in the
// templates. Due to inter-dependent tests, this NEEDS to go last.

We-St · 2017-10-17T13:28:59Z

timesketch/ui/graphs/cytoscape-settings.component.spec.ts

+  })
+
+  describe('checkboxes', () => {
+    it('should work correctly', () => {


I would create a local list of all options here that are represented by a checkbox. Then you can iterate this list and see that all options can be toggled on the UI.

To make sure that people do not forget to update the tests when new options are introduced, I would create a second test case that checks that all checkboxes are in your local list. I.e. two tests:

before(('...') => {
const booleanOptions = ['a', 'b', 'c'];
it('allows toggling options', () => {
// Checks that all elements in booleanOptions are present in the template.
});
if('does not specify additional boolean options', () => {
// Checks that all checkboxes in the template are present in booleanOptions.
});
});

WDYT?

There are already assertions to ensure that all options are tested. Applying this suggestion would introduce some duplication, maybe even worth adding for readability. But I would rather not spend more time on it, there are other priorities.

We-St · 2017-10-17T13:52:27Z

timesketch/ui/graphs/graph-view.component.html

@@ -0,0 +1,57 @@
+<div *ngIf="state.type == 'empty'">


Triple equals here and everywhere.

We-St · 2017-10-17T13:54:15Z

timesketch/ui/graphs/graph-view.component.html

+    [style]="style"
+    [layout]="layout"
+
+    [minZoom]="settings.minZoom"


There are multiple ways to do this. Either you can use immtuables for this or override ngDoCheck (https://angular.io/guide/lifecycle-hooks#lifecycle-sequence). It's also ok to leave as-is for now, since this is a bigger change.

We-St · 2017-10-17T13:55:59Z

timesketch/ui/graphs/graph-view.component.ts

+    boundingBox: undefined,
+    randomize: true,
+    componentSpacing: 200,
+    nodeRepulsion: (node) => 400000,


Remove params if unused.

We-St · 2017-10-17T13:58:20Z

timesketch/ui/graphs/graph-view.component.ts

+
+import {CytoscapeSettings} from './cytoscape-settings.component'
+
+export type GraphViewState = {


This would not be semantically identical to what is written now. It's ok to keep the discriminator columns and be more precise on types, but in such a scenario, I would split this single type definition into multiple definitions, e.g.

export type Empty = { ... }; export type Loading = {...}; export type GraphViewState = Empty | Loading | ... ;

We-St · 2017-10-17T13:59:13Z

timesketch/ui/graphs/main.component.html

@@ -0,0 +1,6 @@
+<div>


Then I wonder why not make the component element itself the flexbox container?

We-St

This change looks good, but there are some follow-up changes we should consider:

-) Pick a style guide and make the current code conform to it.
-) Replace regular functions with arrow functions wherever sensible.
-) Evaluate if tests can be made non-dependent.
-) Add docs to server-side resources.

Nothing of this is a show-stopper ATM, so approving.

We-St · 2017-10-17T15:50:53Z

timesketch/ui/api/models.ts

@@ -0,0 +1,91 @@
+type DateTime = string


Looks really good!

We-St · 2017-10-17T15:57:46Z

timesketch/ui/graphs/cytoscape-settings.component.spec.ts

+  })
+
+  it('should have tests covering all cytoscape options', () => {
+    expect(Object.keys(remainingOptions).length).toEqual(0)


Having your tests depend on each other is probably a lot worse than minimal repetition. Especially because you guarantee with your test that the duplicated information stays in sync. (Note that Jasmine is not the problem here. It guarantees execution order, but you cannot be sure that new code will not be added after your test).

If you choose to leave it like this, please add a striking comment to the last test, something like:

// -------------------- THIS TEST NEEDS TO BE LAST --------------------
// This test verifies that all options were properly represented in the
// templates. Due to inter-dependent tests, this NEEDS to go last.

We-St · 2017-10-17T16:00:30Z

timesketch/ui/graphs/cytoscape.component.ts

+      this.zoomChange.emit(this.zoom)
+    })
+    for (const [k, v] of Object.entries(this)) {
+      if (v instanceof EventEmitter && !k.endsWith('Change')) {


Good job, this is quite an improvement.

berggren requested a review from We-St October 10, 2017 14:02

franekp force-pushed the graphs-initial branch from 4c4fead to 1254502 Compare October 10, 2017 14:17

berggren reviewed Oct 10, 2017

View reviewed changes

timesketch/ui/graphs/graphs.module.ts Outdated

@@ -0,0 +1,42 @@

/*

Copyright 2015 Google Inc. All rights reserved.

Copy link

Contributor

berggren Oct 10, 2017

Choose a reason for hiding this comment

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

s/2015/2017

franekp force-pushed the graphs-initial branch from 1254502 to 6b321b5 Compare October 10, 2017 14:21

We-St reviewed Oct 11, 2017

View reviewed changes

franekp force-pushed the graphs-initial branch 3 times, most recently from 87b40d4 to 7d777ea Compare October 11, 2017 17:31

We-St reviewed Oct 13, 2017

View reviewed changes

franekp force-pushed the graphs-initial branch 16 times, most recently from 586e1da to 905084a Compare October 16, 2017 14:42

We-St reviewed Oct 17, 2017

View reviewed changes

franekp force-pushed the graphs-initial branch 3 times, most recently from d39fa22 to 907f279 Compare October 17, 2017 15:33

franekp force-pushed the graphs-initial branch 4 times, most recently from 4962516 to 1f4c4ef Compare October 17, 2017 16:06

Add a page and navigation entry for graphs.

59c2c07

franekp force-pushed the graphs-initial branch from 1f4c4ef to 1e32eb1 Compare October 17, 2017 16:41

We-St approved these changes Oct 17, 2017

View reviewed changes

franekp force-pushed the graphs-initial branch 2 times, most recently from c30fd49 to a393006 Compare October 17, 2017 16:51

Components for displaying graphs.

c9f39a7

franekp force-pushed the graphs-initial branch from a393006 to c9f39a7 Compare October 17, 2017 16:52

berggren merged commit 2c949bc into google:master Oct 17, 2017

		@@ -0,0 +1,42 @@
		/*
		Copyright 2015 Google Inc. All rights reserved.

		@@ -0,0 +1,37 @@
		import {Component, Input} from '@angular/core'

		export type CytoscapeSettings = {


		import {CytoscapeSettings} from './cytoscape-settings.component'

		export type GraphViewState = {

Graph visualization - initial code #451

Graph visualization - initial code #451

Conversation

franekp commented Oct 10, 2017

Choose a reason for hiding this comment

We-St left a 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

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

franekp Oct 12, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

franekp commented Oct 11, 2017 • edited Loading

We-St left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

franekp Oct 16, 2017 • edited Loading

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

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

franekp Oct 13, 2017 • edited Loading

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

franekp Oct 13, 2017 • edited Loading

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

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

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

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

franekp Oct 12, 2017 •

edited

Loading

franekp commented Oct 11, 2017 •

edited

Loading

franekp Oct 16, 2017 •

edited

Loading

franekp Oct 13, 2017 •

edited

Loading

franekp Oct 13, 2017 •

edited

Loading