Introduce ElmFlutterView class #373

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

This class is the FlutterView class available in EFL UIFW.
According to the Elementary widget api rule(?),
Evas_Object type parent widget is always required before engine run.
When FlutterView starts the Flutter engine normally through RunFlutterEngine(),
it can acquire the Evas Object handle.

Example)

ElmFlutterView flutter_view(parent, width, height);

if (flutter_view.RunEngine()) {
  RegisterPlugins(&flutter_view);
  Evas_Object *evas_object = static_cast<Evas_Object *>(flutter_view.evas_object());

  elm_box_pack_end(parent, evas_object);
}

related PR : flutter-tizen/engine#292

embedding/cpp/elm_flutter_view.cc

WonyoungChoi · 2022-05-27T08:02:22Z

embedding/cpp/include/elm_flutter_view.h

+  void *GetEvasImageHandle() { return evas_image_; };
+
+  // The width of the view, or the maximum width if the value is zero.
+  int32_t window_width_ = 0;


window_width_ and window_height_ can be set again after starting the View engine for now.
How about providing getter methods instead?

Thank you for review.
I added Getter and Resize methods.
This is resize test image (https://github.com/JSUYA/jsuya.github.io/blob/master/ElmFlutterView_test.gif)

I made a getter for size, but now there are some issues. ( + #373 (comment))
I'll apply your comments on Getter through another PR.

@WonyoungChoi Anything more to discuss?

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

contribute to #351

embedding/cpp/elm_flutter_view.cc

embedding/cpp/include/elm_flutter_view.h

embedding/cpp/elm_flutter_view.cc

swift-kim · 2022-05-31T02:54:26Z

embedding/cpp/elm_flutter_view.cc

+}
+
+bool ElmFlutterView::RunFlutterEngine(void *elm_parent, int32_t width,
+                                      int32_t height) {


This is a public API and cannot be changed in the future without breaking existing apps. Is it okay to finalize this API? The name of the API (RunFlutterEngine) should be able to tolerate possible implementation changes such as

The engine instance becomes reusable. That is, if a cached engine instance is available, you can use the instance and don't have to "run" a new engine.

Starting an engine and creating a view become separate steps. (e.g. Allow the user to pre-initialize an engine instance, to reduce the time to draw the first frame in an add-to-app scenario.) The engine instance might be even passed from outside of ElmFlutterView.

I don't mean that these changes will necessarily take place in the future, but we just have to consider various scenarios before finalizing the API.

The reuse of engine instances has not been considered yet. The engine's lifecycle follows ElmFlutterView.
As far as I know, FlutterView does not consider multi-instance of engine.
I will discuss this a little more.

How do you like this design?

ElmFlutterView flutter_view(parent, width, height); // flutter_view.SetEngine(cached_engine); // this may be implemented later flutter_view.RunEngine(); // implicitly creates an engine if not set

Also we need to think carefully about properties such as dart_entrypoint_ and dart_entrypoint_args_. If an engine can be started before creating an ElmFlutterView, having the dart_entrypoint_ value inside ElmFlutterView might be a design flaw because ElmFlutterView doesn't necessarily create an engine on RunEngine.

I renamed the API to RunEngine and added a new constructor.

About dart_entrypoint I agree too. Currently not working. I think we need an Interface for these.
I think we can keep this code until considering the new interface(for entrypoint or other argument(sw renderer)).

I think @swift-kim's opinion is reasonable.
Maybe we've reached the point where we need to abstract about the engine?

In fact, since the view and the engine APIs are provided respectively by Embedder, it is a very natural idea to set the engine that has already been executed(or cached) in the view.
preparing this for a variety of uses, such as caching or spawning engine, seems may to help us in many ways.

But I'm not saying it's necessary right now.

You're right. but don't we also need a way to warm up or cache the engine? even if flutter-tizen be provided as an app or service.

The page you linked is about adding a Flutter screen to a normal app. The same thing applies to Tizen as well (although we don't have a concept of Android's Activity in Tizen). If a developer wants to utilize "engine warm-up" in a single screen Flutter app, they can implement the native "app" part by themself and use a cached engine with an explicit ElmFlutterView attached to the app.

Oh, I see, ElmFlutterView can take a that role. If so, it is expected that there will be no breaking change even if abstraction of the engine is carried out in the current state.

I think I can take over the part that was further discussed because @JSUYA left the office for a while.
(or it would be better to open another PR when he returns to the office.)

Okay, the name of the method (RunEngine) seems flexible enough to tolerate any possible future API changes. Let's discuss later again when we are to make further changes.

swift-kim · 2022-05-31T04:51:25Z

embedding/cpp/elm_flutter_view.cc

+  engine_prop.dart_entrypoint_argc = entrypoint_args.size();
+  engine_prop.dart_entrypoint_argv = entrypoint_args.data();
+
+  engine_ = FlutterDesktopEngineCreate(engine_prop);


FlutterApp, FlutterServiceApp, and ElmFlutterView have duplicated portions of code (lines 29-55, and the data members in the header) and we might be able to improve reusability by introducing a common base class for all (e.g. FlutterView). How do you think? It's just an idea and we don't have to restructure the classes in this PR right now.

In addition to these, I think we should consider the interface of Lifecycle Callback and App Event Callback. I think the view should provide those kinds of things. These interfaces seem to be common.

I would recommend that this be treated as an another topic.

embedding/cpp/elm_flutter_view.cc

bbrto21 · 2022-06-07T05:37:56Z

Can you share an example like the app you showed here?

JSUYA · 2022-06-07T05:46:00Z

Can you share an example like the app you showed here?

@bbrto21
https://github.com/JSUYA/gallery/tree/test_flutterview
This is sample here

embedding/cpp/elm_flutter_view.cc

embedding/cpp/include/elm_flutter_view.h

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

LGTM

swift-kim

Further work is required, but seems enough for now.

embedding/cpp/include/elm_flutter_view.h

bbrto21 · 2022-06-15T00:50:10Z

This PR must include latest engine release hash, without this you cannot build a c++ host flutter-tizen app.
and this can cause the plugin's build action to fail.

This class is the FlutterView class available in EFL UIFW. According to the Elementary widget api rule(?), Evas_Object type parent widget is always required at the time of creation. When FlutterView starts the Flutter engine normally through OnCreate(), it can acquire the Evas Image handle. Example) ```cpp ElmFlutterView flutter_view(parent_box); // Optional. flutter_view.window_width_ = 720; flutter_view.window_height_ = 600; if (flutter_view.OnCreate()) { RegisterPlugins(&flutter_view); Evas_Object* evas_image = (Evas_Object*)flutter_view.GetEvasImageHandle(); elm_box_pack_end(parent_box, evas_image); } ```

Co-authored-by: Boram Bae <[email protected]>

JSUYA · 2022-06-17T02:09:52Z

This PR must include latest engine release hash, without this you cannot build a c++ host flutter-tizen app. and this can cause the plugin's build action to fail.

I added a patch to update the engine hash. thank you for check

embedding/cpp/elm_flutter_view.cc

bbrto21 · 2022-06-20T02:33:55Z

I added a patch to update the engine hash. thank you for check

Thanks! It is now expected that there will be no problems.

JSUYA mentioned this pull request May 27, 2022

Introduce TizenViewElementary flutter-tizen/engine#292

Merged

JSUYA changed the title ~~Introduce to ElmFlutterView class~~ Introduce ElmFlutterView class May 27, 2022

JSUYA force-pushed the dev/jsuya/cpp_view branch from 05d4b0c to 45e6193 Compare May 27, 2022 07:43

WonyoungChoi reviewed May 27, 2022

View reviewed changes

embedding/cpp/elm_flutter_view.cc Outdated Show resolved Hide resolved

WonyoungChoi reviewed May 27, 2022

View reviewed changes

swift-kim reviewed May 31, 2022

View reviewed changes

JSUYA force-pushed the dev/jsuya/cpp_view branch from 4a33ba7 to 19b4925 Compare June 7, 2022 05:15

JSUYA force-pushed the dev/jsuya/cpp_view branch from 19b4925 to d3dc7ce Compare June 10, 2022 02:41

swift-kim reviewed Jun 13, 2022

View reviewed changes

bbrto21 reviewed Jun 14, 2022

View reviewed changes

embedding/cpp/include/elm_flutter_view.h Outdated Show resolved Hide resolved

bbrto21 reviewed Jun 14, 2022

View reviewed changes

embedding/cpp/include/elm_flutter_view.h Outdated Show resolved Hide resolved

bbrto21 approved these changes Jun 14, 2022

View reviewed changes

swift-kim approved these changes Jun 14, 2022

View reviewed changes

embedding/cpp/include/elm_flutter_view.h Show resolved Hide resolved

embedding/cpp/include/elm_flutter_view.h Outdated Show resolved Hide resolved

swift-kim reviewed Jun 14, 2022

View reviewed changes

embedding/cpp/include/elm_flutter_view.h Outdated Show resolved Hide resolved

swift-kim added the no merge This PR is not ready for merge yet label Jun 15, 2022

JSUYA and others added 9 commits June 17, 2022 10:00

Add resize api

519842c

Change API name

270f675

Update based on review

8f7cc8e

Change RunEngine interface

7cf2e7c

Update embedding/cpp/include/elm_flutter_view.h

b84297d

Co-authored-by: Boram Bae <[email protected]>

Update embedding/cpp/include/elm_flutter_view.h

29c7ebe

Co-authored-by: Boram Bae <[email protected]>

Remove unused entrypoint_arg member

11ec19a

Update engine hash to 91edce9

0b15541

JSUYA force-pushed the dev/jsuya/cpp_view branch from fa3b601 to 0b15541 Compare June 17, 2022 02:06

Fix comment

27d3489

JSUYA force-pushed the dev/jsuya/cpp_view branch from 122a9ec to 27d3489 Compare June 17, 2022 02:25

swift-kim reviewed Jun 17, 2022

View reviewed changes

embedding/cpp/elm_flutter_view.cc Show resolved Hide resolved

Move local variables declaration

b75ec18

JSUYA force-pushed the dev/jsuya/cpp_view branch from 9e9fa3a to b75ec18 Compare June 20, 2022 02:12

bbrto21 removed the no merge This PR is not ready for merge yet label Jun 20, 2022

bbrto21 merged commit 59a6084 into flutter-tizen:master Jun 20, 2022

JSUYA deleted the dev/jsuya/cpp_view branch June 20, 2022 07:56

bbrto21 mentioned this pull request Jun 21, 2022

Introduce FlutterEngine #380

Closed

Introduce ElmFlutterView class #373

Introduce ElmFlutterView class #373

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!

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 31, 2022

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!

Choose a reason for hiding this comment

Uh oh!

swift-kim Jun 14, 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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bbrto21 commented Jun 7, 2022

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bbrto21 left a comment

Choose a reason for hiding this comment

Uh oh!

swift-kim left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

JSUYA commented Jun 17, 2022

Uh oh!

Uh oh!

bbrto21 commented Jun 20, 2022

Uh oh!

Reviewers

JSUYA commented May 27, 2022 •

edited

Loading

swift-kim Jun 14, 2022 •

edited

Loading

JSUYA commented Jun 7, 2022 •

edited

Loading

bbrto21 commented Jun 15, 2022 •

edited

Loading