Introduce TizenViewElementary #292

JSUYA · 2022-05-27T07:23:07Z

There are two ways to draw FlutterTizenView in Tizen. (TizenWindowElementary and TizenWindowEcore)
Both ways are drawing on Window component(surface).

We try to draw FlutterTizenView on the appropriate component
provided by Tizen UIFW and support it to be place with other components.
To do that, we created a TizenView class on the same level as TizenWindow
and abstracted common methods to TizenViewBase.
we can add FlutterTizenView to other UIFW of Tizen while adding TizenView{?}(maybe Tizen NUI).

 TizenViewBase -> TizenView -> TizenViewElementary
                              -> TizenView{?}
                 -> TizenWindow -> TizenWindowElementary
                                -> TizenWindowEcore

related PR : flutter-tizen/flutter-tizen#373

bbrto21 · 2022-05-27T07:55:39Z

In my opinion, I would recommend that you rebase this PR to the master after #291 is merged.

swift-kim · 2022-05-27T08:12:53Z

The name TizenBaseHandle is somewhat ambiguous. We'll need to discuss about the class design and hierarchy.

shell/platform/tizen/tizen_view_elementary.cc

JSUYA · 2022-05-27T08:29:03Z

The name TizenBaseHandle is somewhat ambiguous. We'll need to discuss about the class design and hierarchy.

Do you have a recommended class name and hierarchy?
I've been discussing names and hierarchy with @bbrto21.
Please let me know if there is a better way

JSUYA · 2022-05-31T00:53:48Z

contribute to flutter-tizen/flutter-tizen#351

bbrto21

please rebase to flutter-3.0.0-tizen

shell/platform/tizen/public/flutter_tizen.h

shell/platform/tizen/tizen_view_elementary.cc

There are two ways to draw FlutterTizenView in Tizen. (TizenWindowElementary and TizenWindowEcore) Both ways are drawing on Window component(surface). We try to draw FlutterTizenView on the appropriate component provided by Tizen UIFW and support it to be place with other components. To do that, we created a TizenView class on the same level as TizenWindow and abstracted common methods to TizenBaseHandle. we can add FlutterTizenView to other UIFW of Tizen while adding TizenView{?}(maybe Tizen NUI). TizenBaseHandle -> TizenView -> TizenViewElementary -> TizenView{?} -> TizenWindow -> TizenWindowElementary -> TizenWindowEcore There are a few unimplemented things. This is a plan to be added later. - We need to add API related to App Event (Resume, etc..). - The plans for key input are not yet cleared up.

JSUYA · 2022-06-07T04:55:14Z

In my opinion, I would recommend that you rebase this PR to the master after #291 is merged.

@bbrto21
I rebased your #291

The name TizenBaseHandle is somewhat ambiguous. We'll need to discuss about the class design and hierarchy.

@swift-kim I renamed it to TizenViewBase as we discussed.
And I cleaned up the level of abstraction.
More reviews are needed. Could you please review it?

shell/platform/tizen/BUILD.gn

shell/platform/tizen/channels/window_channel.h

shell/platform/tizen/flutter_tizen_ecore.cc

shell/platform/tizen/flutter_tizen.cc

shell/platform/tizen/flutter_tizen_ecore.cc

shell/platform/tizen/flutter_tizen_elementary.cc

shell/platform/tizen/flutter_tizen_view.cc

shell/platform/tizen/public/flutter_tizen.h

shell/platform/tizen/channels/window_channel.cc

shell/platform/tizen/flutter_tizen.cc

shell/platform/tizen/flutter_tizen_ecore.cc

shell/platform/tizen/flutter_tizen_elementary.cc

shell/platform/tizen/channels/window_channel.cc

shell/platform/tizen/tizen_view_elementary.cc

shell/platform/tizen/tizen_window.h

shell/platform/tizen/tizen_view_base.h

shell/platform/tizen/tizen_view_elementary.cc

shell/platform/tizen/tizen_window.h

swift-kim · 2022-06-08T08:15:39Z

shell/platform/tizen/tizen_window.h

-  TizenInputMethodContext* input_method_context() {
-    return input_method_context_.get();
-  }
+  std::string GetType() override { return "window"; };


Can we use a dedicated enum instead of a string to represent the type?

Actually this kind of runtime type check is discouraged by the style guide. We may need to either redesign our class hierarchy, or refactor the code so that the type check becomes unnecessary.

Do not hand-implement an RTTI-like workaround. The arguments against RTTI apply just as much to workarounds like class hierarchies with type tags.

I modified it to use enum.

shell/platform/tizen/tizen_window_elementary.h

shell/platform/tizen/flutter_tizen_elementary.cc

shell/platform/tizen/tizen_window_ecore_wl2.cc

shell/platform/tizen/flutter_tizen_view.cc

shell/platform/tizen/tizen_view.h

shell/platform/tizen/tizen_view_elementary.cc

shell/platform/tizen/tizen_view_elementary.h

bbrto21 · 2022-06-13T06:29:48Z

shell/platform/tizen/flutter_tizen_elementary.cc

+                              int32_t height) {
+  auto* view = reinterpret_cast<flutter::FlutterTizenView*>(view_ref);
+  flutter::TizenGeometry view_geometry = {0, 0, width, height};
+  view->tizen_view()->OnGeometryChanged(view_geometry);


In my opinion, the embedder APIs related to View should be something like C wrapper of FlutterTizenView interface.
so, I believe that to provide the resizing, it has to work internally through FlutterTizenView's interface.

This is the basic design I was initially thinking of, so it's open for discussion and you don't need to fix it right away. I'm curious about your opinion.

I don't agree. FlutterTizenView is simply an abstraction layer for a Flutter view and it should not be considered as a wrapper of TizenView.

FlutterTizenView is simply an abstraction layer for a Flutter view and it should not be considered as a wrapper of TizenView.

What you said is a simple principle I came up with when I first designed it.
When I first designed FlutterDeskopViewXXX APIs, I had the following design in mind.

void FlutterDesktopViewXXX() { ... view->XXX() ... }

Currently, FlutterTizenView may have different detailed behaviors depending on TizenViewType.
So I think this idea may not be suitable. That's why I was curious about your thoughts :)

I agree to reduce the abstraction level (depth) of required FlutterDesktopViewXXX.
In the case of resize rather than simple property change, I think it is better to handle it with view->Rsize().
If the ViewType check is handled inside the FlutterTizenView, I think it is easy to fix when the type check is no longer needed through refactoring.

bbrto21 · 2022-06-13T06:33:28Z

shell/platform/tizen/flutter_tizen_view.cc

+FlutterTizenView::FlutterTizenView(std::unique_ptr<TizenViewBase> tizen_view)
+    : tizen_view_(std::move(tizen_view)) {
+  tizen_view_->SetView(this);
+  tizen_view_->BindKeys(kBindableSystemKeys);


When the type of tizen_view_ is not window, I don't think this is the role of the Embedder anymore. I think it should be fixed later.

Yes, that makes sense. I think we can move BindKeys from TizenViewBase to TizenWindow.

I think the view also needs a bind to a special key.
We may also need the TV remote's back key or other special key inside the View.
Since key binding is platform dependent, I think that the information provided for VD profile can be same with either Window or View.

This has already been talked about in private chats.
If necessary, I hope this is discussed in a separate PR regarding special key bindings.

Yes, we can handle it in another PR.

I think the view also needs a bind to a special key.
We may also need the TV remote's back key or other special key inside the View.
Since key binding is platform dependent, I think that the information provided for VD profile can be same with either Window or View.

Sorry. I don't still get why it should be the embedder's role to register the special keys. Isn't is possible for the parent app to call eext_win_keygrab_set with its own window as necessary?

Sorry. I don't still get why it should be the embedder's role to register the special keys. Isn't is possible for the parent app to call eext_win_keygrab_set with its own window as necessary?

Using eext_win_keygrab_set, the app user can also bind a key to the app window.

my opinion is that the flutter app(window and view) created with flutter-tizen can provide the same key bind within the VD profile. If the user has experienced that BackKey works in flutter-app (win), but does not add it to the app in the view, it may not work. Of course, not binding to the app's window may mean that a special key is not used.
This is just my opinion.

however, it seems problematic for the view to modify the properties of the window.
I can also delete the view's bind code from this PR. Should I do that now?
@bbrto21 told me on chat that he would proceed with the modify related to key bind.

shell/platform/tizen/tizen_view_elementary.cc

bbrto21

I think there are a few things to discuss, but it seems to be solvable in further PR.
and it seems to work just as the creator intended. LGTM.

shell/platform/tizen/flutter_tizen_view.cc

shell/platform/tizen/tizen_view.h

swift-kim · 2022-06-13T08:05:36Z

Please don't mark my comment as "resolved" by yourself. I'll do that when necessary.

If I checked your code and didn't close the corresponding comment, there's always a reason for it.

JSUYA · 2022-06-13T08:19:57Z

Please don't mark my comment as "resolved" by yourself. I'll do that when necessary.

If I checked your code and didn't close the corresponding comment, there's always a reason for it.

um... I think of Resolved as a simple expression of the comment("Done") that your review has been accepted.
If you have a reason just do Unresolved again.
If it is the rule of flutter-tizen that the reviewer checks Resolved, I will follow it.

swift-kim · 2022-06-13T08:38:41Z

I told because you were keeping closing comments that I had set as "unresolved". Please leave a simple comment such as "Done" if the issue has been resolved.

bbrto21 · 2022-06-14T00:15:01Z

@swift-kim Do you have anything more to discuss?

bbrto21 · 2022-06-14T00:46:58Z

I told because you were keeping closing comments that I had set as "unresolved". Please leave a simple comment such as "Done" if the issue has been resolved.

Is this a common practice? for those who have just joined the project this can be an inconvenience.
If this is common practice, or if everyone agrees on it, it looks like it needs an addition to the development guide.

swift-kim · 2022-06-14T01:07:45Z

@bbrto21 No, my comment was specifically for him. It's not a rule that applies to everyone.

Do you have anything more to discuss?

It seems there are still some unresolved issues that have not been answered by the author.

Co-authored-by: Swift Kim <[email protected]>

swift-kim

Thank you!

There are two ways to draw FlutterTizenView in Tizen. (TizenWindowElementary and TizenWindowEcore) Both ways are drawing on Window component(surface). We try to draw FlutterTizenView on the appropriate component provided by Tizen UIFW and support it to be place with other components. To do that, we created a TizenView class on the same level as TizenWindow and abstracted common methods to TizenViewBase.

JSUYA changed the title ~~Introduce to TizenViewElementary~~ Introduce TizenViewElementary May 27, 2022

JSUYA force-pushed the dev/flutterview-elm branch from a1a960d to edc4b5d Compare May 27, 2022 07:44

JSUYA mentioned this pull request May 27, 2022

Introduce ElmFlutterView class flutter-tizen/flutter-tizen#373

Merged

bbrto21 reviewed May 27, 2022

View reviewed changes

shell/platform/tizen/tizen_view_elementary.cc Outdated Show resolved Hide resolved

JSUYA force-pushed the dev/flutterview-elm branch from abaa673 to 70a663c Compare May 31, 2022 02:46

bbrto21 mentioned this pull request May 31, 2022

Refactor the key event handling #291

Merged

JSUYA force-pushed the dev/flutterview-elm branch from 70a663c to a38eea4 Compare May 31, 2022 06:08

bbrto21 reviewed Jun 2, 2022

View reviewed changes

JSUYA added 4 commits June 3, 2022 14:31

Change class name TizenBaseHandle to TizenViewBase

311e6b0

Change argument type

ca61f57

Fix code format

b3bb0d6

JSUYA force-pushed the dev/flutterview-elm branch from a38eea4 to 2df8308 Compare June 7, 2022 04:48

bbrto21 reviewed Jun 7, 2022

View reviewed changes

Fix abstraction level

2e777e8

JSUYA force-pushed the dev/flutterview-elm branch 2 times, most recently from d04cc71 to f87e45f Compare June 7, 2022 08:47

Fix api name and description

a1f8ba4

JSUYA force-pushed the dev/flutterview-elm branch from f87e45f to a1f8ba4 Compare June 7, 2022 08:48

Fix scroll event

2c14b3a

JSUYA force-pushed the dev/flutterview-elm branch from c228671 to 2c14b3a Compare June 8, 2022 02:16

swift-kim reviewed Jun 8, 2022

View reviewed changes

JSUYA added 2 commits June 9, 2022 14:06

Update based on review

c282815

Modify view size calc

7041b0e

Modify GetWindowId

0625e16

swift-kim reviewed Jun 9, 2022

View reviewed changes

shell/platform/tizen/flutter_tizen_elementary.cc Outdated Show resolved Hide resolved

shell/platform/tizen/tizen_window_ecore_wl2.cc Outdated Show resolved Hide resolved

Update based on review

eaf8efd

swift-kim reviewed Jun 13, 2022

View reviewed changes

Clean up include header

0fa299f

JSUYA force-pushed the dev/flutterview-elm branch from 7d58102 to 0fa299f Compare June 13, 2022 06:24

bbrto21 reviewed Jun 13, 2022

View reviewed changes

bbrto21 approved these changes Jun 13, 2022

View reviewed changes

swift-kim reviewed Jun 13, 2022

View reviewed changes

shell/platform/tizen/flutter_tizen_view.cc Show resolved Hide resolved

shell/platform/tizen/tizen_view.h Outdated Show resolved Hide resolved

Fix integer type

08c9172

Update shell/platform/tizen/tizen_view_elementary.cc

0e39447

Co-authored-by: Swift Kim <[email protected]>

swift-kim approved these changes Jun 14, 2022

View reviewed changes

bbrto21 merged commit b98c6b6 into flutter-tizen:flutter-3.0.0-tizen Jun 14, 2022

JSUYA deleted the dev/flutterview-elm branch June 20, 2022 01:40

bbrto21 mentioned this pull request Jun 20, 2022

Add the view's GetWidth/Height APIs #300

Closed

swift-kim mentioned this pull request Jun 29, 2022

TizenViewElementary: Change the size of view when the size of container changes #302

Merged

Introduce TizenViewElementary #292

Introduce TizenViewElementary #292

Uh oh!

Conversation

JSUYA commented May 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bbrto21 commented May 27, 2022

Uh oh!

swift-kim commented May 27, 2022

Uh oh!

Uh oh!

JSUYA commented May 27, 2022

Uh oh!

JSUYA commented May 31, 2022

Uh oh!

bbrto21 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JSUYA commented Jun 7, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JSUYA commented May 27, 2022 •

edited

Loading

bbrto21 left a comment •

edited

Loading

swift-kim commented Jun 13, 2022 •

edited

Loading

bbrto21 commented Jun 14, 2022 •

edited

Loading

swift-kim commented Jun 14, 2022 •

edited

Loading