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

feat(ColorPicker): Add ColorPicker, ColorPickerSlider & ColorSpectrum #3955

Merged
merged 45 commits into from
Sep 28, 2020

Conversation

robloo
Copy link
Contributor

@robloo robloo commented Sep 3, 2020

GitHub Issue (If applicable): #

Closes #3875

PR Type

What kind of change does this PR introduce?

  • Feature

What is the current behavior?

The Microsoft.UI.Xaml.Controls.ColorPicker and related primitives (ColorSpectrum, ColorPickerSlider) are not implemented.

What is the new behavior?

The following controls are implemented:
Microsoft.UI.Xaml.Controls.ColorPicker
Microsoft.UI.Xaml.Controls.Primitives.ColorPickerSlider
Microsoft.UI.Xaml.Controls.Primitives.ColorSpectrum

PR Checklist

Please check if your PR fulfills the following requirements:

Other information

Submission containing materials of a third party: Microsoft, MIT License https://github.com/microsoft/microsoft-ui-xaml

Internal Issue (If applicable):

Notes

Architecture of the ColorPicker itself can be found here:
https://github.com/microsoft/microsoft-ui-xaml/blob/786129a37d36890d9d8ef1a61f945cc2f3be19a4/dev/ColorPicker/readme.md

  • Rgb and Hsv were converted to structs and placed in their own files, take a look at ColorConversion.h to see what is in WinRT
  • Rgb and Hsv structs in WinUI have lowercase fields. This did not align with C# convention so all fields were converted to uppercase properties -- which unnecessarily changed a lot of code. Since every line was re-written to C# it seems justifiable. Edit: The properties were changed back to fields so their ref could be used.
  • New internal static classes ColorConversion and ColorHelpers were created. It is required in C# as methods cannot be directly in a namespace.
  • ColorConversion goes in a new 'Common' directory outside of ColorPicker. This follows the organization of WinUI.
  • ColorChangedEventArgs, as projected in C#, has get-only properties. In WinRT you can read/write to the properties and setting to the property directly is done in the RaiseColorChanged() method in ColorPicker. To keep this read-only externally, properties were set to 'internal set'
  • The property changed callbacks in ColorPicker/ColorSpectrum were adjusted to to avoid OnColorPropertyChanged -> OnPropertyChanged -> OnColorChanged which makes little sense in C# and is only needed in C++ with the auto-generated properties. Instead OnPropertyChanged -> OnColorChanged is called (same ideas as in NumberBox). All of this is considered negligible as the properties are nearly 100% re-written for C#.
  • DownlevelHelper.ToDisplayNameExists() was implemented as a stub and always returns false. It automatically disables using ColorHelper.ToDisplayName (which isn't implemented in Uno) and it provides a straightforward mechanism to enable this in the future without any code changes. The base ColorPicker code will still match WinUI.
  • On RS2 or above the ColorPicker actually uses it's own SpectrumBrush to render the spectrum itself. This is disabled and I'm falling back to the byte array -> WritableBitmap -> ImageBrush methodology which should work just fine in Uno. If Uno ever supports Composition this could be re-enabled for added performance. This means SpectrumBrush.cs is not in this port.
  • The threading in ColorSpectrum/ColorHelper was simply ported to C# it does not always use the standard async/await/Task patterns of C#.
  • IColorChangedEventArgs may not be required as it isn't projected into C# normally.
  • GetImpl() was not added to the AutomationPeers. It is not clear why this exists in NumberBox.
  • String literals were removed from dependency property registration and nameof() is used instead. This is different from other controls like NumberBox but convention is so strong here other controls should probably be using nameof() as well.
  • Resource lookup is not using string literals directly. It maintains the 'key' variables from WinUI. Again other ported controls should probably be updated to go back to this convention. New keys can be added to the ResourceAccessor.ResourceKeys.cs partial class.
  • The code is 1-1 converted C++ -> C# in all places possible. Where it had to re-write in some places I commented out the C++ code nearby so you can compare in the future. This is especially important when porting in future changes from WinUI to know where the code goes as it was replaced with a C# implementation. Codacy doesn't like that commented out code.
  • 'this.' was being used in code inconsistently in the C# port. In C++ it isn't used at all. Now 'this.' is only used for public properties and events. It may need to be removed entirely in the future.

Questions

  1. What to do about strings in string resources that have a different format than .net String.Format() accepts. Should these be converted? Is there a string formatter that is equivalent?
  2. Is disabling the ContractXPresent/NotPresent code in XAML still required?
    • It shouldn't be necessary to comment it out with Uno 3.0, the ContractPresent/NotPresent markup should work correctly.
  3. The existing NumberBox is disposing of the event handlers in a strange way. What is the proper method of detaching event handlers (for template part controls) for ported code?

@robloo robloo requested a review from a team September 3, 2020 20:06
@gitpod-io
Copy link

gitpod-io bot commented Sep 3, 2020

@CLAassistant
Copy link

CLAassistant commented Sep 3, 2020

CLA assistant check
All committers have signed the CLA.

@davidjohnoliver
Copy link
Contributor

davidjohnoliver commented Sep 4, 2020

Very impressive! And the thorough PR documentation is a great help. 👍

Your questions:

What to do about strings in string resources that have a different format than .net String.Format() accepts. Should these be converted? Is there a string formatter that is equivalent?

Not sure off the top of my head. Do you have an example?

Is disabling the ContractXPresent/NotPresent code in XAML still required?
It shouldn't be required any more.

PS: re the Codacy tool - for C++-converted code, take the warnings with a grain of salt; fidelity with the original code is preferable to following C# stylistic best practices.

Copy link
Contributor

@davidjohnoliver davidjohnoliver left a comment

Choose a reason for hiding this comment

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

The imported code looks good as best as I can judge - the thorough importation notes are much appreciated.

It looks like the build is failing on some (all?) platforms because Math.Clamp() isn't available in .NET Standard 2.0 - you can use the internal MathEx.Clamp() method instead.

@@ -153,6 +153,8 @@
<Service Include="{508349b6-6b84-4df5-91f0-309beebad82d}" />
</ItemGroup>
<ItemGroup>
<Folder Include="Microsoft\UI\Xaml\Controls\Common\" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove the changes to Uno.UI.csproj from the PR (and you can safely revert them locally too) - they get added when you include files via Visual Studio, but really they're unnecessary, because files are picked up by globbing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I noticed that when I imported the Strings folder it just popped up automatically in the solution. I'll be sure to clean this up!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Concerning InvalidOperationException, I debated using that one or
InvalidEnumArgumentException. Both don't fit 100% so I went with the generic base Exception with a note to take another look. I'll be happy to switch it, thanks for the recommendation.

@robloo
Copy link
Contributor Author

robloo commented Sep 4, 2020

Not sure off the top of my head. Do you have an example?

The string formatting methods can take arguments to automatically insert the specified arguments into the string. In C# this might look like "The value is {0:X2}" -> "The value is FF" in C++/WinRT the string formatting is different. Instead I have localized strings that look like:

<data name="ValueStringColorSpectrumWithColorName" xml:space="preserve">
<value>%1!s!, Hue %2!u!, Saturation %3!u!, Brightness %4!u!</value>
<comment>The string to provide as the UIA value of the ColorSpectrum when we're including the friendly color name. %1 is the friendly name of the color; %2, %3, and %4 are the HSV values for the color between 0 and 359 (for hue) or 0 and 100 (for saturation or value).</comment>
</data>

PS: re the Codacy tool - for C++-converted code, take the warnings with a grain of salt; fidelity with the original code is preferable to following C# stylistic best practices.

Great! I was hoping you would say that. I ensured the code is 1-1 all places possible but where I had to re-write I commented out the C++ code nearby so you can compare in the future. Especially important when porting in future changes from WinUI to know where the code goes if it was replaced with a C# implementation. Codacy doesn't like that.

Is disabling the ContractXPresent/NotPresent code in XAML still required?

I was actually referring to the XAML files where I noticed in NumberBox all the Contract7Present namespace usage was commented out:

<!--UNO TODO <contract7Present:Setter Target="InputBox.CornerRadius" Value="{Binding Source={ThemeResource ControlCornerRadius}}" />-->

It looks like the build is failing on some (all?) platforms because Math.Clamp() isn't available in .NET Standard 2.0 - you can use the internal MathEx.Clamp() method instead.

I'm feeling brave and will remove the clamping. I was hitting overflows in an earlier port but I think some fixes elsewhere will render these unnecessary now. It will then match the original C++ implementation as well.

I have another question:

  • The existing NumberBox is disposing of the event handlers in a strange way. What is the proper method of detaching event handlers (for template part controls) for ported code?

@robloo robloo changed the title [WIP] Add ColorPicker [WIP] feat(ColorPicker): Add ColorPicker, ColorPickerSlider & ColorSpectrum Sep 4, 2020
This code is now aligned more closely with C++ which does not use clamping. In the original C# port overflows were occurring so clamping was added. It is believed other fixes make the clamping unnecessary now.
@davidjohnoliver
Copy link
Contributor

Is disabling the ContractXPresent/NotPresent code in XAML still required?

I was actually referring to the XAML files where I noticed in NumberBox all the Contract7Present namespace usage was commented out

I messed up the reply 😆. It shouldn't be necessary to comment it out with Uno 3.0, the ContractPresent/NotPresent markup should work correctly.

I have another question:

The existing NumberBox is disposing of the event handlers in a strange way. What is the proper method of detaching event handlers (for template part controls) for ported code?

Let me address the broader question of events/lifetime concerns in Uno, then try to explain what's going on in NumberBox (as much as anything for a rough draft that I can add to the contributor docs).

Event handling patterns in Uno revolve around the fact that on iOS, in particular, it's very easy to create memory leaks because the underlying native runtime uses reference-counting for memory management. That means that any reference cycle - like a child view holding a hard reference to a parent view - automatically creates a leak where that subtree will never get collected. This most commonly manifests when subscribing to events on child views.

The pattern to address this in Uno is typically the following:

  • subscribe to the events in OnApplyTemplate()
  • and also from Loaded
  • and unsubscribe from Unloaded

That way the reference cycle is guaranteed to be broken when the view is removed the visual tree, avoiding a leak.

It's not obligatory to use the 'disposable pattern' for this, you can subscribe+unsubscribe in a normal way if you prefer. The disposable pattern just makes it somewhat easier to avoid double subscriptions, forgotten unsubscriptions etc.


As for how the 'disposable pattern' in NumberBox works: It's making use of two fairly simple types, SerialDisposable and CompositeDisposable, that were pinched from Reactive Extensions. As the names imply they both implement IDisposable and have the following qualities:

  • whenever SerialDisposable.Disposable is set, the previous value of the disposable, if any, is disposed
  • whenever CompositeDisposable is disposed, all of its children are disposed
  • the Add(Action action) extension method for CompositeDisposable registers an action to be run when the CompositeDisposable is disposed

So to take a subsection of the relevant code:

		private void InitializeTemplate()
		{
			_eventSubscriptions.Disposable = null;

			var registrations = new CompositeDisposable();

			if (this.GetTemplateChild(c_numberBoxDownButtonName) is RepeatButton spinDown)
			{
				spinDown.Click += OnSpinDownClick;
				registrations.Add(() => spinDown.Click -= OnSpinDownClick);
			}
			...

			_eventSubscriptions.Disposable = registrations;
		}

What that code is doing is:

  1. Unsubscribing any previous event subscriptions (_eventSubscriptions.Disposable = null)
  2. Subscribing to the OnSpinDownClick event
  3. Creating a handler that will unsubscribe from the event when disposed (registrations.Add(() => spinDown.Click -= OnSpinDownClick))
  4. Registering that handler (and any others) for later disposal (_eventSubscriptions.Disposable = registrations)

Sorry for the long reply, but I'm probably going to turn this comment into a section in the docs 😄

These changes are not needed as files/folders are automatically pulled in
@unoplatform unoplatform deleted a comment from davidjohnoliver Sep 4, 2020
These are the full, unmodified files from WinUI -- spaces included.
Removes most places where 'this.' was being used in code. In C++ it isn't used at all and was quite inconsistent in the C# port. Now 'this.' is only used for public properties and events.
@robloo
Copy link
Contributor Author

robloo commented Sep 4, 2020

It's not obligatory to use the 'disposable pattern' for this, you can subscribe+unsubscribe in a normal way if you prefer. The disposable pattern just makes it somewhat easier to avoid double subscriptions, forgotten unsubscriptions etc.

I normally clean up the event handlers and have a note to get that done. However, looking at NumberBox I realized there might be more to it. Thanks so much for your detailed response! I was planning on unsubscribing one-by-one in the Unloaded event. However, using the reactive pattern is definitely helpful so you can remove all at once. It should simplify a lot of code and help minimize chances of mistakes. I'll take a look at what makes the most sense here.

@robloo
Copy link
Contributor Author

robloo commented Sep 7, 2020

@davidjohnoliver I have been testing this with Android and the color spectrum is not rendering at all and the control is zero size. The other components of the ColorPicker seem to appear just fine though. As the ColorSpectrum was working fine in C# this is either a problem with the Uno Platform or a simple mistake importing the code into Uno. Either way, I can't seem to find any errors being reported to discover what the problem might be. I think I need someone with in-depth knowledge of Uno to take a look at what the problem might be. If you have a chance to look at this please see ColorPicker_SimpleText in the sample app.

}

// Uno Doc: C++ methodology does not easily convert to C#
/*wchar_t *end;
Copy link
Contributor

Choose a reason for hiding this comment

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

Codacy found an issue: Remove this commented out code.

strongThis.UpdateBitmapSources();
strongThis.UpdateEllipse();
});
//});
Copy link
Contributor

Choose a reason for hiding this comment

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

Codacy found an issue: Remove this commented out code.

// On RS2 and later, we put the spectrum images in a loaded image surface,
// which we then put into a SpectrumBrush.
// Uno Doc: SpectrumBrush is disabled for now
//private LoadedImageSurface m_hueRedSurface;
Copy link
Contributor

Choose a reason for hiding this comment

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

Codacy found an issue: Remove this commented out code.

}
});

//if (m_createImageBitmapAction != null)
Copy link
Contributor

Choose a reason for hiding this comment

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

Codacy found an issue: Remove this commented out code.


// If we're adding a small increment, then we'll just add or subtract 1.
// If we're adding a large increment, then we want to snap to the next
// or previous major value - for hue, this is every increment of 30;
Copy link
Contributor

Choose a reason for hiding this comment

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

Codacy found an issue: Remove this commented out code.

}

// Uno Doc: The following code is not supported in C# and is removed.
//byte *pixelBuffer = nullptr;
Copy link
Contributor

Choose a reason for hiding this comment

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

Codacy found an issue: Remove this commented out code.

{
for (int x = 0; x < width; x++)
{
//if (workItem.Status == AsyncStatus.Canceled)
Copy link
Contributor

Choose a reason for hiding this comment

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

Codacy found an issue: Remove this commented out code.

private SolidColorBrush m_checkerColorBrush;

// Uno Doc: Added to dispose event handlers
private SerialDisposable _eventSubscriptions = new SerialDisposable();
Copy link
Contributor

Choose a reason for hiding this comment

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

double ellipseY = Canvas.GetTop(selectionEllipsePanel) + selectionEllipsePanel.Height / 2;

this.EllipseXTextBlock.Text = Math.Round(ellipseX).ToString();
this.EllipseYTextBlock.Text = Math.Round(ellipseY).ToString();
Copy link
Contributor

Choose a reason for hiding this comment

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

private Vector4 m_oldHsvColor = new Vector4(0.0f, 0.0f, 1.0f, 1.0f);

// Uno Doc: Added to dispose event handlers
private SerialDisposable _eventSubscriptions = new SerialDisposable();
Copy link
Contributor

Choose a reason for hiding this comment

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

var strongThis = this;
//m_createImageBitmapAction.Completed = new AsyncActionCompletedHandler(
//(IAsyncAction asyncInfo, AsyncStatus asyncStatus) =>
//{
Copy link
Contributor

Choose a reason for hiding this comment

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

Codacy found an issue: Remove this commented out code.

this.BlueTextBlock.Text = this.ColorPicker.Color.B.ToString();
this.AlphaTextBlock.Text = this.ColorPicker.Color.A.ToString();

var verifySubclass = new MyColorSpectrum();
Copy link
Contributor

Choose a reason for hiding this comment

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

private Rectangle m_alphaSliderBackgroundRectangle;
private ImageBrush m_alphaSliderCheckeredBackgroundImageBrush;

private IAsyncAction m_alphaSliderCheckeredBackgroundBitmapAction = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

public ColorPicker()
{
// Uno Doc: Not supported
//__RP_Marker_ClassById(RuntimeProfiler::ProfId_ColorPicker);
Copy link
Contributor

Choose a reason for hiding this comment

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

Codacy found an issue: Remove this commented out code.

string hexString = string.Format(CultureInfo.InvariantCulture, "#{0:X8}", hexValue);

// We'll size this string to accommodate "#XXXXXXXX" - i.e., a full ARGB number with a # sign.
//wchar_t hexString[10];
Copy link
Contributor

Choose a reason for hiding this comment

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

Codacy found an issue: Remove this commented out code.

}

// Uno Doc: This section assigning rgbValue/alphaValue was re-written in C# to avoid strange usage of functions in C++
//const bool isAlphaEnabled = IsAlphaEnabled();
Copy link
Contributor

Choose a reason for hiding this comment

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

Codacy found an issue: Remove this commented out code.

string hexString = string.Format(CultureInfo.InvariantCulture, "#{0:X6}", hexValue);

// We'll size this string to accommodate "#XXXXXX" - i.e., a full RGB number with a # sign.
//wchar_t hexString[8];
Copy link
Contributor

Choose a reason for hiding this comment

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

Codacy found an issue: Remove this commented out code.

private Rectangle m_previousColorRectangle;
private ImageBrush m_colorPreviewRectangleCheckeredBackgroundImageBrush;

private IAsyncAction m_createColorPreviewRectangleCheckeredBackgroundBitmapAction = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

// Implementation of the signum function, which returns the sign of a number while discarding its value.
/*template <typename T>
int sgn(T val)
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Codacy found an issue: Remove this commented out code.

@robloo
Copy link
Contributor Author

robloo commented Sep 28, 2020

@davidjohnoliver I see you already made the internal change, thanks. I also updated to the latest WinUI version to get some important bug fixes. On my end I'm complete, so it's ready whenever the Uno Team is ready to merge.

@davidjohnoliver davidjohnoliver merged commit e697fa3 into unoplatform:master Sep 28, 2020
@davidjohnoliver
Copy link
Contributor

It's in! Thanks again @robloo for this great contribution. :)

@robloo
Copy link
Contributor Author

robloo commented Sep 28, 2020

Awesome! Thanks so much for all your guidance and help. I really appreciate all that you and the Uno Team has accomplished - it is no small feat. It's also a huge time saver for me going forward with cross-platform development.

@robloo robloo deleted the robloo/colorpicker branch September 28, 2020 19:15
@jeromelaban
Copy link
Member

Thanks for the contribution!!

@MelbourneDeveloper
Copy link

@robloo Hi. Would we expect this to be in the current release 3.4? I'm not seeing the ColorPicker control on Android or other platforms...

@robloo
Copy link
Contributor Author

robloo commented Jan 24, 2021

Hi @MelbourneDeveloper, yes this should be in current 3.4. Both iOS and Android were tested to work (however slow) when the code was added.

Have you added the WinUI library to your project? This control is only available under the new Microsoft.UI.Xaml namespaces NOT the Windows.UI.Xaml namespace of vanilla UWP.

If thats not it, it might be a regression so please file a new issue.

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.

Add ColorPicker & ColorSpectrum Control
6 participants