TimesStatusPanel: Cut seconds of for the final flight time calculation.

[hjr]

closes #957

[MaxKellermann]

Is this robust against midnight rollover?

[MaxKellermann]

It's not robust against midnight rollover. One mistake is your new FromSinceMidnight() implementation, and in general your use of class RoughTime. That class cannot have values beyond 24 hours. Therefore, you cannot use it to calculate time stamps which may have exceeded to the next day (which is very common in certain time zones).
Your code is not only complicated and slow, but also wrong.

[hjr]

I was uncertain myself if that will be 24h rollover safe and tested it. The outcome with the igc replay was positive. Starting one day landing the next day is handled correctly.
Generally I was also not happy to see this limitation in the time stamp implementation serving as a time base for xcs. But that is worth a discussion. Will kick this off.
Thank you, Max, for reviewing carefully.

[hjr]

Yes it is. takeoff_time is of type TimeStamp that is rollover hard, FromSinceMidnight uses the plain constructor of RoughTime , which does allow >24h values. There should actually no time container that is limited to 24h in XCS. Am 29.08.22 um 09:59 schrieb Max Kellermann:

***@***.**** commented on this pull request. ------------------------------------------------------------------------ In src/Dialogs/StatusPanels/TimesStatusPanel.cpp <#1027 (comment)>: > + RoughTime rough_takeoff_time = RoughTime::FromSinceMidnight(flight.takeoff_time); + RoughTime rough_landing_time = RoughTime::FromSinceMidnight(flight.landing_time); + SetText(FlightTime, FormatSignedTimeHHMM(FloatDuration(rough_landing_time-rough_takeoff_time))); Is this robust against midnight rollover? — Reply to this email directly, view it on GitHub <#1027 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADFKNMLEOJMQWORVHFSWC3V3RUVBANCNFSM57345MOA>. You are receiving this because you authored the thread.Message ID: ***@***.***>

-- Helmut Rohs t: +49 761 887 990 24 m: +49 151 250 505 44

[MaxKellermann]

There should actually no time container that is limited to 24h in XCS.

But there is. The API contract of RoughTime is that it "stores a time of day with minute granularity". Whatever behaviour you observed, it is not documented, outside of the specification and thus undefined behavior. The internal implementation may change any time, and such a change must not break any code. That kind of fragile code must be avoided.

The whole roundtrip from std::chrono to RoughTime and back to std::chrono you introduced is not necessary.

We could, of course, change the API contract of RoughTime, but since your use of it is just unnecessary overhead, I'd decide not to.

[hjr]

Ok Max. What I expect from a positively executed review is a indication to a favoured solution instead of just the statement: I dont like the way you did it.
You are pushing me to just guess what would be your taste and try a different solution. This trial and error is not very target oriented. So before I go and change the implementation here my suggestion.
The challenge: We need a on minutes chopped (not rounded) time representation with epoch 0am of the takeoff day.
The options:

1. Closest implementation is RoughTime: Remove the <24h restriction from it, use it.
2. 2nd closest is TimeStamp: Add the ChopSeconds method and use it.
3. Not use any encapsulation and put the few lines into the module TimesStatusPanel.cpp in need.
4. Create a new implementation "RoughStamp" and have the required implementation there.
   There would certainly numerous more ways to go, pls advise .. no trial and error!

[MaxKellermann]

Ok Max. What I expect from a positively executed review is a indication to a favoured solution instead of just the statement: I dont like the way you did it. You are pushing me to just guess what would be your taste and try a different solution. This trial and error is not very target oriented.

Finding good solutions for software problems is hard. You made mistakes (everybody makes mistakes, no big deal), but these mistakes are not about my taste or my preference. You violated the documented API contract of RoughTime (not my taste, but a fact), and additionally, the performance of your solution was inferior.

Note that inferior performance is also not about taste; that can be proven. Inferior performance is not necessarily an obstacle for merging. But in my taste, unnecessary performance degradation should be improved. In this case, the bad performance of your code was unnecessary and your (unconscious) choice.

Improving one's code until it is good enough is the normal lifecycle of a PR. What you never see is how long it takes me to polish my patches before they are ready for merging, because (unfortunately!) nobody reviews my contributions, and therefore I never submit my contributions as PRs. Some patches are trivial, but many patches are so hard to develop, I amend them dozens of times before they are good enough, and sometimes that takes weeks or months.

Never give up!

So before I go and change the implementation here my suggestion. The challenge: We need a on minutes chopped (not rounded) time representation with epoch 0am of the takeoff day. The options:

1. Closest implementation is RoughTime: Remove the <24h restriction from it, use it.

I already explained two reasons why I believe this is a bad solution.

2. 2nd closest is TimeStamp: Add the ChopSeconds method and use it.

What does this method do, and how does it solve the problem?

3. Not use any  encapsulation and put the few lines into the module  TimesStatusPanel.cpp in need.

If by "few lines" you mean have redundant code - then no.

4. Create a new implementation "RoughStamp" and have the required implementation there.
   There would certainly numerous more ways to go, pls advise .. no trial and error!

Why do you believe that having some Rough class is a good idea? I don't think it's a good idea, because it needs 2 conversions where 0 are necessary. That complicates the code, bloats it and makes it slow.

I suggest:

5. Do nothing, because this is a non-issue, and quite contrary, this PR creates more problems than it fixes, adds overhead, adds code complexity, reduces precision, justified only by pretending to make something appear more accurate while making it actually less accurate. (see flight times rounding error #957 (comment))