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

Track IsEffectivelyVisible state #13972

Merged
merged 11 commits into from
Jan 23, 2024
Merged

Track IsEffectivelyVisible state #13972

merged 11 commits into from
Jan 23, 2024

Conversation

jmacato
Copy link
Member

@jmacato jmacato commented Dec 15, 2023

Previously, IsEffectivelyVisible was being calculated each time it was called. This was problematic for a couple of reasons:

  1. It traversed the tree each time it was called
  2. There is no way to track changes to IsEffectivelyVisible

This PR changes IsEffectivelyVisible to be tracked like IsEffectivelyEnabled which fixes 1). It doesn't add a way to track IsEffectivelyVisible changes yet though so doesn't address 2).

Regarding 2), making a IsEffectivelyVisible a DirectProperty may be problematic for performance reasons as the change notifications need to be fired even if there are no listeners. A CLR event may be acceptable, but the main reason we need to track changes to this is internally for the animation system (PR to follow which hooks into this) so there are currently no plans to expose an API for detecting changes to this.

@jmacato jmacato requested a review from grokys December 15, 2023 18:20
@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.1.999-cibuild0042968-beta. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

Copy link
Contributor

@jp2masa jp2masa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some method names seem to be wrong: UpdateIsEffectivelyEnabled, SyncIsEffectivelyEnabledFromParent, it should be Visible, not Enabled.

Also, I may be reading it wrong, but not sure if the logic always works, e.g. does adding this to the test work?

child2.IsVisible = false;
root.IsVisible = true;

Assert.False(child2.IsEffectivelyEnabled);

Finally, I think there's room for optimization in UpdateIsEffectivelyEnabled, so that it only needs to do the recursive call when child.IsVisible.

@jmacato
Copy link
Member Author

jmacato commented Dec 16, 2023

@jp2masa thanks for the advise!

child2.IsVisible = false;
root.IsVisible = true;
Assert.False(child2.IsEffectivelyEnabled);

Good catch on this one! I'm adding this to the test now!

Finally, I think there's room for optimization in UpdateIsEffectivelyEnabled, so that it only needs to do the recursive call when child.IsVisible.

Makes sense

@avaloniaui-bot
Copy link

You can test this PR using the following package version. 11.1.999-cibuild0043002-beta. (feed url: https://nuget-feed-all.avaloniaui.net/v3/index.json) [PRBUILDID]

@workgroupengineering
Copy link
Contributor

We could simplify the code this way by avoiding loops.

        /// <summary>
        /// Gets a value indicating whether this control and all its parents are visible.
        /// </summary>
        public bool IsEffectivelyVisible => 
            IsVisible && (VisualParent?.IsEffectivelyVisible ?? true);

@jmacato
Copy link
Member Author

jmacato commented Dec 16, 2023

@workgroupengineering we intend to provide internal change notifications when IEV changes, so we can't do what you suggested there unfortunately.

@jmacato jmacato marked this pull request as ready for review January 5, 2024 11:40
Copy link
Member

@grokys grokys left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few nits, but also this doesn't seem to handle correctly setting the IsEffectivelyVisible state when adding a grandchild. Here is a failing test:

        [Fact]
        public void Added_Grandchild_Has_Correct_IsEffectivelyVisible()
        {
            var child = new Decorator();
            var grandchild = new Decorator();
            var root = new TestRoot 
            { 
                IsVisible = false,
                Child = child
            };

            child.Child = grandchild;
            Assert.False(grandchild.IsEffectivelyVisible);
        }

src/Avalonia.Base/Visual.cs Outdated Show resolved Hide resolved
src/Avalonia.Base/Visual.cs Outdated Show resolved Hide resolved
src/Avalonia.Base/Visual.cs Outdated Show resolved Hide resolved
- Pass the parent state to `UpdateIsEffectivelyVisible`
- Remove some unneeded methods
- Update `IsEffectivelyVisible` before calling visual tree attach/detach events.
@grokys grokys changed the title Make IsEffectivelyVisible useful Track IsEffectivelyVisible state Jan 19, 2024
@grokys
Copy link
Member

grokys commented Jan 19, 2024

The VisualExtensions_GetVisualsAt.Should_Find_Control test seems to be flaky: I've seen it occur on macOS and now the Linux CI builds, but it passes locally. Not sure if it's caused by this PR or not.

@grokys grokys added this pull request to the merge queue Jan 23, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 23, 2024
@jmacato jmacato added this pull request to the merge queue Jan 23, 2024
Merged via the queue into master with commit 92e1c65 Jan 23, 2024
7 checks passed
@jmacato jmacato deleted the iseffectivelyvisible branch January 23, 2024 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants