Skip to content

Conversation

@JSUYA
Copy link
Member

@JSUYA JSUYA commented Jul 12, 2022

Set the renderer type before creating the engine.
The window is selected as EcoreWl2 or ElmWin
according to the renderer type.

This commit requires engine side commits.
(flutter-tizen/engine#312)

@JSUYA
Copy link
Member Author

JSUYA commented Jul 12, 2022

I think FlutterDesktopViewCreateFromNewEcoreWl2Window and FlutterDesktopViewCreateFromNewElmWindow can be changed with one API. This can be done with additional work.

@JSUYA JSUYA marked this pull request as draft July 12, 2022 04:51
@JSUYA JSUYA force-pushed the dev/renderer_type branch from 03b9c2a to 36a2c89 Compare July 12, 2022 04:58
@JSUYA JSUYA marked this pull request as ready for review July 12, 2022 05:14
@JSUYA
Copy link
Member Author

JSUYA commented Jul 12, 2022

I think FlutterDesktopViewCreateFromNewEcoreWl2Window and FlutterDesktopViewCreateFromNewElmWindow can be changed with one API. This can be done with additional work.

This is a problem that is implemented incorrectly in the engine. I will fix it soon

@JSUYA JSUYA force-pushed the dev/renderer_type branch 2 times, most recently from aea21cd to 1585e61 Compare July 13, 2022 09:26
/// <summary>
/// The renderer type of Flutter engine.
/// </summary>
protected FlutterDesktopRendererType RendererType { get; set; } = FlutterDesktopRendererType.kEGL;
Copy link
Member

Choose a reason for hiding this comment

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

What if the selected type is not supported by the current profile? (i.e. wearable)

How do we automatically select the kEvasGL type for wearable apps?

Copy link
Member Author

Choose a reason for hiding this comment

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

I added profile check macro for this.

@JSUYA JSUYA force-pushed the dev/renderer_type branch 2 times, most recently from d3e6882 to 6b9e2b7 Compare July 14, 2022 06:22
@bbrto21
Copy link
Contributor

bbrto21 commented Jul 15, 2022

Could you please validate a service app? a service app goes into the crash with this PR and flutter-tizen/engine#312
https://github.com/flutter-tizen/flutter-tizen/wiki/Application-types#service-app

//
// Defaults to kEGL. If the profile is wearable, defaults to kEvasGL.
#ifdef WEARABLE_PROFILE
FlutterDesktopRendererType renderer_type_ =
Copy link
Member

Choose a reason for hiding this comment

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

The type FlutterDesktopRendererType is not expected to be exposed through the public interface of the embedding. I'll talk to you offline in detail.

Copy link
Member Author

Choose a reason for hiding this comment

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

In the case of the app, if you want the renderer type to be fixed to the profile, we can remove this member.
(wearable app = EvasGL, others = EGL)

Copy link
Member Author

Choose a reason for hiding this comment

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

I added FlutterRendererType to C# and Cpp.
(I have a question. FlutterDesktopPluginRegistrar and FlutterDesktopEngineRef are already exposed. Should these be fixed later as well? )

Copy link
Member

Choose a reason for hiding this comment

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

Should these be fixed later as well?

Well, I think the cases you mentioned are implemented in that way on purpose (to directly access underlying native pointers) and thus won't need a fix. However, let me take a look again.

Copy link
Member

Choose a reason for hiding this comment

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

IPluginRegistry is a C# version of C++'s PluginRegistry, and its GetRegistrarForPlugin method is expected to return a raw FlutterDesktopPluginRegistrar handle.

That said, we can still provide a higher level API of FlutterDesktopPluginRegistrar in the C# side which provides the same functionality as C++'s PluginRegistrar class. However, even if it's implemented, there will be no use for it.

Copy link
Member

Choose a reason for hiding this comment

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

Also I was thinking of introducing a new public interface for FlutterApp that can be used to configure all (currently experimental) window-related properties (such as window_offset_x_ and is_window_transparent_) and also the renderer type in this PR at once using a single struct. I don't have a concrete plan yet. You may think about it if interested (maybe later).

@JSUYA JSUYA force-pushed the dev/renderer_type branch 2 times, most recently from 3b97a81 to c849340 Compare July 20, 2022 07:44
Copy link
Member

@swift-kim swift-kim left a comment

Choose a reason for hiding this comment

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

Only minor nitpicks.

Could you also change the file mode of FlutterEngine.cs to 644? (It's accidentally 755 right now.)

@JSUYA
Copy link
Member Author

JSUYA commented Jul 20, 2022

Could you please validate a service app? a service app goes into the crash with this PR and flutter-tizen/engine#312 https://github.com/flutter-tizen/flutter-tizen/wiki/Application-types#service-app

I tested on multi-app samples. now works fine

@JSUYA JSUYA force-pushed the dev/renderer_type branch from 987f548 to c37af37 Compare July 20, 2022 10:48
@swift-kim swift-kim added the no merge This PR is not ready for merge yet label Jul 21, 2022
@JSUYA JSUYA force-pushed the dev/renderer_type branch from c37af37 to 3bff10b Compare August 1, 2022 02:03
@JSUYA JSUYA force-pushed the dev/renderer_type branch from 3bff10b to 78543fb Compare August 1, 2022 03:51
@swift-kim swift-kim merged commit c3e0b19 into flutter-tizen:master Aug 2, 2022
@swift-kim swift-kim removed the no merge This PR is not ready for merge yet label Aug 2, 2022
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.

3 participants