Fix layouting bug in SS14 ghost role menu (space-wizards/space-station-14#41434)

[Fruitsalad]

Fixes space-wizards/space-station-14#41434.

While debugging I also came across another UI bug where descriptions would be hidden behind buttons. This change seems to fix that bug as well.

The short version

A dirty flag problem caused a layouting problem in the SS14 ghost role menu sometimes. This happens because under very specific circumstances a ScrollContainer might get queued for measuring but not for subsequent rearranging. This can be fixed by adding an InvalidateArrange() call into FrameUpdate() so that all Controls that are remeasured will also get rearranged.

The *very very long* version

I'd written down the what & why of this solution while debugging so I figured I might as well share it.

Timeline of the bug

What's special about the ghost role menu is that it receives its list entries from a network request. The bug only occurred when the network response was received one frame after creating the UI window, and even then it was an inconsistent bug.

Frame 1

• Dispatch network request for list of ghost roles
• Create ghost role window
• In UserInterfaceManager.FrameUpdate, rearrange the ghost role menu ScrollContainer:
  • ScrollContainer.set_Visible() gets called
  • That calls ScrollContainer.InvalidateMeasure()
  • Set IsMeasureValid = false & add it to the measure queue
  • Call ScrollContainer.InvalidateArrange()
  • Early-out because we're currently rearranging ScrollContainer
  • IsArrangeValid does not get set to false and it is not added to the arrange queue (here's your problem)
• The ghost role window is displayed with an empty list placeholder message

Between frame 1 & frame 2

• ScrollContainer is at the front of the measure queue
• ScrollContainer is not in the rearrange queue
• ScrollContainer.IsMeasureValid = false
• ScrollContainer.IsArrangeValid = true

Frame 2

• Receive list of ghost roles from the server
• Add entries to the ghost role list
• In UserInterfaceManager.FrameUpdate:
  • ScrollContainer is measured first (it's at the front of the queue)
  • This involves measuring EntryContainer
  • EntryContainer calls Parent?.InvalidateMeasure(), but this earlies-out because its parent ScrollContainer is currently measuring
  • As a result, ScrollContainer is never rearranged
  • Therefore ScrollContainer never gets a chance to notice that it should make the scrollbar visible

This leads to the buggy UI seen in the bug report.

Solutions

Solution 1 (dead end)

public void InvalidateMeasure()
{
    if (!IsMeasureValid || _measuring || _arranging)
        return;

    IsMeasureValid = false;
    UserInterfaceManagerInternal.QueueMeasureUpdate(this);
    InvalidateArrange();
}

Upside: Solves the immediate problem.

Downside: With this solution, in the ghost list window, the scrollbar is always drawn on top of EntryContainer until you resize the window. I think it's because EntryContainer doesn't get a chance to remeasure/rearrange. It's a pretty subtle bug.

Solution 2 (dead end)

public void InvalidateMeasure()
{
    if (!IsMeasureValid || _measuring)
        return;

    IsMeasureValid = false;
    UserInterfaceManagerInternal.QueueMeasureUpdate(this);

    if (IsArrangeValid)
    {
        IsArrangeValid = false;
        UserInterfaceManagerInternal.QueueArrangeUpdate(this);
    }
}

Ensure IsArrangeValid is always false when IsMeasureValid is false.

Upside: It works pretty well for the ghost list menu

Downside: You can get an infinite loop if an Arrange() implementation calls InvalidateMeasure() on this either directly or indirectly. It would keep adding itself to the arrange queue over and over. Didn't happen during my testing but hypothetically it could.

Solution 3

Call InvalidateArrange() in FrameUpdate() so that each Control that is measured in a frame will also be rearranged in the same frame.

Upside: Works well.

Downside: A Control could keep updating itself every frame over and over if an Arrange() implementation calls InvalidateMeasure() on this either directly or indirectly. This is limited to one update per frame, so unlike solution 2 this would likely not be something you would notice in practice.

[Fruitsalad]

Gonna look at the tests tomorrow (whoops)

[lzk228]

it would be better to keep the standard pr template and i believe there's no need to explain that much things in the pr description

[Fruitsalad]

I didn't see a template on this repo but I can use the SS14 template for the engine repo in the future. You're right that the explanation is overly long. I've added some <details> tags to hide the sections that aren't necessarily interesting.

[lzk228]

I didn't see a template on this repo but I can use the SS14 template for the engine repo in the future.

oh that's the engine repo, didn't notice, i'm out of here

[PJB3005]

Thanks for the investigation on this issue from both you and @eoineoineoin. Sorry for not reviewing it sooner.

I do prefer this fix. Layout loops happening across frames (but only one per frame) is indeed not that bad. WPF actually limits the amount of updates that can happen per frame to completely eliminate a possibility of freeze (while it'd likely make the app ungodly slow, it should be enough for a user to save work etc)