Add ContentPopup #276

sung-su · 2020-04-21T03:04:33Z

Description of Change

Add ContentPopup.
It is a popup with only one view, and has less interface restrictions than other popups. (TwoButtonPopup, InformationPopup)

API Changes

Added:

public class ContentPopup
{
    public ContentPopup();
    public event EventHandler Dismissed;
    public View Content { get; set; }
    public bool IsOpen { get; set; }
    public void Dismiss();
    protected virtual bool OnBackButtonPressed();
}

public class ContentPopupManager
{
    public static async Task ShowPopup(this INavigation navigation, ContentPopup popup);
    public static async Task ShowPopup(ContentPopup popup);
}

Usage:

ContentPopup popup = new ContentPopup();
await Navigation.ShowPopup(popup);
popup.Dismiss();

Screenshot:

rookiejava · 2020-04-21T04:50:10Z

src/Tizen.Wearable.CircularUI.Forms/ContentPopup.cs

+        /// BindableProperty. Identifies the content bindable property.
+        /// </summary>
+        /// <since_tizen> 4 </since_tizen>
+        public static readonly BindableProperty ContentProperty = BindableProperty.Create(nameof(Content), typeof(View), typeof(TwoButtonPopup), null);


Suggested change

public static readonly BindableProperty ContentProperty = BindableProperty.Create(nameof(Content), typeof(View), typeof(TwoButtonPopup), null);

public static readonly BindableProperty ContentProperty = BindableProperty.Create(nameof(Content), typeof(View), typeof(ContentPopup), null);

sung-su · 2020-04-21T09:44:52Z

Currently, I am considering changing to a renderer structure.

myroot · 2020-04-22T12:15:03Z

src/Tizen.Wearable.CircularUI.Forms/ContentPopup.cs

+            }
+        }
+
+        public void Dispose()


Use Dispose pattern

public void Dispose() { Dispose(true); } protected virtual void Dispose(bool disposing) { if (disposing && _renderer != null) { _renderer.Dispose(); _renderer = null; } }

myroot · 2020-04-22T12:16:47Z

src/Tizen.Wearable.CircularUI.Forms/IContentPopupRenderer.cs

+        /// <summary>
+        /// Shows the popup.
+        /// </summary>
+        void Show();


No more need

myroot · 2020-04-22T12:22:13Z

src/Tizen.Wearable.CircularUI.Forms.Renderer/ContentPopupRenderer.cs

+        public void Dismiss()
+        {
+            _popup?.Hide();
+            _popup?.Dismiss();


Should not use Dismiss, because it destroy popup

myroot · 2020-04-22T12:25:12Z

src/Tizen.Wearable.CircularUI.Forms/ContentPopup.cs

+        /// BindableProperty. Identifies the Content bindable property.
+        /// </summary>
+        /// <since_tizen> 4 </since_tizen>
+        public static readonly BindableProperty ContentProperty = BindableProperty.Create(nameof(Content), typeof(View), typeof(ContentPopup), null);


Need to update parent

void UpdateContent() { OnChildAdded(Content); }

// it is method of Element protected virtual void OnChildAdded(Element child) { child.Parent = this; child.ApplyBindings(skipBindingContext: false, fromBindingContextChanged: true); ChildAdded?.Invoke(this, new ElementEventArgs(child)); OnDescendantAdded(child); foreach (Element element in child.Descendants()) OnDescendantAdded(element); }

myroot · 2020-04-22T12:36:56Z

src/Tizen.Wearable.CircularUI.Forms.Renderer/ContentPopupRenderer.cs

+
+        public void SetElement(Element element)
+        {
+            element.Parent = Application.Current.Parent;


Set only if no parent

src/Tizen.Wearable.CircularUI.Forms.Renderer/ContentPopupRenderer.cs

myroot · 2020-04-22T12:41:14Z

src/Tizen.Wearable.CircularUI.Forms.Renderer/ContentPopupRenderer.cs

+                var renderer = Platform.GetOrCreateRenderer(_element.Content);
+                (renderer as LayoutRenderer)?.RegisterOnLayoutUpdated();
+                var native = renderer.NativeView;
+                var sizeRequest = _element.Content.Measure(XForms.NativeParent.Geometry.Width, XForms.NativeParent.Geometry.Height).Request.ToPixel();


myroot · 2020-04-22T12:42:15Z

src/Tizen.Wearable.CircularUI.Forms.Renderer/ContentPopupRenderer.cs

+                (renderer as LayoutRenderer)?.RegisterOnLayoutUpdated();
+                var native = renderer.NativeView;
+                var sizeRequest = _element.Content.Measure(XForms.NativeParent.Geometry.Width, XForms.NativeParent.Geometry.Height).Request.ToPixel();
+                native.MinimumHeight = sizeRequest.Height;


Content should be always fullscreen size

myroot · 2020-04-22T12:46:38Z

Please test BindingContext chaining is working on ContentPopup.

ContentPopup should use parent's BindingContext, if not set a BindingContext
and Content of ContentPopup also use BindingContext of ContentPage, if not set a BindingContext on Content

rookiejava · 2020-04-24T04:26:42Z

src/Tizen.Wearable.CircularUI.Forms.Renderer/CircularUI.cs

@@ -48,6 +48,7 @@ public static void Init()
        {
            if (IsInitialized) return;
            IsInitialized = true;
+            ContentPopup.RendererFunc = () => new ContentPopupRenderer();


where this technique come from?

come from me 😉

myroot · 2020-04-26T22:16:44Z

src/Tizen.Wearable.CircularUI.Forms.Renderer/ContentPopupRenderer.cs

+        ElmSharp.Popup _popup;
+        ContentPopup _element;
+
+        public void SetElement(Element element)


Suggested change

public void SetElement(Element element)

public void SetElement(ContentPopup element)

Because it is ContentPopupRenderer

src/Tizen.Wearable.CircularUI.Forms.Renderer/ContentPopupRenderer.cs

myroot · 2020-04-26T22:20:57Z

src/Tizen.Wearable.CircularUI.Forms.Renderer/ContentPopupRenderer.cs

+        void OnDismissed(object sender, EventArgs e)
+        {
+            _element.SendDismissed();
+            Dispose();


Why do you dispose it?

myroot · 2020-04-26T22:26:32Z

src/Tizen.Wearable.CircularUI.Forms.Renderer/ContentPopupRenderer.cs

+                var renderer = Platform.GetOrCreateRenderer(_element.Content);
+                (renderer as LayoutRenderer)?.RegisterOnLayoutUpdated();
+                var native = renderer.NativeView;
+                native.MinimumHeight = 360;


Do not use static value 360
How about use align and weight?

native.AlignmentX= -1; native.AlignmentY= -1; native.WeightX = 1; native.WeightY = 1;

myroot · 2020-04-26T22:30:23Z

src/Tizen.Wearable.CircularUI.Forms.Renderer/CircularUI.cs

@@ -48,6 +48,7 @@ public static void Init()
        {
            if (IsInitialized) return;
            IsInitialized = true;
+            ContentPopup.RendererFunc = () => new ContentPopupRenderer();


Suggested change

ContentPopup.RendererFunc = () => new ContentPopupRenderer();

ContentPopup.CreateRenderer = () => new ContentPopupRenderer();

myroot · 2020-04-26T22:39:40Z

test/WearableUIGallery/WearableUIGallery/TC/TCContentPopup.xaml.cs

+
+        private void OnContentPopupDismissButtonClicked(object sender, EventArgs e)
+        {
+            ContentPopup _popup = new ContentPopup();


using (ContentPopup _popup = new ContentPopup()) { var dismiss = new Button { Text = "Dismiss", }; dismiss.Clicked += (s, ee) => { _popup?.Dismiss(); label1.Text = "Test2 Dismissed"; }; var label = new Label { Text = "This ContentPopup is dismissed as a below dismiss button.", HorizontalTextAlignment = TextAlignment.Center, }; var grid = new Grid(); grid.HeightRequest = 360; grid.WidthRequest = 360; grid.RowDefinitions.Add(new RowDefinition()); grid.RowDefinitions.Add(new RowDefinition()); grid.RowDefinitions.Add(new RowDefinition()); grid.RowDefinitions.Add(new RowDefinition()); grid.RowDefinitions.Add(new RowDefinition()); grid.Children.Add(label, 0, 1, 1, 3); grid.Children.Add(dismiss, 0, 1, 3, 4); _popup.Content = grid; _popup.Show(); TaskCompletionSource<bool> tcs = new TaskCompletionSource<bool>(); _popup.Dismissed += (s, evt) => tcs.SetResult(true); _ = await tcs.Task; }

myroot · 2020-04-26T22:43:22Z

test/WearableUIGallery/WearableUIGallery/TC/TCContentPopup.xaml.cs

+
+        private void OnContentPopupDismissBackKeyClicked(object sender, EventArgs e)
+        {
+            ContentPopup _popup = new ContentPopup();


using (ContentPopup _popup = new ContentPopup()) { _popup.BackButtonPressed += (s, ee) => { _popup?.Dismiss(); label1.Text = "Test1 Dismissed"; }; string _longText = "This ContentPopup is dismissed as a back key."; var content = new Label { Text = _longText, HorizontalTextAlignment = TextAlignment.Center }; _popup.Content = content; _popup.Show(); TaskCompletionSource<bool> tcs = new TaskCompletionSource<bool>(); _popup.Dismissed += (s, evt) => tcs.SetResult(true); _ = await tcs.Task; }

myroot · 2020-04-26T22:51:30Z

src/Tizen.Wearable.CircularUI.Forms/ContentPopup.cs

+        /// Shows the popup.
+        /// </summary>
+        /// <since_tizen> 4 </since_tizen>
+        public void Show()


How about rename to Open?
and How about return Task that indicate closing timing

TaskCompletionSource<bool> _tcsForDismiss; public Task Open() { IsShow = true; _tcsForDismiss = new TaskCompletionSource<bool>(); return _tcsForDismiss.Task; } public void SendDismissed() { Dismissed?.Invoke(this, EventArgs.Empty); _tcsForDismiss?.TrySetResult(true); }

If you provide Task we can easily use

using (var popup = new ContentPopup()) { poup.Content = new StackLayout { ... }; _ = await popup.Open(); }

sung-su · 2020-04-28T08:39:39Z

Fixed with reference to the above comments.

myroot · 2020-04-28T23:31:55Z

src/Tizen.Wearable.CircularUI.Forms.Renderer/ContentPopupRenderer.cs

+                var renderer = Platform.GetOrCreateRenderer(_element.Content);
+                (renderer as LayoutRenderer)?.RegisterOnLayoutUpdated();
+                var native = renderer.NativeView;
+                native.MinimumHeight = XForms.NativeParent.Geometry.Height;


How about use AligementX/Y and WeightX/Y?

I will set MinimunHeight and MinimumWidth instead of Alignment and Weight.

myroot · 2020-04-28T23:35:27Z

src/Tizen.Wearable.CircularUI.Forms/ContentPopup.cs

+        /// BindableProperty. Identifies the IsShow bindable property.
+        /// </summary>
+        /// <since_tizen> 4 </since_tizen>
+        public static readonly BindableProperty IsOpenProperty = BindableProperty.Create(nameof(IsOpen), typeof(bool), typeof(ContentPopup), false);


TwoWay binding is more proper
defaultBindingMode : BindingMode.TwoWay

myroot · 2020-04-28T23:37:41Z

src/Tizen.Wearable.CircularUI.Forms/ContentPopupManager.cs

+        public static async Task ShowPopup(ContentPopup popup)
+        {
+            if (popup == null)
+                await Task.FromResult(false);


Suggested change

await Task.FromResult(false);

return Task.FromResult(false);

change to return

myroot · 2020-04-28T23:37:55Z

src/Tizen.Wearable.CircularUI.Forms/ContentPopupManager.cs

+            using (var renderer = DependencyService.Get<IContentPopupRenderer>(DependencyFetchTarget.NewInstance))
+            {
+                if (renderer == null)
+                    await Task.FromResult(false);


Suggested change

await Task.FromResult(false);

return Task.FromResult(false);

change to return

test/WearableUIGallery/WearableUIGallery/TC/TCContentPopup.xaml.cs

myroot · 2020-04-28T23:39:31Z

test/WearableUIGallery/WearableUIGallery/TC/TCContentPopup.xaml.cs

+
+            popup.Content = label;
+
+            await ContentPopupManager.ShowPopup(popup);


Suggested change

await ContentPopupManager.ShowPopup(popup);

await Navigation.ShowPopup(popup);

myroot · 2020-04-28T23:42:35Z

src/Tizen.Wearable.CircularUI.Forms.Renderer/ContentPopupRenderer.cs

+        public Task Open()
+        {
+            _popup.Show();
+            _element.IsOpen = true;


Suggested change

_element.IsOpen = true;

_element.SetValueFromRenderer(ContentPopup.IsOpenProperty, true);

rookiejava

We decided to think again about providing ContentPopup on Galaxy Watch, which has a smaller screen size than mobile or TV.

The two main parts we are concerned with are:

Since the screen is small, the popup always occupies the entire screen, so there is no difference from using the page.
The complexity of managing the bezel interaction between the content of pop-ups created in a new window and the content of the existing page can be increased.

sung-su requested review from JoonghyunCho, myroot, rookiejava and shyunMin April 21, 2020 03:05

rookiejava reviewed Apr 21, 2020

View reviewed changes

sung-su force-pushed the contentpopup branch from ed56cee to d2d2bd5 Compare April 21, 2020 04:53

Add ContentPopup

15d7ac8

rookiejava mentioned this pull request Apr 21, 2020

Popup with CustomUI? #249

Open

sung-su force-pushed the contentpopup branch from d2d2bd5 to 15d7ac8 Compare April 21, 2020 09:42

Refectoring ContentPopup

61915f4

myroot suggested changes Apr 22, 2020

View reviewed changes

Fix ContentPopup

008c361

sung-su force-pushed the contentpopup branch from 0527f14 to 008c361 Compare April 23, 2020 12:16

rookiejava reviewed Apr 24, 2020

View reviewed changes

myroot suggested changes Apr 26, 2020

View reviewed changes

myroot reviewed Apr 26, 2020

View reviewed changes

Add ContentPopupManager

35c5d15

sung-su force-pushed the contentpopup branch from 69da61d to 35c5d15 Compare April 28, 2020 09:03

myroot suggested changes Apr 28, 2020

View reviewed changes

Fix ContentPopup

a2ae1e5

myroot approved these changes Apr 29, 2020

View reviewed changes

JoonghyunCho approved these changes Apr 29, 2020

View reviewed changes

shyunMin approved these changes Apr 29, 2020

View reviewed changes

rookiejava reviewed May 7, 2020

View reviewed changes

rookiejava added the DO-NOT-MERGE-!!! 🛑 label May 7, 2020

	public static readonly BindableProperty ContentProperty = BindableProperty.Create(nameof(Content), typeof(View), typeof(TwoButtonPopup), null);
	public static readonly BindableProperty ContentProperty = BindableProperty.Create(nameof(Content), typeof(View), typeof(ContentPopup), null);

	public void SetElement(Element element)
	public void SetElement(ContentPopup element)

	ContentPopup.RendererFunc = () => new ContentPopupRenderer();
	ContentPopup.CreateRenderer = () => new ContentPopupRenderer();


		popup.Content = label;

		await ContentPopupManager.ShowPopup(popup);

	await ContentPopupManager.ShowPopup(popup);
	await Navigation.ShowPopup(popup);

	_element.IsOpen = true;
	_element.SetValueFromRenderer(ContentPopup.IsOpenProperty, true);

Add ContentPopup #276

Are you sure you want to change the base?

Add ContentPopup #276

Conversation

sung-su commented Apr 21, 2020 • edited Loading

Description of Change

API Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sung-su commented Apr 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

myroot commented Apr 22, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

myroot Apr 26, 2020 • edited Loading

Choose a reason for hiding this comment

sung-su commented Apr 28, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rookiejava left a comment

Choose a reason for hiding this comment

sung-su commented Apr 21, 2020 •

edited

Loading

myroot Apr 26, 2020 •

edited

Loading