Skip to content

Fix travelTime and backwardTravelTime textElement#1011

Open
louisgreiner wants to merge 1 commit into
mainfrom
lgr/fix-travel-times-positions
Open

Fix travelTime and backwardTravelTime textElement#1011
louisgreiner wants to merge 1 commit into
mainfrom
lgr/fix-travel-times-positions

Conversation

@louisgreiner

@louisgreiner louisgreiner commented May 12, 2026

Copy link
Copy Markdown
Contributor

Fixes the travel times positions. Bug can only be reproduce in a specific source-target position.

Before fix (bug, positions overlap):

Enregistrement.de.l.ecran.2026-05-12.185048.mp4

After fix:

Enregistrement.de.l.ecran.2026-05-12.184912.mp4

Bug can be reproduced using this: networkGraphic(9).json

@louisgreiner louisgreiner requested a review from a team as a code owner May 12, 2026 16:52
@louisgreiner louisgreiner force-pushed the lgr/fix-travel-times-positions branch from ee7e1dd to 034fcc3 Compare May 12, 2026 16:53
@louisgreiner louisgreiner changed the title Lgr/fix travel times positions Fix travelTime and backwardTravelTime textElement May 13, 2026
new DirectedTrainrunSectionProxy(this.selectedTrainrunSection, direction),
this.timeStructure,
);
this.trainrunSectionService.updateText();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is already called from inside setTimeStructureToSingleTrainrunSection(), do we need to do it twice?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Mmh I don't see where this is called in setTimeStructureToSingleTrainrunSection(), but it seems that this call can be removed anyway, since I see surprisingly no difference with or without.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh, I mixed up with setTimeStructureToTrainrunSections().

I find it surprising that setTimeStructureToTrainrunSections() calls the function, while setTimeStructureToSingleTrainrunSection() doesn't.

this.trainrunSectionTimesUpdated(pair.trainrunSection);
}

this.updateText();

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Travel times may be updated for many reasons, e.g. time propagation, transition undocking, and so on. This only covers a narrow case.

Additionally, this is a pretty big hammer - it affects all trainrun sections.

I think we should try to find a better way to fix this, e.g. by removing isBackwardTravelTimeDisplayed in placeTextOnTrainrunSection().

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I totally agree. However, I have no clue on how to proceed elsewise, since the text positions are directly bound with the travel times values. Also, this update logic will be removed in #654.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the delay, I've come up to this fixup commit (7c68321), following your advice. It works well :)

Enregistrement.de.l.ecran.2026-05-28.112709.mp4

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Would the fixup commit resolve this thread?

@Uriel-Sautron Uriel-Sautron left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Tested, the bug has been fixed

@louisgreiner louisgreiner moved this to In Progress in Board PI 20 May 26, 2026
@louisgreiner louisgreiner self-assigned this May 26, 2026
@louisgreiner louisgreiner moved this from In Progress to Awaiting merge in Board PI 20 May 28, 2026
@louisgreiner louisgreiner requested a review from emersion May 28, 2026 09:48
…lapping)

Because if travelTime === backwardTravelTime, if we start from that state and
press +/-1, the positions are not updated and times overlap.
By doing so, positions are recalculated at each update.

Signed-off-by: Louis Greiner <louis.greiner@proton.me>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Awaiting merge

Development

Successfully merging this pull request may close these issues.

4 participants