feat: modify node display #554

Math-R · 2025-10-07T12:50:12Z

Description

This PR implements the collapsed nodes functionality for the Netzgrafik editor, allowing intermediate nodes to be hidden while maintaining proper visual representation of train routes.

Features Implemented

1. Node Model Enhancement

Added isCollapsed field to Node model with getter/setter methods
Integrated collapsed state into node serialization/deserialization
Maintains backward compatibility with existing data

2. Visual Filtering System

NodesView: Filter out collapsed nodes from visual display
ConnectionsView: Hide connections associated with collapsed nodes
TransitionsView: Hide transitions for collapsed nodes
Collapsed nodes become completely invisible in the editor

3. Intelligent Section Grouping

TrainrunSectionService.groupTrainrunSectionsIntoChains(): Algorithm to group sections with collapsed intermediate nodes
Automatically detects chains like A→B→C→D where B,C are collapsed
Creates grouped objects with proper start/end node references

4. Direct Path Calculation

TrainrunSectionsView.createViewObjectForCollapsedChain(): Creates direct visual paths A→D bypassing collapsed nodes
Automatic routing that circumvents collapsed intermediate nodes
Proper port calculation for direct connections

User Experience

Nodes marked as collapsed become invisible
Train routes A→B→C→D (with B,C collapsed) display as direct A→D connections
Interface remains coherent and schedules are preserved
No impact on existing data or workflows

Technical Implementation

Zero breaking changes to existing data structures
Clean separation between data model and visual representation
Proper TypeScript typing throughout

Issues

Related to feature request for collapsed nodes functionality to simplify complex network display.

Checklist

This PR contains a description of the changes I'm making
I've read the Contribution Guidelines
Core functionality implemented and tested: Node filtering, section grouping, path calculation working
I've added tests for changes or features I've introduced (comprehensive tests planned for follow-up PR)
I documented any high-level concepts I'm introducing in documentation/ (documentation to be added in follow-up)
TypeScript compilation clean: No compilation errors
Backward compatibility preserved: No breaking changes to existing data
Ready for functional testing: Core collapsed nodes feature fully operational

Next Steps (Future PRs)

Node creation UI for collapsed nodes
Legacy file compatibility (numberOfStops conversion)
Comprehensive unit test suite
User documentation and examples
Port assignment optimization for parallel routing

aiAdrian · 2025-10-07T19:42:18Z

Thanks you for the new feature - would it be possible to get a live demo?

aiAdrian · 2025-10-07T19:50:16Z

I haven't done a full review yet, but at first glance I don't understand why:

TrainrunSectionViewObject → there's no change trigger for isCollapsed
NodeViewObject → there's no change trigger for isCollapsed

It seems there's no change status (generateKey) related to isCollapsed. These objects are supposed to signal that something has changed so that a visual update can be triggered (i.e., only render what has changed to improve performance). I assume that if we don't explicitly trigger the update (mark the object as changed), it won't be updated (rendered).

  displayNodes(inputNodes: Node[]) {
    const nodes = inputNodes.filter(
      (n) =>
        this.editorView.doCullCheckPositionsInViewport([
          new Vec2D(n.getPositionX(), n.getPositionY()),
          new Vec2D(n.getPositionX() + n.getNodeWidth(), n.getPositionY()),
          new Vec2D(n.getPositionX(), n.getPositionY() + n.getNodeHeight()),
          new Vec2D(n.getPositionX() + n.getNodeWidth(), n.getPositionY() + n.getNodeHeight()),
        ]) && this.filterNodesToDisplay(n),
    );

    const group = this.nodeGroup
      .selectAll(StaticDomTags.NODE_ROOT_CONTAINER_DOM_REF)
      .data(this.createViewNodeDataObjects(nodes), (n: NodeViewObject) => n.key);

...

(see: https://github.com/OpenRailAssociation/netzgrafik-editor-frontend/blob/main/src/app/view/editor-main-view/data-views/nodes.view.ts#L106)

It would be very helpful if we could add some tests to verify that changes to isCollapsed and it's functionality are functioning as intended.

Even if this doesn't directly affect the visualization layer, it’s still important for the underlying service methods, especially in areas like data migration. The base functionality should be properly tested to ensure robustness and maintainability.

Synar · 2025-10-09T01:45:25Z

src/app/view/editor-main-view/data-views/connections.view.ts


    const node: Node = this.editorView.getNodeFromConnection(con);
+
+    // filter if node is collapsed - do not show connections for collapsed nodes


Tiny nit: why the comment here and not in transitions.view.ts?

louisgreiner · 2025-10-13T11:43:53Z

Context: This PR implements part of the 3 first sections of the implementation plan given here

louisgreiner

LGTM, not tested.

Maybe, instead of fully hiding the nodes when isCollapsed is true, you should display them as a "Dot", as it is shown here (mock-up from main issue). But maybe you've planned to do it in the next PR, ignore this comment if so.

src/app/view/editor-main-view/data-views/nodes.view.ts

src/app/services/data/trainrunsection.service.ts

emersion

Thanks for splitting the changes into small commits! Here are a few comments.

src/app/view/editor-main-view/data-views/trainrunsections.view.ts

src/app/services/data/trainrunsection.service.ts

Synar

Made a pass, nothing to add for now. I'll do another pass after you resolved @emersion's comments.

Math-R · 2025-10-16T13:10:45Z

I haven't done a full review yet, but at first glance I don't understand why:

TrainrunSectionViewObject → there's no change trigger for isCollapsed

NodeViewObject → there's no change trigger for isCollapsed

It seems there's no change status (generateKey) related to isCollapsed. These objects are supposed to signal that something has changed so that a visual update can be triggered (i.e., only render what has changed to improve performance). I assume that if we don't explicitly trigger the update (mark the object as changed), it won't be updated (rendered).
  displayNodes(inputNodes: Node[]) {
    const nodes = inputNodes.filter(
      (n) =>
        this.editorView.doCullCheckPositionsInViewport([
          new Vec2D(n.getPositionX(), n.getPositionY()),
          new Vec2D(n.getPositionX() + n.getNodeWidth(), n.getPositionY()),
          new Vec2D(n.getPositionX(), n.getPositionY() + n.getNodeHeight()),
          new Vec2D(n.getPositionX() + n.getNodeWidth(), n.getPositionY() + n.getNodeHeight()),
        ]) && this.filterNodesToDisplay(n),
    );

    const group = this.nodeGroup
      .selectAll(StaticDomTags.NODE_ROOT_CONTAINER_DOM_REF)
      .data(this.createViewNodeDataObjects(nodes), (n: NodeViewObject) => n.key);

... 
(see: https://github.com/OpenRailAssociation/netzgrafik-editor-frontend/blob/main/src/app/view/editor-main-view/data-views/nodes.view.ts#L106)

It would be very helpful if we could add some tests to verify that changes to isCollapsed and it's functionality are functioning as intended.

Even if this doesn't directly affect the visualization layer, it’s still important for the underlying service methods, especially in areas like data migration. The base functionality should be properly tested to ensure robustness and maintainability.

Sorry for the late answer. i'm not sure to completly understand your first point. And sure, I will add some test in the next commit

emersion

I haven't time to read this in full yet, sorry. Here are some comments still.

As mentioned in the implementation plan, I don't think keeping a TrainrunSectionViewObject.trainrunSection is great. I think replacing it with a trainrunSections: TrainrunSection[] field would ensure we never grab information from the first trainrun when we would need to grab it from the last, or from intermediate sections. For instance the target arrival time as mentioned below in comments, but applies to others as well.

src/app/services/data/trainrunsection.service.ts

src/app/view/editor-main-view/data-views/trainrunSectionViewObject.ts

src/app/view/editor-main-view/data-views/trainrunsections.view.ts

Signed-off-by: Simon Ser <[email protected]>

The TrainrunSectionViewObject already contains the full chain, no need to re-compute it. Signed-off-by: Simon Ser <[email protected]>

…Key() We were only using the first trainrun section's path here. Expose a getPath() method to get a view object's path. We need to call it from inside generateKey() so drop the static attribute. While at it, make the generateKey() helper function private because it shouldn't be called from elsewhere. Signed-off-by: Simon Ser <[email protected]>

Target arrival was included twice, and source arrival was missing. Signed-off-by: Simon Ser <[email protected]>

We were only grabbing metadata from the first section, but we need to make the key change when the last section changes. Signed-off-by: Simon Ser <[email protected]>

Instead, grab the path from TrainrunSectionViewObject.getPath(). We need to update a few functions to take the veiw object instead of the first trainrun section. Signed-off-by: Simon Ser <[email protected]>

This function overwrites a trainrun section's path. Instead, pick the right section (first or last) when computing path-related outputs. Signed-off-by: Simon Ser <[email protected]>

Further down, these two calls return the same values: this.getTrainrun().getTrainrunFrequency().frequency this.getTrainrun().getTrainrunFrequency().offset Signed-off-by: Simon Ser <[email protected]>

…ect.generateKey() Only the source matters for the first section, the target is handled by the last section of the chain. Same as this commit, but for the consecutive time: 9689208 Signed-off-by: Simon Ser <[email protected]>

…ainrunSectionViewObject

…WithCollapsed support to avoid duplication

emersion · 2025-11-19T09:53:44Z

src/app/view/editor-main-view/data-views/trainrunsections.view.ts

+        case TrainrunSectionText.TrainrunSectionTravelTime:
+          // Special case for multiple sections: calculate total time including stop times at intermediate nodes
+          if (viewObject.trainrunSections.length > 1) {
+            const totalTime = viewObject.trainrunSections.reduce((sum, section, index) => {


We could move this into TrainrunSectionViewObject.getTravelTime(), and call it from here. That way, we fix bugs in getTravelTime() and reduce code duplication.

emersion · 2025-11-19T13:09:05Z

src/app/view/editor-main-view/data-views/trainrunsections.view.ts


  static getTrainrunSectionValueToShow(
-    trainrunSection: TrainrunSection,
+    trainrunSectionOrViewObject: TrainrunSection | TrainrunSectionViewObject,


I don't think we need to accept a raw TrainrunSection here: this sounds like a footgun where some callers would just pass the first section and ignore the rest. We can always take a TrainrunSectionViewObject as input (and that object may contain a single section).

Sure i made a refacto of this method

see 8ea0098

emersion · 2025-11-19T13:09:51Z

src/app/view/editor-main-view/data-views/trainrunsections.view.ts

+      // ViewObject case: determine which section to use based on textElement
+      const viewObject = trainrunSectionOrViewObject;
+
+      switch (textElement) {


I think this can be merged with the switch below. We don't need to special-case multiple trainrun sections.

same as above see 8ea0098

emersion · 2025-11-19T13:12:36Z

src/app/view/editor-main-view/data-views/trainrunsections.view.ts

+    const firstSection = viewObject.trainrunSections[0];
+    const lastSection = viewObject.trainrunSections.at(-1)!;
+
+    // Handle multi-section chains (collapsed nodes)


I don't think we need to special-case view objects with multiple sections. We should be able to adapt the codepath below the if block and drop the if block, for instance:

const trgNode = lastSection.getTargetNode();

const path = viewObject.getPath();

Sure changed it

emersion · 2025-11-19T13:14:33Z

src/app/view/editor-main-view/data-views/trainrunsections.view.ts

  static translateAndRotateText(
    trainrunSection: TrainrunSection,
    trainrunSectionText: TrainrunSectionText,
+    viewObject?: TrainrunSectionViewObject,


Can we completely replace the trainrunSection argument with the viewObject argument?

I think we can but, this changes would affect multiple functions and tests, and the main limitation is that createNumberOfStopsTextElement currently doesn’t have access to a viewObject.

Since this PR already includes quite a bit of refactoring, it might be cleaner to handle that work in a further PR.

WDYT?

emersion · 2025-11-19T13:14:56Z

src/app/view/editor-main-view/data-views/trainrunsections.view.ts

+    // Use viewObject path if provided, otherwise use trainrunSection path
+    const pathVec2D: Vec2D[] = viewObject
+      ? viewObject.trainrunSections[0].getPath()
+      : trainrunSection.getPath();


Can't we use viewObject.getPath() here?

Sure, actually i think it's cleaner thanks

…inrunSection param -Remove TrainrunSection parameter support (accept only TrainrunSectionViewObject) -Merge duplicate switch statements into single switch -Merge Source/Target and Departure/Arrival into single case

-use complete path for collapsed nodes in text rotation

louisgreiner

Functional review

Using this scenario (no collapsed node):
reticulaire.json

And this scenario (collapsed node in Olten):
reticulaire(1).json

Copy/paste a node selection also takes the collapsed nodes -> it should not

Enregistrement.de.l.ecran.2025-11-20.142852.mp4

In the same scenario, trainruns are drawn weirdly (ports) (but may it be caused because of import/export? should try to rebase onto Valentin's branch)

It looks like the ports assignation is mixed up

Times update works great

louisgreiner · 2025-11-21T12:47:26Z

The trainrun deletion using a selected trainrun then "DEL" key is broken

louisgreiner · 2025-11-21T12:50:37Z

Comparing two network graphics:

The path drawn between RTR and ZF in the collapsed case is weird. It should be drawn that way (I just created a new trainrun between these 2 nodes):

louisgreiner · 2025-11-21T12:55:28Z

There is also a small bug in the node selection:

Enregistrement.de.l.ecran.2025-11-21.135157.mp4

The workflow should be:

if node selection is composed of expanded and collapsed node, then keep all the nodes in the node selection
- then if CTRL+D (duplication), duplicate everything
  - Note: there is a also a bug here, duplication does not duplicate node with the right isCollapsed value
if node selection is composed of only collapsed nodes, then do not push these nodes in the node selection (as it is done for filtered node selection)
- then no action is possible

Math-R force-pushed the mrd/modify-node-display branch 4 times, most recently from 087bc7c to 185666b Compare October 7, 2025 16:41

Math-R marked this pull request as ready for review October 7, 2025 16:41

Math-R requested a review from aiAdrian as a code owner October 7, 2025 16:41

Math-R self-assigned this Oct 7, 2025

Math-R requested review from Synar, emersion and louisgreiner October 7, 2025 16:42

Synar reviewed Oct 9, 2025

View reviewed changes

louisgreiner approved these changes Oct 13, 2025

View reviewed changes

src/app/view/editor-main-view/data-views/nodes.view.ts Show resolved Hide resolved

src/app/services/data/trainrunsection.service.ts Outdated Show resolved Hide resolved

emersion reviewed Oct 13, 2025

View reviewed changes

src/app/view/editor-main-view/data-views/trainrunsections.view.ts Outdated Show resolved Hide resolved

src/app/services/data/trainrunsection.service.ts Outdated Show resolved Hide resolved

src/app/services/data/trainrunsection.service.ts Show resolved Hide resolved

Synar reviewed Oct 14, 2025

View reviewed changes

Math-R force-pushed the mrd/modify-node-display branch 3 times, most recently from c4cc009 to 6145bff Compare October 16, 2025 13:06

Math-R force-pushed the mrd/modify-node-display branch 2 times, most recently from a1d2b05 to fa9b231 Compare October 16, 2025 15:03

Math-R requested review from Synar, emersion and louisgreiner October 16, 2025 15:04

emersion reviewed Oct 17, 2025

View reviewed changes

Math-R force-pushed the mrd/modify-node-display branch from fa9b231 to 48fd522 Compare October 22, 2025 12:39

louisgreiner mentioned this pull request Oct 23, 2025

Backward and non-stop backward iterators #586

Merged

3 tasks

Math-R force-pushed the mrd/modify-node-display branch from 48fd522 to 871f667 Compare October 28, 2025 11:20

emersion and others added 16 commits November 17, 2025 15:56

fixup! simplify text nodes position computation

a32d2cc

Signed-off-by: Simon Ser <[email protected]>

fixup! drop unnecessary getCollapsedChainValueToShow() wrapper

b28b18f

Signed-off-by: Simon Ser <[email protected]>

fixup! drop getAllSectionsInCollapsedChain()

bc3998d

The TrainrunSectionViewObject already contains the full chain, no need to re-compute it. Signed-off-by: Simon Ser <[email protected]>

Fix missing source arrival in TrainrunSectionViewObject.generateKey()

6658713

Target arrival was included twice, and source arrival was missing. Signed-off-by: Simon Ser <[email protected]>

fixup! use last section in TrainrunSectionViewObject.generateKey()

8df2822

We were only grabbing metadata from the first section, but we need to make the key change when the last section changes. Signed-off-by: Simon Ser <[email protected]>

fixup! drop TrainrunSectionsView.getCollapsedChainPath()

e127d6b

Instead, grab the path from TrainrunSectionViewObject.getPath(). We need to update a few functions to take the veiw object instead of the first trainrun section. Signed-off-by: Simon Ser <[email protected]>

fixup! drop updateTrainrunSectionPathForCollapsedChain()

09524ec

This function overwrites a trainrun section's path. Instead, pick the right section (first or last) when computing path-related outputs. Signed-off-by: Simon Ser <[email protected]>

Drop duplicate frequency in TrainrunSectionViewObject.generateKey()

dd80b4c

Further down, these two calls return the same values: this.getTrainrun().getTrainrunFrequency().frequency this.getTrainrun().getTrainrunFrequency().offset Signed-off-by: Simon Ser <[email protected]>

Implement new getTravelTime method to compute total travel time to Tr…

1aa6c9a

…ainrunSectionViewObject

Merge getTrainrunSectionValueToShow and getTrainrunSectionValueToShow…

8aa047c

…WithCollapsed support to avoid duplication

Drop applyBasicFiltering after merging the logic in transformPath

4be737e

refactor: throw error on cycle detection in trainrun section chains

ed4f922

refactor: merge duplicate text positioning functions

2279e88

fix: fix the calculation of collapsedsection total travel time

2ae95ab

emersion reviewed Nov 19, 2025

View reviewed changes

Math-R mentioned this pull request Nov 19, 2025

fix: update the tests from trainrunsections.view.spec and data.view.s… #625

Open

emersion reviewed Nov 19, 2025

View reviewed changes

emersion self-requested a review November 19, 2025 13:15

Math-R force-pushed the mrd/modify-node-display branch from f01f31b to 2ae95ab Compare November 19, 2025 16:05

Math-R added 3 commits November 19, 2025 17:08

Update trainrunsectionViewObject.gettraveltime to handle stop times

203e3ad

refactor: remove special case for multi-section paths in transformPath

328e7e5

-use complete path for collapsed nodes in text rotation

Math-R requested a review from clarani November 20, 2025 08:40

louisgreiner reviewed Nov 20, 2025

View reviewed changes

fix copy/paste behaviour to keep node collapsed if they are

3f64cfc


		const node: Node = this.editorView.getNodeFromConnection(con);

		// filter if node is collapsed - do not show connections for collapsed nodes

feat: modify node display #554

Are you sure you want to change the base?

feat: modify node display #554

Conversation

Math-R commented Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Features Implemented

1. Node Model Enhancement

2. Visual Filtering System

3. Intelligent Section Grouping

4. Direct Path Calculation

User Experience

Technical Implementation

Issues

Checklist

Next Steps (Future PRs)

Uh oh!

aiAdrian commented Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aiAdrian commented Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

louisgreiner commented Oct 13, 2025

Uh oh!

louisgreiner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

emersion left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Synar left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Math-R commented Oct 16, 2025

Uh oh!

emersion left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

louisgreiner left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Math-R commented Oct 7, 2025 •

edited

Loading

aiAdrian commented Oct 7, 2025 •

edited

Loading

aiAdrian commented Oct 7, 2025 •

edited

Loading

Synar left a comment •

edited

Loading

louisgreiner left a comment •

edited

Loading