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

Fix for Windows SearchBar MaxLength > 0 not working properly. #24919

Merged
merged 32 commits into from
Oct 8, 2024

Conversation

BagavathiPerumal
Copy link
Contributor

@BagavathiPerumal BagavathiPerumal commented Sep 25, 2024

Root cause

WinUI AutoSuggestbox does not have MaxLength property in native, SearchBar.MaxLength is not updated to native property.

Description of Issue Fix

Modified SearchBar by taking the AutoSuggestbox child, a textbox control in UpdateMaxLength() and updated SearchBar.MaxLength to native textbox.MaxLength.

Tested the behavior in the following platforms.

  • Android
  • Windows
  • iOS
  • Mac

Issues Fixed

Fixes #5669

Output

Before Issue Fix After Issue Fix
5669_BeforeChange.mp4
5669_AfterChange.mp4

@dotnet-policy-service dotnet-policy-service bot added the community ✨ Community Contribution label Sep 25, 2024
@BagavathiPerumal BagavathiPerumal changed the title fix-5669-Fix for Windows SearchBar MaxLength > 0 not working properly. Fix for Windows SearchBar MaxLength > 0 not working properly. Sep 25, 2024
Comment on lines 116 to 117
if (textbox != null)
textbox!.MaxLength = searchBar.MaxLength;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (textbox != null)
textbox!.MaxLength = searchBar.MaxLength;
if (textbox is not null)
textbox.MaxLength = searchBar.MaxLength;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MartyIX, I have modified changes. Could you please validate it once and let me know if there are any further concerns.

Comment on lines 115 to 124
var child = platformControl.GetChildren<TextBox>();
if (child is not null && child.Count() > 0)
{
child.FirstOrDefault()!.MaxLength = searchBar.MaxLength;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps

Suggested change
var child = platformControl.GetChildren<TextBox>();
if (child is not null && child.Count() > 0)
{
child.FirstOrDefault()!.MaxLength = searchBar.MaxLength;
}
var children = platformControl.GetChildren<TextBox>();
var child = children?.FirstOrDefault();
if (child is not null)
{
child.MaxLength = searchBar.MaxLength;
}

-or-

Suggested change
var child = platformControl.GetChildren<TextBox>();
if (child is not null && child.Count() > 0)
{
child.FirstOrDefault()!.MaxLength = searchBar.MaxLength;
}
var child = platformControl.GetChildren<TextBox>()?.FirstOrDefault();
if (child is not null)
{
child.MaxLength = searchBar.MaxLength;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MartyIX, I have modified changes based on your suggestion. Could you please validate it once and let me know if there are any further concerns.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please see #24919 (comment). Other than that, it looks OK to me.

@@ -112,6 +112,17 @@ public static void UpdateMaxLength(this AutoSuggestBox platformControl, ISearchB
if (maxLength == -1)
maxLength = int.MaxValue;

var child = platformControl.GetChildren<TextBox>();
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rename this to children

@BagavathiPerumal BagavathiPerumal marked this pull request as ready for review September 26, 2024 10:29
@BagavathiPerumal BagavathiPerumal requested a review from a team as a code owner September 26, 2024 10:29
@jfversluis
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

Copy link
Member

@rmarinho rmarinho left a comment

Choose a reason for hiding this comment

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

I think it's missing some using the builds is failing

D:\a\1\s\src\Core\src\Platform\Windows\SearchBarExtensions.cs(118,28): error CS1061: 'IEnumerable<TextBox?>' does not contain a definition for 'FirstOrDefault' and no accessible extension method 'FirstOrDefault' accepting a first argument of type 'IEnumerable<TextBox?>' could be found (are you missing a using directive or an assembly reference?) [D:\a\1\s\src\Core\src\Core.csproj::TargetFramework=net8.0-windows10.0.20348.0]

@BagavathiPerumal
Copy link
Contributor Author

I think it's missing some using the builds is failing

D:\a\1\s\src\Core\src\Platform\Windows\SearchBarExtensions.cs(118,28): error CS1061: 'IEnumerable<TextBox?>' does not contain a definition for 'FirstOrDefault' and no accessible extension method 'FirstOrDefault' accepting a first argument of type 'IEnumerable<TextBox?>' could be found (are you missing a using directive or an assembly reference?) [D:\a\1\s\src\Core\src\Core.csproj::TargetFramework=net8.0-windows10.0.20348.0]

@rmarinho, I have modified changes for this issue. Could you please validate it once and let me know if there are any further concerns.

@rmarinho
Copy link
Member

Can we also rebase on main? we made some changes on the pipeline agents and we need the pr to pick them up.

Thanks

@rmarinho
Copy link
Member

/rebase

@BagavathiPerumal
Copy link
Contributor Author

SearchBarMaxLength failing

@PureWeen, I have updated test image for this issue. Could you please validate it once and let me know if there are any further concerns.

@rmarinho
Copy link
Member

rmarinho commented Oct 3, 2024

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

Foda
Foda previously approved these changes Oct 3, 2024
Copy link
Member

@Foda Foda left a comment

Choose a reason for hiding this comment

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

LGTM, just please address LINQ comment

@BagavathiPerumal
Copy link
Contributor Author

LGTM, just please address LINQ comment

@Foda, I have updated the code based on your suggestion. Could you please review it and let me know if you have any further concerns.

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@jfversluis jfversluis requested review from PureWeen and Foda October 4, 2024 08:21
PureWeen
PureWeen previously approved these changes Oct 4, 2024
@PureWeen
Copy link
Member

PureWeen commented Oct 4, 2024

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@jsuarezruiz
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@PureWeen PureWeen dismissed rmarinho’s stale review October 8, 2024 16:54

Comments addressed

@PureWeen PureWeen merged commit 7f56f6c into dotnet:main Oct 8, 2024
97 checks passed
@samhouts samhouts added the fixed-in-net8.0-nightly This may be available in a nightly release! label Oct 14, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Nov 16, 2024
@sheiksyedm sheiksyedm added the partner/syncfusion Issues / PR's with Syncfusion collaboration label Dec 6, 2024
@samhouts samhouts added fixed-in-9.0.10 and removed fixed-in-net8.0-nightly This may be available in a nightly release! labels Dec 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
community ✨ Community Contribution fixed-in-9.0.10 partner/syncfusion Issues / PR's with Syncfusion collaboration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Windows SearchBar MaxLength > 0 not working properly
9 participants