This function should have the additional checks that the detailed timer is doing, such as the specialized logic for the best segments comparison and so on. The goal should be to remove the function from the detailed timer and use this one instead. Then it should be good to merge.

@wooferzfg Looks like in the splits we use "combined comparison segment time", in the detailed timer we use "single comparison segment time" and for the segment time component it could potentially be either. What do you think we should do? Do the same thing for all of them? (the difference being that skipped splits get combined together, while the detailed timer just shows a - then).

My initial reasoning for using the combined segment time in the splits vs. a single segment time in the detailed timer is that you can see all of the segments in the splits, which means you know which segment the combined segment time started at.

In the detailed timer, you can only see the current segment, so we never show combined segments. There's also the issue of the segment timer starting at a different segment than the comparison segment time if we start combining stuff. For example, you might get a case where the segment timer is at 5 minutes, the PB comparison segment time is 40 minutes (because it's combined), and the second comparison segment time is 2 hours (because it's also combined, but starting from an earlier segment). That makes it look like you can save a lot of time, but in reality that's not necessarily true.

However, we don't really apply this logic everywhere else, so it's not totally clear what we should do here. The previous segment component uses combined segments, for example. In my opinion, it would be best to use the combined comparison segment time here because there's only one piece of information being displayed.

Yeah I think for the splits it makes sense to keep it showing the combined segments, as you can see that the ones before are skipped. For the detailed timer you don't, so it makes sense to dash it out. But I'm not quite following your argument for the segment time component, as you seem to argue the opposite there?

combined comparison segment time here because there's only one piece of information being displayed.

For the segment time component, we only show one value, so there's no issue with conflicting information within the component (unlike the detailed timer, where we show a segment timer and 2 different comparisons). I don't feel too strongly about this though.

I don't have many strong feelings here either, but if I was forced to choose I'd say just unify them for the sake of code quality and consistency.

I don't think that the current implementation of this method makes sense.

• This provides a combined segment time, which means that the detailed timer's logic will change when switching over to it.
• Using the best segment time only makes sense if we're using non-combined segment times (because, by definition, the best segment time is a non-combined segment time). That's why the detailed timer was using it previously, but the splits component was not.

Yeah, this wasn't meant to be considered finished.

I would prefer if there were two separate methods in state_helper: one for combined segment times and one for non-combined segment times. The non-combined segment time method would do the best segment time override, while the combined segment time method would not. The splits component would use the combined segment time method, and the detailed timer component would use the non-combined segment time method. The segment time component would use whichever one we think makes the most sense (which is the discussion we just had in the other thread that didn't reach a clear answer).

So if we end up splitting comparison segment time into two distinct methods and splits keep using the combined one, then there's no way for the splits to show the best segment times as a column. Are we sure that's what we want? Is there a disadvantage to just switch to the best segment times despite us usually showing combined segment times? The problem is that just like best split times and co. we can't really derive any segment times from it, as the times aren't truly building on top of each other.

Personally I'd prefer just having a single method and explicitly handling empty segments in the detailed timer itself if the resulting code is cleaner than having 2 different methods with almost identical code.

Eh, I guess it's fine to use the best segment times then, even if it's a little inconsistent.

Also, I agree on having a single method if you're able to cleanly keep the current logic of the detailed timer while using this method.

Looks like we might actually have to have 2 different functions.

The cleanest way I can think of handling things in the detailed timer is to use a wrapper function and at that point we might as well just have 2 functions.

Update: In the end I went with the solution of having 2 different functions. I couldn't find a way to cleanly keep the current behavior of the detailed timer without creating a wrapper function (Mostly because my rust skills are quite lacking)