Skip to content

Conversation

@louisgreiner
Copy link
Contributor

@louisgreiner louisgreiner commented Oct 24, 2025

Description

Feature not perfect, but it's such a rabbit hole I could implement / fix features for a century.

Note

About the consecutive times propagation: I keep the existing code identical since travelTime.consecutiveTime and backwardTravelTime.consecutiveTime are never called. However these might be nice be to refactored. (propagateInitialConsecutiveTimes, propagateTrainrunInitialConsecutiveTimes, propagateConsecutiveTimesForTrainrun, propagateConsecutiveTimes) => TimeLockDto is a bigger than the needs of the travel times

Ref #418

For reviewing: https://sncf.sharepoint.com/:p:/r/sites/OSRD644GrpO365/_layouts/15/Doc.aspx?sourcedoc=%7B68C024EF-2C05-4B4D-86C1-ED205F966F2F%7D&file=Cas%20modifs%20temps%20NGE.pptx&action=edit&mobileredirect=true

Missing documentation / video perhaps

Issues

Some bugs (shaky features) to fix:

  • symmetry recalculation on non-stop (only reset symmetry on first or last section currently)
  • BackwardTravelTime text position on reticular bugged (inverted sometimes with the travelTime)

Checklist

  • This PR contains a description of the changes I'm making
  • I've read the Contribution Guidelines
  • I've added tests for changes or features I've introduced
  • I documented any high-level concepts I'm introducing in documentation/
  • CI is currently green and this is ready for review

@aiAdrian aiAdrian added this to the 2.11 milestone Oct 24, 2025
@aiAdrian
Copy link
Contributor

I try to review it and we have to understand when and how we like to merge.

I see currently three release/merge path:

  1. merge just after release of 2.10
  2. Integrate it as soon as possible
  3. integrate it as soon as possible but disable asym feature to user to avoid unfinished buggy feature

I personally would prefere vraiant 1) - TBD

@louisgreiner
Copy link
Contributor Author

I agree, I would prefer to merge after the 2.10 release as well. This review might take some time

@louisgreiner
Copy link
Contributor Author

For the bug symmetry reset I edited:

symmetry recalculation on non-stop (only reset symmetry on first or last section currently)
SOLUTION : can we merge this as a new feature after the first merge?

@aiAdrian @emersion what do you think? It will simplify the review

@louisgreiner
Copy link
Contributor Author

louisgreiner commented Nov 4, 2025

Edit: I set it to 14px, tell me what you think of this

Note: About the last commit: asymmetry: fix travel times and name overlapping, I'm still hesistating if we should also reduce the font size from 16px to 14px for name and travel times
@aiAdrian @maelysLeratRosso

16px:
image

14px (I think I prefer this one):
image

@louisgreiner louisgreiner force-pushed the lgr/asymmetry-review branch 2 times, most recently from 66f9626 to 6a7c571 Compare November 4, 2025 15:36
@louisgreiner louisgreiner force-pushed the lgr/asymmetry-review branch 3 times, most recently from 48395b7 to 04b9e8b Compare November 13, 2025 16:52
This was referenced Nov 19, 2025
Copy link
Member

@emersion emersion left a comment

Choose a reason for hiding this comment

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

A few comments I've written when I've started reviewing below.

Getters, setters and util functions will be usefull later.

Fixed by the way an old wrong rebase on reconnect tests. Sorry for that.

Also fixed the associated tests, which updated other fields (like resources) that have been forgotten to be updated by 'fix: Resource deletion when node gets deleted'.

Signed-off-by: Louis Greiner <[email protected]>
Signed-off-by: Louis Greiner <[email protected]>
We could also reduce the font size for travel times and name texts: $LINE_FONT_SIZE: 16px; -> $LINE_FONT_SIZE_REDUCED: 14px;

Signed-off-by: Louis Greiner <[email protected]>
Adapted trainrun-section-tab and perlenkette for bottomTravelTime, and made backwardTravelTime displayed only if different of travelTime.

Signed-off-by: Louis Greiner <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants