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.

Sign up for GitHub

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

[iOS] Use same font size for Editor/Entry placeholders #13140

Merged
merged 3 commits into from
Feb 6, 2023
Merged

[iOS] Use same font size for Editor/Entry placeholders #13140

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
129a47a
[iOS] Use same font size for placeholder
rmarinho Feb 6, 2023
44b5c1a
[iOS] Update placeholder fontsize when UITextView Font is updated
rmarinho Feb 6, 2023
25e8955
Fix getting _defaultPlaceholderSize only 1 time
rmarinho Feb 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions src/Core/src/Platform/iOS/MauiTextView.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ namespace Microsoft.Maui.Platform
public class MauiTextView : UITextView
{
readonly UILabel _placeholderLabel;
nfloat _defaultPlaceholderSize = -1;

public MauiTextView()
{
Expand Down Expand Up @@ -80,6 +81,17 @@ public override string? Text
}
}

public override UIFont? Font
{
get => base.Font;
set
{
base.Font = value;
UpdatePlaceholderFontSize(value);

}
}

public override NSAttributedString AttributedText
{
get => base.AttributedText;
Expand Down Expand Up @@ -161,5 +173,21 @@ void ShouldCenterVertically()
_ => new CGPoint(0, 0),
};
}

void UpdatePlaceholderFontSize(UIFont? value)
mattleibow marked this conversation as resolved.
Show resolved Hide resolved
{
if (value != null)
Copy link
Member

Choose a reason for hiding this comment

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

is not is preferred over != because operator can be overridden leading to possible bugs.

Suggested change
if (value != null)
if (value is not null)

{
if (_defaultPlaceholderSize == -1)
{
_defaultPlaceholderSize = _placeholderLabel.Font.PointSize;
}
_placeholderLabel.Font = _placeholderLabel.Font.WithSize(value.PointSize);
}
else if (_defaultPlaceholderSize != -1)
{
_placeholderLabel.Font = _placeholderLabel.Font.WithSize(_defaultPlaceholderSize);
}
}
Comment on lines +177 to +191
Copy link
Member

@mandel-macaque mandel-macaque Feb 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I don't quite understand the logic, lets walk through it, first invert the if, that results in the following:

Suggested change
void UpdatePlaceholderFontSize(UIFont? value)
{
if (value != null)
{
if (_defaultPlaceholderSize == -1)
{
_defaultPlaceholderSize = _placeholderLabel.Font.PointSize;
}
_placeholderLabel.Font = _placeholderLabel.Font.WithSize(value.PointSize);
}
else if (_defaultPlaceholderSize != -1)
{
_placeholderLabel.Font = _placeholderLabel.Font.WithSize(_defaultPlaceholderSize);
}
}
void UpdatePlaceholderFontSize(UIFont? value)
{
if (value is null) {
if (_defaultPlaceholderSize != -1)
_placeholderLabel.Font = _placeholderLabel.Font.WithSize (_defaultPlaceholderSize);
}
else {
if (_defaultPlaceholderSize == -1) {
_defaultPlaceholderSize = _placeholderLabel.Font.PointSize;
}
_placeholderLabel.Font = _placeholderLabel.Font.WithSize (value.PointSize);
}
}

Same behaviour, of course if we have a nested if with no else, that equals a && so we have:

Suggested change
void UpdatePlaceholderFontSize(UIFont? value)
{
if (value != null)
{
if (_defaultPlaceholderSize == -1)
{
_defaultPlaceholderSize = _placeholderLabel.Font.PointSize;
}
_placeholderLabel.Font = _placeholderLabel.Font.WithSize(value.PointSize);
}
else if (_defaultPlaceholderSize != -1)
{
_placeholderLabel.Font = _placeholderLabel.Font.WithSize(_defaultPlaceholderSize);
}
}
void UpdatePlaceholderFontSize(UIFont? value)
{
if (value is null && _defaultPlaceholderSize != -1) {
_placeholderLabel.Font = _placeholderLabel.Font.WithSize (_defaultPlaceholderSize);
}
else {
if (_defaultPlaceholderSize == -1) {
_defaultPlaceholderSize = _placeholderLabel.Font.PointSize;
}
_placeholderLabel.Font = _placeholderLabel.Font.WithSize (value.PointSize);
}
}

We can now move to the else clause, which is the one that confuses me, looks like you want to set _defaultPlaceholderSize to the size of the _placeholderLabel.Font.PointSize only when it is -1. By the look of the code _defaultPlaceholderSize is never modified by any other value, so why not moving that if outside the else clause?

Suggested change
void UpdatePlaceholderFontSize(UIFont? value)
{
if (value != null)
{
if (_defaultPlaceholderSize == -1)
{
_defaultPlaceholderSize = _placeholderLabel.Font.PointSize;
}
_placeholderLabel.Font = _placeholderLabel.Font.WithSize(value.PointSize);
}
else if (_defaultPlaceholderSize != -1)
{
_placeholderLabel.Font = _placeholderLabel.Font.WithSize(_defaultPlaceholderSize);
}
}
void UpdatePlaceholderFontSize(UIFont? value)
{
if (_defaultPlaceholderSize == -1) {
_defaultPlaceholderSize = _placeholderLabel.Font.PointSize;
}
if (value is null && _defaultPlaceholderSize != -1) {
_placeholderLabel.Font = _placeholderLabel.Font.WithSize (_defaultPlaceholderSize);
}
else {
_placeholderLabel.Font = _placeholderLabel.Font.WithSize (value.PointSize);
}
}

No a huge fan of a read-write variable accessible by the rest of the code to hold the default value and be updated the first time a property is used. Maybe using https://learn.microsoft.com/en-us/dotnet/api/system.lazy-1?view=net-7.0 is a better approach, but I let you judge. Nevertheless the code is cleaner this way, or at least with the first iteration.

I believe you can make it a lot nicer if you use a nullable value, rather than _defaultPlaceholderSize = -1 make it a nfloat? which is much safer, we know we started with null, if not null someone changed it:

Suggested change
void UpdatePlaceholderFontSize(UIFont? value)
{
if (value != null)
{
if (_defaultPlaceholderSize == -1)
{
_defaultPlaceholderSize = _placeholderLabel.Font.PointSize;
}
_placeholderLabel.Font = _placeholderLabel.Font.WithSize(value.PointSize);
}
else if (_defaultPlaceholderSize != -1)
{
_placeholderLabel.Font = _placeholderLabel.Font.WithSize(_defaultPlaceholderSize);
}
}
void UpdatePlaceholderFontSize(UIFont? value)
{
if (_defaultPlaceholderSize is null) {
_defaultPlaceholderSize = _placeholderLabel.Font.PointSize;
}
if (value is null && _defaultPlaceholderSize is not null) {
_placeholderLabel.Font = _placeholderLabel.Font.WithSize (_defaultPlaceholderSize);
}
else {
_placeholderLabel.Font = _placeholderLabel.Font.WithSize (value.PointSize);
}
}

You can make it even smaller using a ternary operator:

Suggested change
void UpdatePlaceholderFontSize(UIFont? value)
{
if (value != null)
{
if (_defaultPlaceholderSize == -1)
{
_defaultPlaceholderSize = _placeholderLabel.Font.PointSize;
}
_placeholderLabel.Font = _placeholderLabel.Font.WithSize(value.PointSize);
}
else if (_defaultPlaceholderSize != -1)
{
_placeholderLabel.Font = _placeholderLabel.Font.WithSize(_defaultPlaceholderSize);
}
}
void UpdatePlaceholderFontSize(UIFont? value)
{
if (_defaultPlaceholderSize is null) {
_defaultPlaceholderSize = _placeholderLabel.Font.PointSize;
}
_placeholderLabel.Font = _placeholderLabel.Font.WithSize (
value is null && _defaultPlaceholderSize is not null ? _defaultPlaceholderSize.Value : value.PointSize);
}
}

The last implementation can be improved by removing the is not for the _defaultPlaceholderSize since we know it was set already:

Suggested change
void UpdatePlaceholderFontSize(UIFont? value)
{
if (value != null)
{
if (_defaultPlaceholderSize == -1)
{
_defaultPlaceholderSize = _placeholderLabel.Font.PointSize;
}
_placeholderLabel.Font = _placeholderLabel.Font.WithSize(value.PointSize);
}
else if (_defaultPlaceholderSize != -1)
{
_placeholderLabel.Font = _placeholderLabel.Font.WithSize(_defaultPlaceholderSize);
}
}
void UpdatePlaceholderFontSize(UIFont? value)
{
if (_defaultPlaceholderSize is null) {
_defaultPlaceholderSize = _placeholderLabel.Font.PointSize;
}
_placeholderLabel.Font = _placeholderLabel.Font.WithSize (
value is null ? _defaultPlaceholderSize.Value : value.PointSize);
}
}

The last suggestion seems the cleaner one. But do as you wish :)

}
}
2 changes: 2 additions & 0 deletions src/Core/src/PublicAPI/net-ios/PublicAPI.Unshipped.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
#nullable enable
override Microsoft.Maui.Handlers.SwitchHandler.NeedsContainer.get -> bool
override Microsoft.Maui.Platform.MauiTextView.Font.get -> UIKit.UIFont?
override Microsoft.Maui.Platform.MauiTextView.Font.set -> void
static Microsoft.Maui.Layouts.LayoutExtensions.ArrangeContentUnbounded(this Microsoft.Maui.IContentView! contentView, Microsoft.Maui.Graphics.Rect bounds) -> Microsoft.Maui.Graphics.Size
override Microsoft.Maui.Platform.MauiTextField.WillMoveToWindow(UIKit.UIWindow? window) -> void
override Microsoft.Maui.Platform.MauiTextView.WillMoveToWindow(UIKit.UIWindow? window) -> void
Expand Down
2 changes: 2 additions & 0 deletions src/Core/src/PublicAPI/net-maccatalyst/PublicAPI.Unshipped.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
#nullable enable
override Microsoft.Maui.Handlers.SwitchHandler.NeedsContainer.get -> bool
override Microsoft.Maui.Platform.MauiTextView.Font.get -> UIKit.UIFont?
override Microsoft.Maui.Platform.MauiTextView.Font.set -> void
static Microsoft.Maui.Layouts.LayoutExtensions.ArrangeContentUnbounded(this Microsoft.Maui.IContentView! contentView, Microsoft.Maui.Graphics.Rect bounds) -> Microsoft.Maui.Graphics.Size
override Microsoft.Maui.Platform.MauiTextField.WillMoveToWindow(UIKit.UIWindow? window) -> void
override Microsoft.Maui.Platform.MauiTextView.WillMoveToWindow(UIKit.UIWindow? window) -> void
Expand Down
20 changes: 20 additions & 0 deletions src/Core/tests/DeviceTests/Handlers/Editor/EditorHandlerTests.iOS.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,26 @@ namespace Microsoft.Maui.DeviceTests
{
public partial class EditorHandlerTests
{
#if IOS
[Fact(DisplayName = "Placeholder Size is the same as control")]
public async Task PlaceholderFontHasTheSameSize()
{
var sizeFont = 22;
var editor = new EditorStub()
{
Text = "Text",
Font = Font.SystemFontOfSize(sizeFont)
};

var nativePlaceholderSize = await GetValueAsync(editor, handler =>
{
return GetNativePlaceholder(handler).Font.PointSize;
});

Assert.True(nativePlaceholderSize == sizeFont);
}
#endif

[Fact(DisplayName = "Placeholder Toggles Correctly When Text Changes")]
public async Task PlaceholderTogglesCorrectlyWhenTextChanges()
{
Expand Down