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

[TextBox] Add readonly property: LineCount. #17656

Merged
merged 2 commits into from
Jan 22, 2025
Merged

Conversation

rabbitism
Copy link
Contributor

What does the pull request do?

Port WPF property to Avalonia
https://github.com/dotnet/wpf/blob/a08bc2f79bb45e46257368363b4bff594a0c848c/src/Microsoft.DotNet.Wpf/src/PresentationFramework/System/Windows/Controls/TextBox.cs#L899-L920

What is the current behavior?

What is the updated/expected behavior with this PR?

How was the solution implemented (if it's not obvious)?

Some notes about test cases:
The test cases are what I observed from actual running app's behavior.

Checklist

Breaking changes

Obsoletions / Deprecations

Fixed issues

@avaloniaui-bot
Copy link

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

@maxkatz6
Copy link
Member

maxkatz6 commented Dec 1, 2024

Not sure how impactful on perf it would be, but LineCount can be a DirectProperty, supporting change notifications.

@rabbitism
Copy link
Contributor Author

Not sure how impactful on perf it would be, but LineCount can be a DirectProperty, supporting change notifications.

TextLayout.TextLines cannot be subscribed, thus we have to report change for every Text change and Bounds change. It can be expensive.

@MrJul
Copy link
Member

MrJul commented Dec 15, 2024

I'm really surprised that WPF exposes a non-observable property here.
I can already see the bug reports coming in from bindings not working.

If we can't bind it, maybe it should be a method instead?

@rabbitism
Copy link
Contributor Author

If we can't bind it, maybe it should be a method instead?

I don't have a preference, please let me know if team get a final decision.

@MrJul MrJul added the needs-api-review The PR adds new public APIs that should be reviewed. label Dec 17, 2024
@rabbitism
Copy link
Contributor Author

Any updates on this?

@MrJul
Copy link
Member

MrJul commented Jan 14, 2025

Diff for API review:

 namespace Avalonia.Controls
 {
     public class TextBox
     {
+        public int LineCount { get; }
     }
 }

@MrJul
Copy link
Member

MrJul commented Jan 14, 2025

Any updates on this?

Sorry for the delay, we'll have an API review soon.

Copy link
Member

@MrJul MrJul left a comment

Choose a reason for hiding this comment

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

Notes from the API review meeting:

Use a method instead.

Expected API:

public class TextBox
{
    public int GetLineCount();
}

@MrJul MrJul added api-change-requested The new public APIs need some changes. and removed needs-api-review The PR adds new public APIs that should be reviewed. labels Jan 21, 2025
@avaloniaui-bot
Copy link

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

Copy link
Member

@MrJul MrJul left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@MrJul MrJul added this pull request to the merge queue Jan 22, 2025
Merged via the queue into AvaloniaUI:master with commit 26bc229 Jan 22, 2025
10 checks passed
@MrJul MrJul added api-approved The new public APIs have been approved. and removed api-change-requested The new public APIs need some changes. labels Jan 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-approved The new public APIs have been approved. feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants