ColumnType: CustomVariable#27
Conversation
6597da7 to
f45c480
Compare
| { | ||
| if (column.Type is ColumnType.CustomVariable) | ||
| { | ||
| CustomVariableValues[state.CurrentSplitIndex][column.Name] = state.Run.Metadata.CustomVariableValue(column.Name) ?? TimeFormatConstants.DASH; |
There was a problem hiding this comment.
Shouldn't this be happening in the LiveSplit code and stored in the Run, instead of in this component? Otherwise you won't get any variable values if you switch layouts mid-run
There was a problem hiding this comment.
That didn't seem necessary to me since these variable values don't need to be saved in the .lss splits file, just temporarily displayed as the run is going. Persisting across layout changes mid-run isn't something needed for any of the use-cases I was considering
There was a problem hiding this comment.
Everything else related to a run is persisted across layout changes
There was a problem hiding this comment.
I have updated this PR to use LiveSplit/LiveSplit#2600 to store Custom Variable values in each Segment of the run, rather than in the layout component.
| else if (type is ColumnType.CustomVariable) | ||
| { | ||
| // if the split was skipped, wipe the custom variable column entry for the split | ||
| if (Split.SplitTime[timingMethod] == null) |
There was a problem hiding this comment.
How will these custom variables be set? It's a bit surprising to me that you have this check here, rather than this being done by whatever process is setting the custom variables (related to LiveSplit/LiveSplit#2600)
There was a problem hiding this comment.
Is there a reason you're allowing custom variable values to be set for skipped splits and future splits, if they're never displayed?
There was a problem hiding this comment.
In LiveSplit/LiveSplit#2600, the custom variable values are cleared for a skipped segment, here:
https://github.com/LiveSplit/LiveSplit/pull/2600/files#diff-5702f4e00c13058babd747d35ff6fa73fff0a5a774a55eb542ad076c29bdf4f9R89
public void SkipSplit()
{
if ((CurrentState.CurrentPhase == TimerPhase.Running
|| CurrentState.CurrentPhase == TimerPhase.Paused)
&& CurrentState.CurrentSplitIndex < CurrentState.Run.Count - 1)
{
CurrentState.CurrentSplit.SplitTime = default;
CurrentState.CurrentSplit.CustomVariableValues.Clear();
CurrentState.CurrentSplitIndex++;
CurrentState.Run.HasChanged = true;
OnSkipSplit?.Invoke(this, null);
}
}This Split.SplitTime[timingMethod] == null is a holdover from before then. I'll remove it after I find time to test that its behavior still matches LiveSplit One after removal.
There was a problem hiding this comment.
I've cleaned up this holdover code and tested it to still be the same as LiveSplit One.
However, at the same time I also tested whether switching between a layout containing Splits vs a layout containing Subsplits, and it did not properly preserve past segment custom variable values.
There was a problem hiding this comment.
However, at the same time I also tested whether switching between a layout containing Splits vs a layout containing Subsplits, and it did not properly preserve past segment custom variable values.
That's not good; do you know why that's the case?
There was a problem hiding this comment.
Not yet, but I'm looking into it
|
Update Subsplits as well? |
|
I figured out why it was looking like it didn't properly preserve past segment custom variable values when switching layouts. Turns out it was preserving them, but I couldn't see them because the text color was black! The text color gets set back to white when scrolling. This video shows what it looks like, at 4:45: |
|
After |
|
|
||
| public IEnumerable<ColumnData> ColumnsList { get; set; } | ||
| public IList<SimpleLabel> LabelsList { get; set; } | ||
| protected List<float> ColumnWidths { get; } |
There was a problem hiding this comment.
To store the list of column widths that comes from SplitsComponent, and is meant to be a shared mutable object: when a column gets larger in one SplitComponent, it should get larger on all SplitComponents. Right now it looks a bit janky to me... there's probably a better way to do this
There was a problem hiding this comment.
Is this fixing a bug that existed before? Or is it fixing an issue that was introduced with custom variable columns?
There was a problem hiding this comment.
To my knowledge, it isn't fixing a bug that existed before. Unless it was possible to make split times or deltas be so big that they overrun the set column widths for those based on MeasureDeltaLabel.ActualWidth... but I'm not aware of such an issue before Custom Variables.
Custom Variables can be much longer than split times or deltas, so this is a new issue with those.
The "jank" I'm referring to is a slight delay from when the variable gets longer to when the next column over shifts to accommodate it. For that short period of time, there can be some overrun.
Also, sometimes it updates the current split first, and past splits a slight bit later. For that moment, the next column over looks crooked.
There was a problem hiding this comment.
The "jank" I'm referring to is a slight delay from when the variable gets longer to when the next column over shifts to accommodate it. For that short period of time, there can be some overrun.
Ideally, the custom variable text would temporarily be cut off instead of the columns not being aligned. And all the columns would shift at the same time
There was a problem hiding this comment.
What if a custom variable gets longer and then shorter again? Do the columns shrink?
There was a problem hiding this comment.
The columns do not currently shrink. I can implement that if you want, I just hadn't thought of it because in my use-cases, the values wouldn't shrink so the columns would never need to.
There was a problem hiding this comment.
What if you have a large variable value and then undo a split? Doesn't that cause the issue even with your use case?
There was a problem hiding this comment.
After SplitsComponent: CalculateColumnWidths, it can shrink the columns when the values shrink. It is also less precise, not accounting for characters with different widths, such as Fullwidth Unicode CJK characters.
I was hoping it would also get rid of the jank, but some of the jank I referred to above is still there.
|
I was hoping |
This reverts commit 2e1032b.
|
With both |
A companion PR to LiveSplit/LiveSplit#2528. Allows a column in the splits component to display the value of a Custom Variable, with Column Type choice option.
Depends on LiveSplit/LiveSplit#2600.
Tasks:
wipe custom variable column for skipped splits, when a split is skipped, both wipe the variable entries for that split