Refactor embedder and introduce FlutterTizenView #278

bbrto21 · 2022-04-21T11:21:18Z

Introduce FlutterTizenView. It is a delegate for the flutter rendering host. and it provides interfaces.
Introduce TizenView. it abstracts the platform window and it provides interfaces.
Responsibilities of TizenRenderer is reduced, and those responsibilities are transferred to TizenWindow and FlutterTizenView.
Rename the Flutter-tizen C-APIs. this change requires an update of flutter-tizen and plugin too.
The engine runs as headless mode by default now.

Furthermore, I plan to gradually add flutter-tizen C-APIs that allow implementing views at Embedding using FlutterTizenView.

P.S I'm sorry for making such a big diff, but there's not much difference in behaviors.

bbrto21 · 2022-04-22T10:22:07Z

shell/platform/tizen/flutter_tizen_engine.h

-  std::unique_ptr<KeyEventHandler> key_event_handler_;
-
-  // An event dispatcher for Ecore mouse events.
-  std::unique_ptr<TouchEventHandler> touch_event_handler_;


The event handlers are not engine dependent now
I think It is better to depend on something that can be hosts to a flutter-tizen, such as a Window or any UI toolkit component.

bbrto21 · 2022-04-22T10:25:41Z

I checked all flutter-tizen app types on the emulators and devices using this PR.(includes window_channel behavior)
To test this PR, please use flutter-tizen/flutter-tizen#358

bwikbs · 2022-04-23T07:48:27Z

shell/platform/tizen/flutter_project_bundle.h

  std::filesystem::path aot_library_path_;

+  // Custom entrypoint in the Dart project
+  std::string custom_dart_entrypoint_;


What is this for? I mean.. What scenario was it made for? I thought this was necessary for a certain scenario... I wonder what it is.

A most typical scenario is a multi-packaging app.
this is not a new feature, it has just been refactored. please see this

bwikbs · 2022-04-23T07:54:15Z

Rename the Flutter-tizen C-APIs. this change requires an update of flutter-tizen and plugin too.

Is there any special reason for this?

swift-kim

When you say "engine" in this embedder codebase, it normally refers to an instance of the Flutter engine wrapper (FlutterTizenEngine) and you don't typically need to prefix it with flutter_tizen_ or FlutterTizen in most contexts. The same may apply to FlutterTizenView and FlutterTizenWindow instances.

The overall direction of this change looks good to me. We'll just need some more clarifications and style fixes.

shell/platform/tizen/channels/platform_channel.cc

shell/platform/tizen/channels/window_channel.cc

shell/platform/tizen/flutter_project_bundle.h

shell/platform/tizen/flutter_project_bundle.cc

shell/platform/tizen/flutter_project_bundle_unittests.cc

shell/platform/tizen/flutter_tizen_view.cc

shell/platform/tizen/flutter_tizen_view.h

bbrto21 · 2022-04-26T03:10:08Z

Rename the Flutter-tizen C-APIs. this change requires an update of flutter-tizen and plugin too.

Is there any special reason for this?

For APIs based on a specific handle, it is natural to start names with that type in C.

bbrto21 · 2022-04-26T07:44:35Z

When you say "engine" in this embedder codebase, it normally refers to an instance of the Flutter engine wrapper (FlutterTizenEngine) and you don't typically need to prefix it with flutter_tizen_ or FlutterTizen in most contexts. The same may apply to FlutterTizenView and FlutterTizenWindow instances.

The overall direction of this change looks good to me. We'll just need some more clarifications and style fixes.

Thank you for review. I've update PR based on your comments.

bbrto21 · 2022-04-28T02:48:13Z

When you say "engine" in this embedder codebase, it normally refers to an instance of the Flutter engine wrapper (FlutterTizenEngine) and you don't typically need to prefix it with flutter_tizen_ or FlutterTizen in most contexts. The same may apply to FlutterTizenView and FlutterTizenWindow instances.

Thank you for helping me understand the terms used in the project!

shell/platform/tizen/flutter_project_bundle_unittests.cc

shell/platform/tizen/flutter_tizen_engine_unittest.cc

shell/platform/tizen/flutter_tizen_view.cc

shell/platform/tizen/flutter_tizen_view.h

shell/platform/tizen/tizen_renderer_egl.cc

shell/platform/tizen/tizen_window_ecore_wl2.cc

shell/platform/tizen/tizen_window_ecore_wl2.h

shell/platform/tizen/tizen_window_elementary.cc

swift-kim · 2022-04-28T06:48:27Z

shell/platform/tizen/tizen_window_elementary.cc

+  // FIXME : evas_object_geometry_get() and ecore_wl2_window_geometry_get() are
+  // not equivalent.


Could you elaborate on how they are different?

Let's suppose the window is initialized as follows { width: 720, height: 1280 }, In this situation, depending on the rotation, these show the following results:

When the window rotation is 90 or 270 degree
evas_object_geometry_get returns: { width: 1280, height: 720 }
but
ecore_wl2_window_geometry_get returns: { width: 720, height: 1280 }

shell/platform/tizen/tizen_renderer_egl.h

swift-kim · 2022-05-03T02:09:00Z

shell/platform/tizen/flutter_tizen_view.cc

+constexpr char kExitKey[] = "XF86Exit";
+
+// Keys that should always be handled by the app first but not by the system.
+const std::vector<std::string> kBindableSystemKeys = {


This constant is not being used.

Where do you think BindKeys should be called? In this class (in the constructor)?

Exactly! This is my mistake. Thank you for finding this out.

swift-kim · 2022-05-03T02:37:14Z

shell/platform/tizen/flutter_tizen_view.cc

+      delta_y * scroll_offset_multiplier, timestamp, device_kind, device_id);
+}
+
+void FlutterTizenView::OnKey(Ecore_Event_Key* event, bool is_down) {


This class (FlutterTizenView) is an abstraction layer between the platform window and Flutter, so it's expected to be platform type agnostic. Do you plan to refactor this part again in the future? Should we wrap this type (Ecore_Event_Key) into another type that contains all information required by TextInputChannel, PlatformViewChannel, and KeyEventChannel, or just extract the values and pass them as arguments (just like KeyboardHook() of the Windows embedder)?

Note) Refactoring the input system is out of scope of this PR. I just want to hear your thoughts.

You are right. actually, I wanted to refactor handling key events similar to the point event in this PR.
However, I thought that this was related to input method context and it lead too many additional changes. so I didn't do it.
I think this is the part that must be refactored. I plan to gradually refactor Embedder including this and develop new feature related View.

Okay, improving the input system is also related to the issue #221. We'll revisit this topic soon.

* This is Intial commit to refactor engine using View concept. Signed-off-by: Boram Bae <boram21.bae@samsung.com>

* FlutterTizenView is more suitable for top-level graphics-related interfaces than FlutterTizenEngine. Signed-off-by: Boram Bae <boram21.bae@samsung.com>

* Name of APIs based on handle must start with handle type name. Signed-off-by: Boram Bae <boram21.bae@samsung.com>

* This is initial implementation based on ecore. * There is a lot of to-do things (event handler, rework based on enligntment). Signed-off-by: Boram Bae <boram21.bae@samsung.com>

* Touch event handling is now dependent on FlutterTizenWindow and FlutterTizenView. Signed-off-by: Boram Bae <boram21.bae@samsung.com>

Signed-off-by: Boram Bae <boram21.bae@samsung.com>

* Remove KeyEventHandler * Add key event hander to FlutterTizenWindow impl Signed-off-by: Boram Bae <boram21.bae@samsung.com>

Signed-off-by: Boram Bae <boram21.bae@samsung.com>

* remove a prefix "Flutter" Signed-off-by: Boram Bae <boram21.bae@samsung.com>

Signed-off-by: Boram Bae <boram21.bae@samsung.com>

* If view is not specified, the engine is run in headless mode. Signed-off-by: Boram Bae <boram21.bae@samsung.com>

Signed-off-by: Boram Bae <boram21.bae@samsung.com>

swift-kim

The code looks good to me, but I have not tested the changes on real devices. Thank you for your hard work.

* Introduce FlutterTizenView. It is a delegate for the flutter rendering host. and it provides interfaces. * Introduce TizenView. it abstracts the platform window and it provides interfaces. Responsibilities of TizenRenderer is reduced, and those responsibilities are transferred to TizenWindow and FlutterTizenView. * Rename the Flutter-tizen C-APIs. this change requires an update of flutter-tizen and plugin too. * The engine runs as headless mode by default now. Signed-off-by: Boram Bae <boram21.bae@samsung.com>

bbrto21 · 2022-05-25T05:01:46Z

contribute to flutter-tizen/flutter-tizen#351

* Introduce FlutterTizenView. It is a delegate for the flutter rendering host. and it provides interfaces. * Introduce TizenView. it abstracts the platform window and it provides interfaces. Responsibilities of TizenRenderer is reduced, and those responsibilities are transferred to TizenWindow and FlutterTizenView. * Rename the Flutter-tizen C-APIs. this change requires an update of flutter-tizen and plugin too. * The engine runs as headless mode by default now. Signed-off-by: Boram Bae <boram21.bae@samsung.com>

bbrto21 marked this pull request as draft April 21, 2022 11:21

bbrto21 force-pushed the refactor branch 3 times, most recently from efbfa7c to 2369967 Compare April 22, 2022 01:50

bbrto21 changed the title ~~Refactor Wndow related things~~ Refactor engine and Introduce FlutterTizenView Apr 22, 2022

bbrto21 force-pushed the refactor branch from 68f7166 to f8bf2b5 Compare April 22, 2022 05:19

bbrto21 mentioned this pull request Apr 22, 2022

Apply embedder C API changes flutter-tizen/flutter-tizen#358

Merged

bbrto21 force-pushed the refactor branch from f8bf2b5 to be92ddc Compare April 22, 2022 06:16

bbrto21 marked this pull request as ready for review April 22, 2022 10:16

bbrto21 commented Apr 22, 2022

View reviewed changes

bwikbs reviewed Apr 23, 2022

View reviewed changes

swift-kim reviewed Apr 25, 2022

View reviewed changes

bbrto21 changed the title ~~Refactor engine and Introduce FlutterTizenView~~ Refactor embedder and introduce FlutterTizenView Apr 26, 2022

swift-kim reviewed Apr 28, 2022

View reviewed changes

bbrto21 mentioned this pull request May 2, 2022

Current limitations of the Evas GL renderer flutter-tizen/embedder#7

Open

7 tasks

swift-kim reviewed May 3, 2022

View reviewed changes

bbrto21 added 8 commits May 3, 2022 14:42

Implement FlutterTizenView foundation

8a5e823

* This is Intial commit to refactor engine using View concept. Signed-off-by: Boram Bae <boram21.bae@samsung.com>

Add graphics callback to FlutterTizenView

d29bd8f

* FlutterTizenView is more suitable for top-level graphics-related interfaces than FlutterTizenEngine. Signed-off-by: Boram Bae <boram21.bae@samsung.com>

Change flutter-tizen API names

aaed4a7

* Name of APIs based on handle must start with handle type name. Signed-off-by: Boram Bae <boram21.bae@samsung.com>

Implement FlutterTizenView, FlutterTizenWindow

9ebdd2e

* This is initial implementation based on ecore. * There is a lot of to-do things (event handler, rework based on enligntment). Signed-off-by: Boram Bae <boram21.bae@samsung.com>

Refactor touch event

638665d

* Touch event handling is now dependent on FlutterTizenWindow and FlutterTizenView. Signed-off-by: Boram Bae <boram21.bae@samsung.com>

Add GetRotatoin to FlutterTizenWindow

0fdd175

Signed-off-by: Boram Bae <boram21.bae@samsung.com>

Move window dependent channels to FlutterTizenView

4377447

Signed-off-by: Boram Bae <boram21.bae@samsung.com>

Refactor key event

b8a42f0

* Remove KeyEventHandler * Add key event hander to FlutterTizenWindow impl Signed-off-by: Boram Bae <boram21.bae@samsung.com>

bbrto21 added 21 commits May 3, 2022 14:42

Fix a WindowChannel bug

b5e3fad

Signed-off-by: Boram Bae <boram21.bae@samsung.com>

Rename TizenWindow

bd6632a

* remove a prefix "Flutter" Signed-off-by: Boram Bae <boram21.bae@samsung.com>

Cleanup minor things

745212a

Signed-off-by: Boram Bae <boram21.bae@samsung.com>

Implement TizenWindowElementary

30746fa

Signed-off-by: Boram Bae <boram21.bae@samsung.com>

Fix gn format error

11aaa65

Signed-off-by: Boram Bae <boram21.bae@samsung.com>

Fix a bug on headless

73ca452

Signed-off-by: Boram Bae <boram21.bae@samsung.com>

Fix a custom dart entrypoint bug

8b379af

Signed-off-by: Boram Bae <boram21.bae@samsung.com>

Remove a headed property

36fecf5

* If view is not specified, the engine is run in headless mode. Signed-off-by: Boram Bae <boram21.bae@samsung.com>

Fix a crash on testing

8c26155

Signed-off-by: Boram Bae <boram21.bae@samsung.com>

Remove prefix "FlutterXXX"

394fc64

Signed-off-by: Boram Bae <boram21.bae@samsung.com>

Adding a trailing comma for readability

2f3ae4c

Signed-off-by: Boram Bae <boram21.bae@samsung.com>

Add null check for custom entrypoint

319e5c1

Signed-off-by: Boram Bae <boram21.bae@samsung.com>

Update based on review

7030839

Signed-off-by: Boram Bae <boram21.bae@samsung.com>

Change API name based on review

0fd880d

Signed-off-by: Boram Bae <boram21.bae@samsung.com>

Update based on review

3c913a3

Signed-off-by: Boram Bae <boram21.bae@samsung.com>

Clean up minor things

4fb0343

Signed-off-by: Boram Bae <boram21.bae@samsung.com>

Update based on review

6cca9bf

Signed-off-by: Boram Bae <boram21.bae@samsung.com>

Remove prefix "FLUTTER" from protection macro

e7781a9

Signed-off-by: Boram Bae <boram21.bae@samsung.com>

Fix a typo

b989ba3

Signed-off-by: Boram Bae <boram21.bae@samsung.com>

Fix regression changes

2910892

Signed-off-by: Boram Bae <boram21.bae@samsung.com>

Call bindKeys when Creating the view

6d136c4

Signed-off-by: Boram Bae <boram21.bae@samsung.com>

bbrto21 force-pushed the refactor branch from f64ddf0 to 6d136c4 Compare May 3, 2022 05:44

bbrto21 added 2 commits May 3, 2022 14:55

Updated based on review

5fef421

Signed-off-by: Boram Bae <boram21.bae@samsung.com>

Fix a typo

67324df

Signed-off-by: Boram Bae <boram21.bae@samsung.com>

swift-kim approved these changes May 3, 2022

View reviewed changes

bbrto21 merged commit 6ac2e23 into flutter-tizen:flutter-2.10.1-tizen May 3, 2022

		// FIXME : evas_object_geometry_get() and ecore_wl2_window_geometry_get() are
		// not equivalent.

Refactor embedder and introduce FlutterTizenView #278

Refactor embedder and introduce FlutterTizenView #278

Uh oh!

Conversation

bbrto21 commented Apr 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bbrto21 commented Apr 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bwikbs Apr 23, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

bwikbs commented Apr 23, 2022

Uh oh!

swift-kim left a comment

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!

Uh oh!

Uh oh!

bbrto21 commented Apr 26, 2022

Uh oh!

bbrto21 commented Apr 26, 2022

Uh oh!

bbrto21 commented Apr 28, 2022

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!

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!

bbrto21 May 3, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

swift-kim left a comment

Choose a reason for hiding this comment

Uh oh!

bbrto21 commented May 25, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bbrto21 commented Apr 21, 2022 •

edited

Loading

bbrto21 commented Apr 22, 2022 •

edited

Loading

bwikbs Apr 23, 2022 •

edited

Loading

bbrto21 May 3, 2022 •

edited

Loading