Skip to content

Support textDecorationLine on <Text> components (#709) #1056

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

Merged
matthargett merged 17 commits into from Apr 7, 2017
Merged

Support textDecorationLine on <Text> components (#709) #1056

Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
12ac2cb
feat(WPF) Support textDecorationLine on <Text> components (#709)
ymusiychuk-lohika Feb 28, 2017
5de9c8e
Underline implementation for UWP
ymusiychuk-lohika Mar 6, 2017
b4a8fe2
Removed reference to System.Runtime.Serialization
ymusiychuk-lohika Mar 6, 2017
bdc0a98
Styling fixes
ymusiychuk-lohika Mar 6, 2017
c4b93d3
Returned back System.Serialization
ymusiychuk-lohika Mar 7, 2017
27f7ce1
Style changes
ymusiychuk-lohika Mar 7, 2017
fdbb0ec
Removed ChakraCore directory
ymusiychuk-lohika Mar 7, 2017
c05a80e
Merge branch 'master' into textDecorationLine-support-709
matthargett Mar 7, 2017
5b5aa6b
Merge branch 'master' into textDecorationLine-support-709
matthargett Mar 8, 2017
355182a
Small EnumHelpers reflection improvement
ymusiychuk-lohika Mar 9, 2017
f35c67a
Revert "Underline implementation for UWP"
ymusiychuk-lohika Mar 9, 2017
ced5687
Line endings fix
ymusiychuk-lohika Mar 9, 2017
1db84ac
Merge branch 'master' into textDecorationLine-support-709
matthargett Mar 9, 2017
5e842a0
Moved implementation from ShadowNode to TextViewManager and SpanViewM…
ymusiychuk-lohika Mar 13, 2017
e259858
Returned ReactTextShadowNode.cs to initial state
ymusiychuk-lohika Mar 13, 2017
2e2f60e
Merge branch 'master' into textDecorationLine-support-709
matthargett Apr 6, 2017
4850e9c
Merge branch 'master' into textDecorationLine-support-709
kevinvangelder Apr 7, 2017
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
1 change: 1 addition & 0 deletions ReactWindows/ReactNative.Net46/ReactNative.Net46.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@
<HintPath>$(SolutionDir)\packages\System.Reactive.Windows.Threading.3.1.1\lib\net45\System.Reactive.Windows.Threading.dll</HintPath>
<Private>True</Private>
</Reference>
<Reference Include="System.Runtime.Serialization" />
<Reference Include="System.Windows" />
<Reference Include="System.Windows.Forms" />
<Reference Include="System.Xaml" />
Expand Down
32 changes: 31 additions & 1 deletion ReactWindows/ReactNative.Net46/Views/Text/ReactTextViewManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
using ReactNative.UIManager;
using ReactNative.Reflection;
using ReactNative.UIManager;
using ReactNative.UIManager.Annotations;
using System;
using System.Collections;
using System.Collections.Generic;
using System.Linq;
using System.Windows;
using System.Windows.Controls;
using System.Windows.Documents;
Expand Down Expand Up @@ -40,6 +43,33 @@ public void SetColor(TextBlock view, uint? color)
: null;
}

/// <summary>
/// Sets the TextDecorationLine for the node.
/// </summary>
/// <param name="view">The view.</param>
/// <param name="textDecorationLineValue">The TextDecorationLine value.</param>
[ReactProp(ViewProps.TextDecorationLine)]
public void SetTextDecorationLine(TextBlock view, string textDecorationLineValue)
{
var textDecorationLine = EnumHelpers.ParseNullable<TextDecorationLine>(textDecorationLineValue) ?? TextDecorationLine.None;
switch (textDecorationLine)
{
case TextDecorationLine.Underline:
view.TextDecorations = TextDecorations.Underline;
break;
case TextDecorationLine.LineThrough:
view.TextDecorations = TextDecorations.Strikethrough;
break;
case TextDecorationLine.UnderlineLineThrough:
view.TextDecorations = new TextDecorationCollection(TextDecorations.Underline.Concat(TextDecorations.Strikethrough));
break;
case TextDecorationLine.None:
default:
view.TextDecorations = null;
break;
}
}

/// <summary>
/// Sets whether or not the text is selectable.
/// </summary>
Expand Down
1 change: 1 addition & 0 deletions ReactWindows/ReactNative.Shared/ReactNative.Shared.projitems
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,7 @@
<Compile Include="$(MSBuildThisFileDirectory)Views\TextInput\ReactTextInputSelectionEvent.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Views\TextInput\ReactTextInputShadowNode.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Views\TextInput\ReactTextInputSubmitEditingEvent.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Views\Text\TextDecorationLine.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Views\ViewManagersPropertyCache.cs" />
<Compile Include="$(MSBuildThisFileDirectory)Views\View\ReactViewManager.cs" />
</ItemGroup>
Expand Down
32 changes: 27 additions & 5 deletions ReactWindows/ReactNative.Shared/Reflection/EnumHelpers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Linq;
using System.Reflection;
using System.Runtime.Serialization;
using static System.FormattableString;

namespace ReactNative.Reflection
Expand All @@ -15,11 +17,7 @@ public static T Parse<T>(string value)
{
var lookup = s_enumCache.GetOrAdd(
typeof(T),
type => Enum.GetValues(type)
.Cast<object>()
.ToDictionary(
e => Normalize(e.ToString()),
e => e));
type => EnumToDictionary(type));

var result = default(object);
if (!lookup.TryGetValue(Normalize(value), out result))
Expand All @@ -41,6 +39,30 @@ public static T Parse<T>(string value)
return Parse<T>(value);
}

private static Dictionary<string, object> EnumToDictionary(Type type)
{
var result = new Dictionary<string, object>();

var names = Enum.GetNames(type);
var values = Enum.GetValues(type);

for (int i = 0; i < values.Length; ++i)
{
string name = names[i];
object value = values.GetValue(i);

result.Add(Normalize(name), value);

var enumMemberAttribute = type.GetField(name).GetCustomAttribute(typeof(EnumMemberAttribute), false);
if (enumMemberAttribute != null)

@rozele rozele Mar 8, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It's unfortunate that we now have to pay a reflection penalty for all other enums where this does not exist. I know this only occurs once, but its a potential performance hit nonetheless.

@ymusiychuk-lohika ymusiychuk-lohika Mar 9, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That's true that Reflection brings in an overhead, but in this case, due to the fact that this method is called only once per enum, I think it's too small to have a significant impact on performance.

{
result.Add(Normalize(((EnumMemberAttribute)enumMemberAttribute).Value), value);

@rozele rozele Mar 8, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

EnumMemberAttribute [](start = 43, length = 19)

Just curious, does the serialization behavior of EnumMemberAttribute only work in one direction? I.e., is the value only used to serialize to string, or is there some tool that can be used for parsing as well?

@ymusiychuk-lohika ymusiychuk-lohika Mar 9, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It works both ways. For example,
Newtonsoft.Json.JsonConvert.SerializeObject(TextDecorationLine.UnderlineLineThrough, new StringEnumConverter())
will respect the EnumMemberAttribute (notice the StringEnumConverter. Without it, enum is serialized as int)

}
}

return result;
}

private static string Normalize(string value)
{
return value.ToLowerInvariant().Replace("-", "");
Expand Down
36 changes: 35 additions & 1 deletion ReactWindows/ReactNative.Shared/Views/Text/ReactSpanViewManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
using ReactNative.UIManager;
using ReactNative.Reflection;
using ReactNative.UIManager;
using ReactNative.UIManager.Annotations;
using System;
using System.Collections;
using System.Collections.Generic;
using System.Linq;

#if WINDOWS_UWP
using Windows.UI.Xaml;
using Windows.UI.Xaml.Documents;
Expand Down Expand Up @@ -55,6 +59,36 @@ public void SetColor(Span view, uint? color)
: null;
}

#if !WINDOWS_UWP
/// <summary>
/// Sets the TextDecorationLine for the node.
/// </summary>
/// <param name="view">The view.</param>
/// <param name="textDecorationLineValue">The TextDecorationLine value.</param>
[ReactProp(ViewProps.TextDecorationLine)]
public void SetTextDecorationLine(Span view, string textDecorationLineValue)
{
var textDecorationLine = EnumHelpers.ParseNullable<TextDecorationLine>(textDecorationLineValue) ?? TextDecorationLine.None;

switch (textDecorationLine)
{
case TextDecorationLine.Underline:
view.TextDecorations = TextDecorations.Underline;
break;
case TextDecorationLine.LineThrough:
view.TextDecorations = TextDecorations.Strikethrough;
break;
case TextDecorationLine.UnderlineLineThrough:
view.TextDecorations = new TextDecorationCollection(TextDecorations.Underline.Concat(TextDecorations.Strikethrough));
break;
case TextDecorationLine.None:
default:
view.TextDecorations = null;
break;
}
}
#endif

/// <summary>
/// Adds a child at the given index.
/// </summary>
Expand Down
28 changes: 28 additions & 0 deletions ReactWindows/ReactNative.Shared/Views/Text/TextDecorationLine.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
using System.Runtime.Serialization;

namespace ReactNative.Views.Text
{
/// <summary>
/// TextDecorationLine values.
/// </summary>
public enum TextDecorationLine
{
/// <summary>
/// Text has no line decoration.
/// </summary>
None,
/// <summary>
/// Text is Underlined.
/// </summary>
Underline,
/// <summary>
/// Text is Stroke out.
/// </summary>
LineThrough,
/// <summary>
/// Text is both Underlined and Stroke out.
/// </summary>
[EnumMember(Value = "underline line-through")]
UnderlineLineThrough

@rozele rozele Mar 8, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is cool, this is compatible with the existing EnumHelpers class? We should re-write all the other enums to use this where possible. Ideally we could get rid of that reflection hack.

@rozele rozele Mar 8, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We actually don't need this enum, you can just as easily switch case on the string itself.

In reply to: 104831419 [](ancestors = 104831419)

@rozele rozele Mar 8, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That will avoid the dependency on System.Runtime.Serialization, and avoid the reflection driven changes to EnumHelpers

In reply to: 104832645 [](ancestors = 104832645,104831419)

@ymusiychuk-lohika ymusiychuk-lohika Mar 9, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I personally vote for enum instead of string, if there is a limited number of variants, because it
makes corresponding code strongly-typed.

@rozele rozele Mar 9, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

While I tend to agree that strong typing is usually good, I don't like that we have to add yet another dependency and pay an attribute reflection penalty for all enums across the framework just to support this strong typing.

In reply to: 105130501 [](ancestors = 105130501)

@ymusiychuk-lohika ymusiychuk-lohika Mar 9, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

When making decision on changing EnumHelpers I was looking onto "textAlign". Logically it is same as "textDecorationLine" and they both should be treated in same way. But "textDecorationLine" contains single value with space in it, which makes it impossible to use existing approach. So, either it must be an exceptional enum, treated differently (How many such enums can we possibly have?) or the approach should be changed. I've chosen the latter.
I am not against "switch" so if you insist, that's ok. But, please note, Reflection does not have "that" bad impact on performance if it happens once. Yes, if we have 100 000 of enums, then we will see the difference, but currently we have ~20 total and maybe it will grow up to 100 or 200 at most.

}
}