New: Add state setters with options #95

AlexIchenskiy · 2024-03-11T13:21:22Z

Resolves #68

As an enhancement to the freshly implemented getters and setters, this PR introduces a new way to set the state of nodes and edges using objects that specify state with additional parameters such as isSingle and isToggle. Now, it is possible to use both the regular setter:

node.setState(state)

and the new setter:

node.setState({ state: GraphObjectState.SELECTED, options: { isSingle: true } })

edge.setState({ state: GraphObjectState.SELECTED, options: { isSingle: false, isToggle: true } })

and so on. This provides more customization flexibility to the user, with default values for these parameters set to false. It should be easily extendable for adding even more options.

AlexIchenskiy · 2024-03-11T13:27:45Z

src/models/graph.ts

+      if (data.type === 'node' && data.options.isSingle) {
+        const nodes = this._nodes.getAll();
+
+        for (let i = 0; i < nodes.length; i++) {


I feel like these iterations through all nodes/edges can be slow. Any ideas? And do we want to clear the state of all nodes/edges or only the adjusting ones?

How about using hash maps? Obviously, this would require a lot of code change. Just starting a discussion, I don't think it's doable right now.

For the second question, I would say only the ones that are being adjusted.

Or maybe to supplement this nodes array we can keep an index where nodeIndex[node.id] === nodes.findIndex((el) => el.id === node.id)

Are you suggesting storing nodes in a hashmap? As far as I know, retrieving elements from an array by a known index is O(1) complexity, so iterating through an array vs a hashmap (which also has O(1) complexity for retrieving element by key) may not make a big difference here. I might have misunderstood your point, or perhaps I missed something, so feel free to clarify it further :)

If all nodes are in state X, and you want to clear them up if they are not node with ID 1, you still need to to N - 1 updates. So hash maps or any other structure won't help there, you already have the fastest structure, an array.

But, you can do this and increase the memory footprint: I am not sure if it makes sense to go into this right now, but this would be the plan:

Graph keeps a structure of nodes/edges by state, so each state has a structure of nodes and edges (probably just id) - a set of a map.

States in nodes and edges are just views, readonly

When state is updated in node, edge it is just updated in the graph

This way, if you have isSingle: true, graph, as an owner of the data can then literally take all selected nodes from a set or a map, clear those states and set the new node as single selected. This would improve the speed, but it would worse the memory footprint and code simplitiy because owner of the state becomes the graph, not graph objects (node/edge).

I would keep this as is and fix only if we see that it hurts performance.

Sorry, I misunderstood the functionality. I saw === instead of !==. I thought you were trying to clear the state of anode with a specific id.

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

src/models/edge.ts

+      if (options && options.isToggle) {
+        this._toggleState(result.state);
+      } else {
+        this._state = result.state;
+      }


Either have this._state = <function that handles options in general>(results.state) where function doesn't mutate the state, but returns what the new state should be, or keep this as is, but still change the toggleState to not mutate the this._state because I don't see a reason why it should. The best functions are the clean ones, which do not mutate state (if it is possible to make them).

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

src/models/edge.ts

+    if (typeof result === 'number') {
+      this._state = result;
+    } else if (typeof result === 'object') {
+      const options = result.options;


For these checks, please use utils/type.utils.ts because of:

cleaner code, isNumber vs `typeof x === 'number'

handles edge cases, isPlainObject vs typeof result === 'object' (edge case is: typeof null will return 'object').

tonilastre · 2024-03-13T17:13:31Z

src/models/node.ts

+      result = arg;
+    }
+
+    if (typeof result === 'number') {


Same comments as for edges.

tonilastre · 2024-03-13T17:15:49Z

src/models/state.ts

+export interface IGraphObjectStateOptions {
+  isToggle?: boolean;
+  isSingle?: boolean;
+}


In these cases, when it is options, it would be best not to have optionals here, but where you use it, line 17: Partial<IGraphObjectStateOptions> where options can also be optional, e.g. options?: Partial<...>.

If you move external logic (having optional params) too deep into the type, it is harder to handle that in some other cases. E.g. if you wish to create default params, you could then just say: const default: IGraphObjectStateOptions = { ... } and you would be forced to define both. If you would use options from Parameters you would see that this object is optional here with all its children. The context of when is something optional, when it isn't is being kept.

tonilastre · 2024-03-13T17:20:04Z

src/models/graph.ts

+      if (data.type === 'node' && data.options.isSingle) {
+        const nodes = this._nodes.getAll();
+
+        for (let i = 0; i < nodes.length; i++) {


If all nodes are in state X, and you want to clear them up if they are not node with ID 1, you still need to to N - 1 updates. So hash maps or any other structure won't help there, you already have the fastest structure, an array.

But, you can do this and increase the memory footprint: I am not sure if it makes sense to go into this right now, but this would be the plan:

Graph keeps a structure of nodes/edges by state, so each state has a structure of nodes and edges (probably just id) - a set of a map.

States in nodes and edges are just views, readonly

When state is updated in node, edge it is just updated in the graph

This way, if you have isSingle: true, graph, as an owner of the data can then literally take all selected nodes from a set or a map, clear those states and set the new node as single selected. This would improve the speed, but it would worse the memory footprint and code simplitiy because owner of the state becomes the graph, not graph objects (node/edge).

I would keep this as is and fix only if we see that it hurts performance.

tonilastre · 2024-03-13T17:20:47Z

src/models/graph.ts

+    if (data && 'type' in data && 'options' in data && 'isSingle' in data.options) {
+      if (data.type === 'node' && data.options.isSingle) {


Can you maybe simplify this to if (data && data.options?.isSingle && data.type === 'node')?

I believe it could be possible, but only after reworking IObserverDataPayload into a more generic solution as you mentioned here. Currently, I am using some sort of custom type guard because of the interfaces used in that payload

tonilastre · 2024-03-18T16:52:22Z

src/models/edge.ts

    this.notifyListeners();
  }
+
+  private _handleState(state: number, options?: Partial<IGraphObjectStateOptions>): number {


It is nitpicking really, but maybe getNextState would be a better name.

…-with-options

* 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

* 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 2 commits March 11, 2024 13:33

New: Add state setters with options

9735ce6

Chore: Remove leftover comments

a252f3b

AlexIchenskiy self-assigned this Mar 11, 2024

AlexIchenskiy requested review from cizl and tonilastre as code owners March 11, 2024 13:21

AlexIchenskiy commented Mar 11, 2024

View reviewed changes

tonilastre requested changes Mar 13, 2024

View reviewed changes

AlexIchenskiy added 2 commits March 14, 2024 08:56

Chore: Refactor state setter logic

f94769a

Chore: Refactor state types

1022aa9

AlexIchenskiy added this to the v1.0.0 milestone Mar 18, 2024

tonilastre approved these changes Mar 18, 2024

View reviewed changes

Merge branch 'new/add-setters-and-getters' into new/add-state-setters…

a5c60a1

…-with-options

AlexIchenskiy merged commit c9de1cf into new/add-setters-and-getters Mar 20, 2024
1 check passed

AlexIchenskiy deleted the new/add-state-setters-with-options branch March 20, 2024 14:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New: Add state setters with options #95

New: Add state setters with options #95

AlexIchenskiy commented Mar 11, 2024

AlexIchenskiy Mar 11, 2024

cizl Mar 12, 2024 •

edited

Loading

cizl Mar 12, 2024 •

edited

Loading

AlexIchenskiy Mar 13, 2024

tonilastre Mar 13, 2024

cizl Mar 14, 2024

tonilastre Mar 13, 2024

tonilastre Mar 13, 2024

tonilastre Mar 13, 2024

tonilastre Mar 13, 2024

tonilastre Mar 13, 2024

tonilastre Mar 13, 2024

AlexIchenskiy Mar 14, 2024

tonilastre Mar 18, 2024

		if (data && 'type' in data && 'options' in data && 'isSingle' in data.options) {
		if (data.type === 'node' && data.options.isSingle) {

New: Add state setters with options #95

New: Add state setters with options #95

Conversation

AlexIchenskiy commented Mar 11, 2024

Choose a reason for hiding this comment

cizl Mar 12, 2024 • edited Loading

Choose a reason for hiding this comment

cizl Mar 12, 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

cizl Mar 12, 2024 •

edited

Loading

cizl Mar 12, 2024 •

edited

Loading