Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Application is always redrawing and update cursor even when not needed. #3813

Closed
BDisp opened this issue Nov 12, 2024 · 37 comments
Closed

Application is always redrawing and update cursor even when not needed. #3813

BDisp opened this issue Nov 12, 2024 · 37 comments
Assignees
Labels
bug v2 For discussions, issues, etc... relavant for v2

Comments

@BDisp
Copy link
Collaborator

BDisp commented Nov 12, 2024

Only by only moving the mouse without affect any view to be needs to redraw, the Application is always redrawing and updating the cursor without no need, wasting CPU resources. I found this behavior with the Editor scenario which happens with all drivers.

@BDisp BDisp added bug v2 For discussions, issues, etc... relavant for v2 labels Nov 12, 2024
@BDisp BDisp self-assigned this Nov 12, 2024
BDisp added a commit to BDisp/Terminal.Gui that referenced this issue Nov 12, 2024
@tig
Copy link
Collaborator

tig commented Nov 12, 2024

This bug report is too vague.

I agree for Cursor the current code is sub-optimal. That will be addressed in

However, for drawing, the current code is quite frugal.

RunIteration does this

        LayoutAndDraw ();

        if (PositionCursor ())
        {
            Driver!.UpdateCursor ();
        }

LayoutAndDraw() does this:

    public static void LayoutAndDraw (bool forceDraw = false)
    {
        bool neededLayout = View.Layout (TopLevels.Reverse (), Screen.Size);

        if (forceDraw)
        {
            Driver?.ClearContents ();
        }

        View.SetClipToScreen ();
        View.Draw (TopLevels, neededLayout || forceDraw);
        View.SetClipToScreen ();

        Driver?.Refresh ();
    }

From this point, only views that have DrawNeeded == true get their Draw method called.

Any call to AddRune that happens gets short-circuited by the clip region if the location is not in the clip. Thus the driver never sees it.

You can see this visually by turning on ViewDiagnostics.DrawIndicator:

oXSMeGr 1

So, what specifically are you referring to when you say "Application is always redrawing"?

@BDisp
Copy link
Collaborator Author

BDisp commented Nov 12, 2024

So, what specifically are you referring to when you say "Application is always redrawing"?

The better scenario to test that I'm right with this is the Editor scenario. It's always redrawing because NeedsLayout is always true.

@BDisp
Copy link
Collaborator Author

BDisp commented Nov 12, 2024

For testing this issue don't use scenario that use timers, because of course they will be drawing. Test only with scenarios that does not change anything.

@tig
Copy link
Collaborator

tig commented Nov 12, 2024

I added a border to _textView so DrawIndicator could be seen:

dJRdGfh 1

What am I not seeing?

@BDisp
Copy link
Collaborator Author

BDisp commented Nov 12, 2024

Do you remember that before existed two methods, one that clear the needs display and the other that clear the needs layout? Currently only exist for clear needs draw but not for clear needs layout. I really think that it's needed only one method for both.

@BDisp
Copy link
Collaborator Author

BDisp commented Nov 12, 2024

What am I not seeing?

I think you didn't understanding the reason for this issue. The issue is only related when you are only moving the mouse without click or anything. In the WindowsDriver even without moving the mouse it's always drawing.

@tig
Copy link
Collaborator

tig commented Nov 12, 2024

Do you remember that before existed two methods, one that clear the needs display and the other that clear the needs layout? Currently only exist for clear needs draw but not for clear needs layout. I really think that it's needed only one method for both.

Please study the fact that I have very carefully decoupled drawing from layout. It is important for correctness that they are decoupled. I am not happy with the fact that NeedsDraw is always true if NeedsLayout is true. As noted by the BUGBUG in the code. But that is the ONLY coupling (unless I missed something) and I want to keep it that way.

@BDisp
Copy link
Collaborator Author

BDisp commented Nov 12, 2024

In the Editor scenario put a breakpoint as showing in the screenshot and you'll see.

image

@BDisp
Copy link
Collaborator Author

BDisp commented Nov 12, 2024

Please study the fact that I have very carefully decoupled drawing from layout. It is important for correctness that they are decoupled. I am not happy with the fact that NeedsDraw is always true if NeedsLayout is true. As noted by the BUGBUG in the code. But that is the ONLY coupling (unless I missed something) and I want to keep it that way.

I really don't understand the problem with my simple and logic fix. Can you please explain to me where I'm wrong?

@BDisp
Copy link
Collaborator Author

BDisp commented Nov 12, 2024

I agree that when NeedsLayout is true make very sense to also considering the NeedsDraw also as true. What does not make sense is the opposite. If a view was resized of course it'll needs to redraw again. If NeedsDraw is true does not mean that it needs to layout at all.

@BDisp
Copy link
Collaborator Author

BDisp commented Nov 12, 2024

I added a border to _textView so DrawIndicator could be seen:

I didn't said to write something nor for select text 😃

@tig
Copy link
Collaborator

tig commented Nov 12, 2024

Please study the fact that I have very carefully decoupled drawing from layout. It is important for correctness that they are decoupled. I am not happy with the fact that NeedsDraw is always true if NeedsLayout is true. As noted by the BUGBUG in the code. But that is the ONLY coupling (unless I missed something) and I want to keep it that way.

I really don't understand the problem with my simple and logic fix. Can you please explain to me where I'm wrong?

I don't see a bug that needs fixing. I set this breakpoint and it only breaks when I actually click in Editor:

image

Your fix (that I don't think we need) is adding complexity to a system that I worked really hard to make as simple as possible.

This couples layout and draw:

 protected void ClearNeedsDrawAndNeedsLayout ()
    {
        _needsDrawRect = Rectangle.Empty;
        SubViewNeedsDraw = false;
        NeedsLayout = false;

@BDisp
Copy link
Collaborator Author

BDisp commented Nov 12, 2024

I added a border to _textView so DrawIndicator could be seen:

I also didn't said to add a DrawIndicator into the TextView, which of course it'll always draw.

@BDisp
Copy link
Collaborator Author

BDisp commented Nov 12, 2024

Do you're using WindowsDriver to test?

@BDisp BDisp changed the title Application is always redrawing and update cursor even when not needed. WindowsDriver is always redrawing and update cursor even when not needed. Nov 12, 2024
@BDisp
Copy link
Collaborator Author

BDisp commented Nov 12, 2024

This couples layout and draw:

That why I changed the method name to ClearNeedsDrawAndNeedsLayout. Doesn't make sense to set NeedsDraw to false without also set the NeedsLayout to false.

@BDisp
Copy link
Collaborator Author

BDisp commented Nov 12, 2024

Another detail, see in the unit tests that I changed where you commented on justifications for doubts, like I'm not really sure if I'm right.

@tig
Copy link
Collaborator

tig commented Nov 12, 2024

This couples layout and draw:

That why I changed the method name to ClearNeedsDrawAndNeedsLayout. Doesn't make sense to set NeedsDraw to false without also set the NeedsLayout to false.

Having one function that does both Draw and Layout is an example of coupling. If there is a bug here (I'm open to that), the right fix would have two functions.

@BDisp
Copy link
Collaborator Author

BDisp commented Nov 12, 2024

Having one function that does both Draw and Layout is an example of coupling. If there is a bug here (I'm open to that), the right fix would have two functions.

But who couple it wasn't me, but I don't think it isn't a good solution having them coupled. Avoids duplicates code doing similar things for different properties.

@tig
Copy link
Collaborator

tig commented Nov 12, 2024

Let's step back and make sure there's really a bug.

Can you author a simple, focused, unit test that demonstrates the bug?

@BDisp
Copy link
Collaborator Author

BDisp commented Nov 12, 2024

Let's step back and make sure there's really a bug.

Can you author a simple, focused, unit test that demonstrates the bug?

The changes I made in the unit tests already justify them. But please answer one question that you didn't before. Does this behavior happens or nor by using WindowsDriver? If not then we have different Windows 11 components.

@BDisp BDisp changed the title WindowsDriver is always redrawing and update cursor even when not needed. Application is always redrawing and update cursor even when not needed. Nov 12, 2024
@tznind
Copy link
Collaborator

tznind commented Nov 12, 2024

I think we should have a global 'frame rate' enforced for iteration. At the moment a lot of issues stem from iteration running too fast e.g. see #3812

To me a MainLoop should look something like (pseudocode)

do
{
     var dt = DateTime.Now();
    
    RunIteration();
    
    var took = DateTime.Now() - dt
    var sleepFor = TimeSpan.FromMilliseconds(200) - took;
    
    if(sleepFor > 0)
        Task.Delay(sleepFor).Wait();

}
while(true)

This solves several problems including the kinda sketchy way that 'has events' somehow has to take responsibility for blocking until there are events - except when there are timers (that being a problem in itself) etc.

The iteration, drawing, event queue inspection etc should all happen as fast as possible. Then we just sleep for however long we want to enforce our frame/loop rate.

Thoughts?

See for example this thread:

Did you realized that the EventsPending method is always running overkilling the CPU usage with 13%, approximately? That why the Threading scenario it's working on Linux and Windows. It's needed to wait until it's necessary.

#3791 (comment)

@BDisp
Copy link
Collaborator Author

BDisp commented Nov 12, 2024

@tig it's really affecting all drivers with the Editor scenario. Do you know if there is some view that need to always be draw in this scenario?
Put this when you are in the Editor:

Captura de ecrã 2024-11-12 190132

and you'll see this:

Captura de ecrã 2024-11-12 190226

So, as you can see the neededLayout variable is always true even without doing nothing. If it's any view setting as true because it's needed, that ok, but if it's the NeedsLayout property that does set to false, then this PR fix this.

@BDisp
Copy link
Collaborator Author

BDisp commented Nov 12, 2024

From this point, only views that have DrawNeeded == true get their Draw method called.

I think you meant NeedsDraw but you already know that when NeedsLayout is true then NeedsDraw will be set to true, which is the correct behavior.

@BDisp
Copy link
Collaborator Author

BDisp commented Nov 12, 2024

Let's step back and make sure there's really a bug.

Can you author a simple, focused, unit test that demonstrates the bug?

See the changed existed ones here 7761bb0

@tig
Copy link
Collaborator

tig commented Nov 12, 2024

Let's step back and make sure there's really a bug.
Can you author a simple, focused, unit test that demonstrates the bug?

The changes I made in the unit tests already justify them. But please answer one question that you didn't before. Does this behavior happens or nor by using WindowsDriver? If not then we have different Windows 11 components.

Yes. WIndows 11/WindowsDriver.

image

@tig
Copy link
Collaborator

tig commented Nov 12, 2024

I think we should have a global 'frame rate' enforced for iteration. At the moment a lot of issues stem from iteration running too fast e.g. see #3812

To me a MainLoop should look something like (pseudocode)

do
{
     var dt = DateTime.Now();
    
    RunIteration();
    
    var took = DateTime.Now() - dt
    var sleepFor = TimeSpan.FromMilliseconds(200) - took;
    
    if(sleepFor > 0)
        Task.Delay(sleepFor).Wait();

}
while(true)

This solves several problems including the kinda sketchy way that 'has events' somehow has to take responsibility for blocking until there are events - except when there are timers (that being a problem in itself) etc.

The iteration, drawing, event queue inspection etc should all happen as fast as possible. Then we just sleep for however long we want to enforce our frame/loop rate.

Thoughts?

Great idea. But, to be clear, this is a last-defense. We need to ensure correctness as much as possible first.

@tig
Copy link
Collaborator

tig commented Nov 12, 2024

Let's step back and make sure there's really a bug.
Can you author a simple, focused, unit test that demonstrates the bug?

See the changed existed ones here 7761bb0

Why are you so opposed to writing primitive unit tests that show a specific bug? The code you pointed to is a test for many several things and is just confusing.

I am not seeing the behavior you are seeing (or I'm not configuring things right). It should be easy for you to write a ~10 line unit test that shows this bug.

@BDisp
Copy link
Collaborator Author

BDisp commented Nov 12, 2024

Yes. WIndows 11/WindowsDriver.

Thanks for the information. The behavior as I tell before happens with all drivers in the Editor scenario. So, some view is setting NeedsLayout to true and since the View class isn't setting it again to false after the Draw process is finished is causing always the redrawing behavior. But I already realized that you don't want to fix this anyway. Do you want I close this?

@BDisp
Copy link
Collaborator Author

BDisp commented Nov 12, 2024

Why are you so opposed to writing primitive unit tests that show a specific bug? The code you pointed to is a test for many several things and is just confusing.

I am not seeing the behavior you are seeing (or I'm not configuring things right). It should be easy for you to write a ~10 line unit test that shows this bug.

Damn, I will but I think this isn't difficult to understand.

@tig
Copy link
Collaborator

tig commented Nov 12, 2024

Did you modify Editor at all? Are there any views with a Margin that's ShadowStyle.Transparent?

image

@BDisp
Copy link
Collaborator Author

BDisp commented Nov 12, 2024

Did you modify Editor at all? Are there any views with a Margin that's ShadowStyle.Transparent?

No, I didn't but I think there is some view that there is calling SetNeedsLayout maybe on DrawComplete after the ClearNeedsDraw. I think this is a possibility. Do you remember what view can be doing this?

@BDisp
Copy link
Collaborator Author

BDisp commented Nov 12, 2024

See this. After call LayoutAndDraw there are this views with NeedsLayout as true.

image

@tig
Copy link
Collaborator

tig commented Nov 12, 2024

Hide StatusBar. Likely culprit as ShortCut uses Margin for spacing.

Have you tried adding a border to _textView and turning on the Draw Indicator? Can you share a video of that on your machine?

@tig
Copy link
Collaborator

tig commented Nov 12, 2024

Found it!

I had forgotten I commented out all the ScrollBar code in Editor in my branch. I just switched to v2_develop and I see this:

kViivl1 1

THIS is precisely why I have spent the last 3 days rewriting the abomination we call ScrollBarView ;-).

Closing this bug as WON'T FIX as the bug is actually in ScrollBarView and will be fixed in

@BDisp
Copy link
Collaborator Author

BDisp commented Nov 12, 2024

Yes it's really the ScrollBarView. Happy you found it. That's really great. Thanks.

@BDisp
Copy link
Collaborator Author

BDisp commented Nov 12, 2024

Hide StatusBar. Likely culprit as ShortCut uses Margin for spacing.

Have you tried adding a border to _textView and turning on the Draw Indicator? Can you share a video of that on your machine?

I add it in the v2_develop branch.

WindowsTerminal_R9NmsVAveC

@BDisp
Copy link
Collaborator Author

BDisp commented Nov 12, 2024

Anyway if you want this unit test that pass in the v2_develop.

    [Fact]
    [AutoInitShutdown]
    public void Draw_Sets_NeedsDraw_And_NeedsLayout_False_When_Finished ()
    {
        var top = new Toplevel ();
        var view = new View { Width = Dim.Fill (), Height = Dim.Fill (), CanFocus = true };
        top.Add (view);

        Assert.True (top.NeedsLayout);
        Assert.True (top.NeedsDraw);
        Assert.True (view.NeedsLayout);
        Assert.True (view.NeedsDraw);

        var rs = Application.Begin (top);
        Application.RunIteration (ref rs);

        Assert.False (top.NeedsLayout);
        Assert.False (top.NeedsDraw);
        Assert.False (view.NeedsLayout);
        Assert.False (view.NeedsDraw);

        view.SetNeedsLayout ();
        Assert.True (top.NeedsLayout);
        Assert.True (top.NeedsDraw);
        Assert.True (view.NeedsLayout);
        Assert.True (view.NeedsDraw);

        Application.RunIteration (ref rs);

        Assert.False (top.NeedsLayout);
        Assert.False (top.NeedsDraw);
        Assert.False (view.NeedsLayout);
        Assert.False (view.NeedsDraw);

        Application.End (rs);
        top.Dispose ();
    }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug v2 For discussions, issues, etc... relavant for v2
Projects
None yet
3 participants