New: Add properties setters and getters #93

AlexIchenskiy · 2024-02-28T06:56:15Z

Resolves #88

This PR introduces setters, patchers, and getters for node/edge properties, along with implementing the Observer pattern to invoke view rerender upon property changes. The API has been significantly changed. Instead of writing unsafe code like

orb.data.getNodeById(0).property = x;

we now use

orb.data.getNodeById(0).setProperty(x);

and

orb.data.getNodeById(0).patchProperty(x);

Accessing properties is also modified, from

orb.data.getNodeById(0).x;

to

orb.data.getNodeById(0).getX();

Documentation needs to be updated accordingly. However, I believe it's more appropriate to do a complete rewrite upon the 1.0.0 release. Additionally, I want to implement tests after discussing all the details.

This PR is marked as a draft because I feel there are important aspects to double-check and review. However, it's open for all sorts of comments and advice!

AlexIchenskiy · 2024-02-28T07:02:58Z

Tests are failing because of the usage of the structuredClone() function, which was introduced in Node.js 17.0.0, while our tests are running on Node.js v16.20.2. Is it ok to update the Node.js version in the package.json to >=17.0.0, or should I use some custom function for deep cloning objects?

src/models/observer.ts

src/models/node.ts

AlexIchenskiy · 2024-02-28T07:19:23Z

src/models/node.ts

+      | INodeCoordinates
+      | INodeMapCoordinates
+      | INodePosition
+      | ((node: INode<N, E>) => INodeCoordinates | INodeMapCoordinates | INodePosition),


Should I merge these interfaces somehow? I was considering assigning a type to nodes (for map view and regular view), but I'm unsure if it's necessary since users can use any value from the node data for lng and lat. Is there a better approach to setting the position for map view nodes maybe?

Check the orb usage for the map, you will see that the user needs to say how lat and long are extracted for each node. That is where user has ability to remove positions (not returning anything, etc.).

https://github.com/memgraph/orb/blob/main/examples/example-graph-on-map.html#L49

So LAT and LNG should be removed from these, first because it changes the data - we should never change the data because that is owner by the user, and second, user can have abc and def for his latitude and longitude information - in the map callback he would just do this:

orb.setView((context) => new Orb.MapView(context, { getGeoPosition: (node) => ({ lat: node.data.abc, lng: node.data.def, }), }));

To recap, map is not using these positions.

AlexIchenskiy · 2024-02-28T07:33:40Z

src/models/edge.ts

-  position: IEdgePosition;
-  style: IEdgeStyle;
-  state: number;
+export interface IEdge<N extends INodeBase, E extends IEdgeBase> extends ISubject {
  readonly id: any;
  readonly offset: number;
  readonly start: any;
  readonly startNode: INode<N, E>;
  readonly end: any;


I was considering rewriting the values to use getters for consistency, but some are required in the IEdgeBase interface (such as id, start, end), which is also required in topology.ts. While I believe it's a good idea to use getters for consistency, it seems unnatural given the existing interface requirements. Is it fine to have a combination of TypeScript getters (e.g., get x) and regular getters (e.g., getX), or should I consider modifying the API further?

Hmm, I am not sure. These are already readonly so user can just read them and there is no way to set those except creating a new edge or node. I am leaving the decision to you, if you wish and you think it will be more consistent, feel free to have it consistently. But Base class will still need to have those simple fields because that's how user creates nodes and edges (as JS objects).

src/models/edge.ts

tonilastre · 2024-02-28T09:33:36Z

Tests are failing because of the usage of the structuredClone() function, which was introduced in Node.js 17.0.0, while our tests are running on Node.js v16.20.2. Is it ok to update the Node.js version in the package.json to >=17.0.0, or should I use some custom function for deep cloning objects?

Yep, we should update Node to some newer version for sure!

tonilastre

This is going in a great direction!

I still need to check 3 of your comments, so stay tuned for responses.

src/models/edge.ts

src/renderer/canvas/node.ts

src/models/style.ts

src/simulator/factory.ts

src/models/edge.ts

src/models/node.ts

src/views/orb-map-view.ts

tonilastre · 2024-02-28T10:47:07Z

src/views/orb-map-view.ts

+  update(): void {
+    this.render();
+  }


Hmm, this is maybe a bit tricky, because on external API (orb view), there is an .update() call because of an observer. Is there a way to handle this better? Would an observer with a callback do the trick or?

Is there anything we can do there? Do you have any ideas?

Do you Toni maybe have an idea how to implement this?

Currently I am seeing two ideas, but there can be more for sure:

Have a proxy class that is an actual listener, but also gives you a callback way to respond to updates. Then view can initiate that proxy class where update will be called, and update calls a callback that is registered privately from the view. Then update is not exposed - easier

Add callback to the observer pattern, so listener can either have a callback or extending the update to get calls for notifications - harder

src/models/graph.ts

.github/workflows/build.yml

package.json

src/models/edge.ts

tonilastre · 2024-03-04T12:26:40Z

src/views/orb-map-view.ts

+  update(): void {
+    this.render();
+  }


Is there anything we can do there? Do you have any ideas?

src/utils/observer.utils.ts

src/utils/object.utils.ts

src/renderer/canvas/edge/base.ts

AlexIchenskiy · 2024-03-07T14:07:53Z

src/models/node.ts

+      | INodeMapCoordinates
+      | INodePosition
+      | ((node: INode<N, E>) => INodeCoordinates | INodeMapCoordinates | INodePosition),
+    isInner?: boolean,


I have to use the second argument here because sometimes we don't need to notify listeners on set position (e.g., on SIMULATION_PROGRESS in orb-view.ts because position handling is done through the simulation). However, this approach does not seem to be very clean. Do you have any ideas on how I could make it better?

If we decide to leave it here, perhaps a better naming option would be isInnerCall? I will also update the interface accordingly when we define it :)

Aha. Angular uses dispatch: false for those kind of events. isInnerCall doesn't really say what you described so I would go into the direction such as: isNotifySkipped or isDispatchSkipped, isNotifyIgnored. In general, use the name that describes what you described to me, you don't want to notify listeners. And the default should be false or not existing object, that's why I used skipped or Ignored. Feel free to come up with a name with such constraints.

Also, this should be an object, optional options?.

AlexIchenskiy · 2024-03-07T14:15:05Z

src/simulator/engine/d3-simulator-engine.ts

@@ -312,6 +312,27 @@ export class D3SimulatorEngine extends Emitter<D3SimulatorEvents> {
    }
  }

+  patchData(data: Partial<ISimulationGraph>) {


Is it somehow possible to reduce the code redundancy between the patchData and _initializeNewData functions? They are essentially performing patch and set operations, but I'm not sure if it's possible to reduce the code repetition here without overcomplicating things.

AlexIchenskiy · 2024-03-07T14:18:42Z

src/views/orb-view.ts

+            fx: data.x,
+            fy: data.y,
+            id: data.id,
+          },


Should I create and use some sort of getStickyNode function here instead of directly writing these properties?

I don't see a problem writing it directly, but maybe @cizl has more to say here?

I think this is fine. I would write a utility function if this starts to get repeated.

But on the other hand, when can this be called? Will it interfere with the physics functionality that depends on fx and fy? Is the nodes in the data always guaranteed to be sticky?

tonilastre · 2024-03-13T17:41:05Z

src/utils/observer.utils.ts

+import { INodeCoordinates, INodeMapCoordinates, INodePosition } from '../models/node';
+
+export type IObserverDataPayload = INodePosition | INodeCoordinates | INodeMapCoordinates;
+
+export interface IObserver {
+  update(data?: IObserverDataPayload): void;
+}


Uh oh, this is not generic anymore because you introduced local models (Position, Coordinates, NodeMapCoordinates) to the util ones. :D

I am considering adding parameterization for data by including parameters that represent the data that can be sent (similar to this payload but specified in the parameters of each implementation). Do you maybe have any better ideas?

I have created an issue #97 for this where we can discuss it further :)

tonilastre · 2024-03-13T17:41:57Z

src/views/orb-view.ts

@@ -282,7 +286,7 @@ export class OrbView<N extends INodeBase, E extends IEdgeBase> implements IOrbVi
      for (let i = 0; i < nodes.length; i++) {
        const position = this._settings.getPosition(nodes[i]);
        if (position) {
-          nodes[i].position = { id: nodes[i].id, ...position };
+          nodes[i].setPosition({ id: nodes[i].getId(), ...position }, true);


Similar to previous comments, seeing this true as second argument makes code harder to read. Having an object instead will work better because it will be more clear enough what true is all about.

@tonilastre When passing objects as inputs to functions, how do we handle sane defaults?
When using function arguments we can put some defaults, but when using objects, we need to specify each and every property.

On a call earlier this week we discussed having a default object in that case, and passing a Partial<IInputParams> as an input to override the "defaults". Does this make sense? How is this usually handled in practice?

It depends on the options. In some other comment, I suggested what the options should be for this one where empty options would mean normal mail flow. Only in exception, the options can be added.

If that is not the case, then yea, the function should define defaults internally and merge it with the input options.

tonilastre · 2024-03-13T17:42:38Z

src/views/orb-view.ts

+            fx: data.x,
+            fy: data.y,
+            id: data.id,
+          },


I don't see a problem writing it directly, but maybe @cizl has more to say here?

tonilastre · 2024-03-13T17:44:06Z

src/utils/object.utils.ts

@@ -120,3 +120,11 @@ const copyPlainObject = <T>(obj: Record<string, T>): Record<string, T> => {
  });
  return newObject;
 };
+
+export const patchProperties = <T>(target: T, source: T): void => {
+  const keys = Object.keys(source as Object);


I am not sure, but if you say here that these keys are (keyof T)[] or something similar, you won't need this monster of as X on line 128.

tonilastre · 2024-03-13T17:45:26Z

src/simulator/engine/d3-simulator-engine.ts

+      data.nodes = this._fixDefinedNodes(data.nodes);
+      const nodeIds = this._nodes.map((node) => node.id);
+      for (let i = 0; i < data.nodes.length; i += 1) {
+        if (nodeIds.includes(data.nodes[i].id)) {


Save indents. Have reverse if here and continue. The main code will have one less indent - it is easier to read code then.

tonilastre · 2024-03-13T17:55:33Z

src/models/node.ts

+      this.setData({ ...data, lng: undefined });
+    }
+
+    if ('lat' in this._data) {
+      this.setData({ ...data, lat: undefined });


This is a bit tricky because you are updating user's data. I understand that you wanted to handle both map and regular view, but user should handle the map positions with the callback that is on the map view. These x, y positions are not even used by the simulator on the map.

To recap, I would remove this because it changes the user's data.

tonilastre · 2024-03-13T17:56:37Z

src/models/node.ts

+  setPosition(position: INodeCoordinates | INodeMapCoordinates | INodePosition, isInner?: boolean): void;
+  setPosition(
+    callback: (node: INode<N, E>) => INodeCoordinates | INodeMapCoordinates | INodePosition,
+    isInner?: boolean,


Btw, these functions in the interface have name call instead of isInner. It would be great to have them consistent.

tonilastre · 2024-03-13T17:59:57Z

src/models/node.ts

+      | INodeCoordinates
+      | INodeMapCoordinates
+      | INodePosition
+      | ((node: INode<N, E>) => INodeCoordinates | INodeMapCoordinates | INodePosition),


Check the orb usage for the map, you will see that the user needs to say how lat and long are extracted for each node. That is where user has ability to remove positions (not returning anything, etc.).

https://github.com/memgraph/orb/blob/main/examples/example-graph-on-map.html#L49

So LAT and LNG should be removed from these, first because it changes the data - we should never change the data because that is owner by the user, and second, user can have abc and def for his latitude and longitude information - in the map callback he would just do this:

orb.setView((context) => new Orb.MapView(context, { getGeoPosition: (node) => ({ lat: node.data.abc, lng: node.data.def, }), }));

To recap, map is not using these positions.

tonilastre · 2024-03-13T18:03:23Z

src/models/node.ts

+      | INodeMapCoordinates
+      | INodePosition
+      | ((node: INode<N, E>) => INodeCoordinates | INodeMapCoordinates | INodePosition),
+    isInner?: boolean,


Aha. Angular uses dispatch: false for those kind of events. isInnerCall doesn't really say what you described so I would go into the direction such as: isNotifySkipped or isDispatchSkipped, isNotifyIgnored. In general, use the name that describes what you described to me, you don't want to notify listeners. And the default should be false or not existing object, that's why I used skipped or Ignored. Feel free to come up with a name with such constraints.

tonilastre · 2024-03-13T18:03:49Z

src/models/node.ts

+      | INodeMapCoordinates
+      | INodePosition
+      | ((node: INode<N, E>) => INodeCoordinates | INodeMapCoordinates | INodePosition),
+    isInner?: boolean,


Also, this should be an object, optional options?.

tonilastre · 2024-03-18T16:57:28Z

src/views/orb-map-view.ts

@@ -89,6 +90,7 @@ export class OrbMapView<N extends INodeBase, E extends IEdgeBase> implements IOr
          this.render();
        }
      },
+      listeners: [this._update],


It would be great to add a comment why this line and not this. Same for map view.

I've added this comment in observer.utils.ts so we don't have to add it everywhere. Is it fine, or should I add it to the view files as well?

* New: Add state setters with options * Chore: Remove leftover comments * Chore: Refactor state setter logic * Chore: Refactor state types

…raph/orb into new/add-setters-and-getters

* New: Add new simulator (#56) * New: Add simulator scenarios for manual testing * New: Refactor simulator (WIP) * New: Add progress overlay, Update descriptions * Fix: Introduce new simulator event, Fix main-thread behavior * Fix: Rearrange class methods based on visibility * Fix: Improve naming * Chore(release): 0.2.0 * Fix: Tweak simulator, adjust API slightly * Fix: Temporarily patch some physics behavior * Fix: Adjust re-heating parameters * Fix: tweak physics behavior -> immediately stop sim when disabling * Chore: Remove the beta from release branches --------- Co-authored-by: dlozic <davidlozic@gmail.com> * New: Add zoom and recenter functions (#74) * New: Add zoom and recenter functions * Fix: Reduce excessive recentering (#75) * Fix: Remove excessive recenterings * Fix: Remove unused code * Fix: New simulator (#92) * Chore: Refactor naming * New: Add new events * Chore: Refactor code styling * Docs: Remove unused flags * Chore: Remove unused simulation functions * Chore: Refactor view render function calls * Chore: Add missing tests * New: Add removal functions (#96) * New: Add removal functions * Fix: Add missing callback data * Chore: Refactor remove return values * Chore: Refactor remove function type usage * Fix: Default settings for node placement (#98) * New: Add properties setters and getters (#93) * New: Add node properties setters * New: Add edge properties setters * New: Add properties getters * New: Add patch for nodes and edges * Fix: Make getters return copies * Fix: Edge factory listeners copying * Fix: Jest outdated tests * Fix: Github actions node version * Chore: Refactor observer interface * Chore: Refactor node/edge constructor settings * Chore: Refactor node/edge function grouping * Chore: Refactor node/edge function grouping * Fix: Listeners behaviour * Chore: Refactor property copying * Chore: Refactor subject implementation * Fix: Set position behaviour on node drag * Chore: Upgrade node version * Chore: Refactor function type check * Fix: Remove listener behaviour * Chore: Refactor util naming * Chore: Remove unused type assertion * Chore: Refactor position setter options * Chore: Refactor property patch function * Fix: Set map node position behaviour * Chore: Refactor simulator data patching * Chore: Change observers to callbacks * New: Add state setters with options (#95) * New: Add state setters with options * Chore: Remove leftover comments * Chore: Refactor state setter logic * Chore: Refactor state types * Fix: Rename merged function usage * Fix: Merged variable naming * Chore: Fix tests --------- Co-authored-by: dlozic <davidlozic@gmail.com> Co-authored-by: Oleksandr Ichenskyi <55350107+AlexIchenskiy@users.noreply.github.com> Co-authored-by: AlexIchenskiy <aichenskiy@gmail.com>

AlexIchenskiy added 6 commits February 26, 2024 07:53

New: Add node properties setters

5d4585f

New: Add edge properties setters

2ba6729

New: Add properties getters

bbe8797

New: Add patch for nodes and edges

0fe1ee7

Fix: Make getters return copies

c401f1e

Fix: Edge factory listeners copying

d328a1d

AlexIchenskiy added this to the v1.0.0 milestone Feb 28, 2024

AlexIchenskiy self-assigned this Feb 28, 2024

AlexIchenskiy commented Feb 28, 2024

View reviewed changes

src/models/observer.ts Outdated Show resolved Hide resolved

AlexIchenskiy commented Feb 28, 2024

View reviewed changes

src/models/node.ts Outdated Show resolved Hide resolved

AlexIchenskiy commented Feb 28, 2024

View reviewed changes

src/models/edge.ts Outdated Show resolved Hide resolved

AlexIchenskiy added 2 commits February 28, 2024 11:14

Fix: Jest outdated tests

13d90d9

Fix: Github actions node version

b8144bb

tonilastre reviewed Feb 28, 2024

View reviewed changes

AlexIchenskiy added 7 commits February 28, 2024 13:01

Chore: Refactor observer interface

91b913e

Chore: Refactor node/edge constructor settings

7fdfeb0

Chore: Refactor node/edge function grouping

1a28187

Chore: Refactor node/edge function grouping

81f5f14

Fix: Listeners behaviour

0b2549c

Chore: Refactor property copying

f6bbec5

Chore: Refactor subject implementation

3a8049f

tonilastre reviewed Mar 4, 2024

View reviewed changes

cizl reviewed Mar 6, 2024

View reviewed changes

src/renderer/canvas/edge/base.ts Outdated Show resolved Hide resolved

AlexIchenskiy added 3 commits March 7, 2024 13:11

Fix: Set position behaviour on node drag

386a076

Chore: Upgrade node version

040c487

Chore: Refactor function type check

5a43a56

AlexIchenskiy added 3 commits March 7, 2024 13:30

Fix: Remove listener behaviour

00257a3

Chore: Refactor util naming

05584e0

Chore: Remove unused type assertion

dd3909c

AlexIchenskiy commented Mar 7, 2024

View reviewed changes

tonilastre requested changes Mar 13, 2024

View reviewed changes

AlexIchenskiy mentioned this pull request Mar 14, 2024

New: Add state setters with options #95

Merged

AlexIchenskiy added 5 commits March 14, 2024 11:46

Chore: Refactor position setter options

7dc0fe7

Chore: Refactor property patch function

84921da

Fix: Set map node position behaviour

f9e13a0

Chore: Refactor simulator data patching

e574358

Chore: Change observers to callbacks

0ee733d

tonilastre approved these changes Mar 18, 2024

View reviewed changes

AlexIchenskiy and others added 7 commits March 20, 2024 15:24

New: Add state setters with options (#95)

c9de1cf

* New: Add state setters with options * Chore: Remove leftover comments * Chore: Refactor state setter logic * Chore: Refactor state types

Merge branch 'new/simulator-fixes' into new/add-setters-and-getters

d802e50

Merge branch 'new/simulator-fixes' into new/add-setters-and-getters

428f4d2

Fix: Rename merged function usage

8138f41

Merge branch 'new/add-setters-and-getters' of https://github.com/memg…

5d42df5

…raph/orb into new/add-setters-and-getters

Fix: Merged variable naming

d5dde27

Chore: Fix tests

81442f7

AlexIchenskiy marked this pull request as ready for review March 21, 2024 07:09

AlexIchenskiy merged commit 7e708a5 into new/simulator-fixes Mar 27, 2024
1 check passed

AlexIchenskiy deleted the new/add-setters-and-getters branch March 27, 2024 09:12

New: Add properties setters and getters #93

New: Add properties setters and getters #93

Conversation

AlexIchenskiy commented Feb 28, 2024

AlexIchenskiy commented Feb 28, 2024

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

tonilastre commented Feb 28, 2024

tonilastre left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cizl Mar 11, 2024 • 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

cizl Mar 14, 2024 • 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

tonilastre left a comment •

edited

Loading

cizl Mar 11, 2024 •

edited

Loading

cizl Mar 14, 2024 •

edited

Loading