Skip to content

Conversation

@bbrto21
Copy link

@bbrto21 bbrto21 commented Jul 5, 2022

  • TizenViewEventHandlerDelegate only provides a limited interface for delegating event handling.
  • FlutterDesktopViewResize should work on all types of views.
  • Implementations of TizenViewBase::SetGeometry only manipulate top-level raw object such as window, container.
    other components in the view are manipulated on a resize callback.
  • window channel use SetGeometry normally.

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

bbrto21 added 6 commits July 5, 2022 11:29
Signed-off-by: Boram Bae <boram21.bae@samsung.com>
Signed-off-by: Boram Bae <boram21.bae@samsung.com>
* Do not manipulate the view, but manipulate renderer.

Signed-off-by: Boram Bae <boram21.bae@samsung.com>
* Use ecore_wl2_window_rotation_geometry_set instead of
  ecore_wl2_window_geometry_set, it triggers a ECORE_WL2_EVENT_WINDOW_CONFIGURE
  callback.

Signed-off-by: Boram Bae <boram21.bae@samsung.com>
Signed-off-by: Boram Bae <boram21.bae@samsung.com>
Signed-off-by: Boram Bae <boram21.bae@samsung.com>
@bbrto21 bbrto21 requested a review from HakkyuKim July 5, 2022 04:40
@bbrto21 bbrto21 changed the title Introduce TizenViewEventHandlerDelegate and Rework FlutterDesktopViewResize Introduce TizenViewEventHandlerDelegate and rework FlutterDesktopViewResize Jul 5, 2022
Signed-off-by: Boram Bae <boram21.bae@samsung.com>
Signed-off-by: Boram Bae <boram21.bae@samsung.com>
Copy link
Member

@JSUYA JSUYA left a comment

Choose a reason for hiding this comment

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

Please check my comments

Comment on lines 129 to 135
int32_t x = 0, y = 0, width = 0, height = 0;
evas_object_geometry_get(self->container_, &x, &y, &width, &height);

evas_object_size_hint_min_set(self->container_, width, height);
evas_object_size_hint_max_set(self->container_, width, height);

EvasObjectResizeWithMinMaxHint(self->image_, width, height);
Copy link
Member

Choose a reason for hiding this comment

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

Directly accessing self->container_ and setting value within this callback and calling EvasObjectResizeWithMinMaxHint goes against the delegation you talked about so far.

If the container and image need to resize, call sefl->view_delegate_->Resize(??) and handle the appropriate process inside.

Signed-off-by: Boram Bae <boram21.bae@samsung.com>
@bbrto21 bbrto21 requested a review from swift-kim July 5, 2022 09:23
Copy link
Member

@JSUYA JSUYA left a comment

Choose a reason for hiding this comment

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

For further explanation, I'm leaving my opinions from the offline meeting here.

First of all, I don't think it's appropriate to directly call container_ and image_'s evas object resize api in resize callback.
Since the declaration of the Resize callback function is in TizenViewElementary, it can look like calling an internal member. But I don't think that. I think that the Resize that occurred on TizenViewXXXX should be delegated to FlutterTizenView and FlutterTizenView should call Resize of TizenViewXXXX in a consistent way.
That's why I think we should delegate the behavior through appropriately modified FlutterTizenView::OnResize() or additional FlutterTizenView::XXXX, and that method should call setGeoemtry or a function that resizes container_ and image_ through the appropriate TizenView::XXX method.

Secondary, FlutterTizenView::Resize only changes container_. As a result, FlutterTizenView::Resize depends on the callback of the container_.
For example, If we need pressKey API for (Elm)FlutterView, we can create FlutterDesktopViewPressKeyXXX() API and add FlutterTizenView::PressKey() to it.
Then, add the TizenViewElemetary's presskey to call it again, or occure to key event through evas_object_event_callback_call() of event_layer_ and rely on that callback.
This is unnatural and I think we can just call OnKey() directly from FlutterTizenView::PressKey().
The same goes for Resize. In FlutterTizenView::Resize, OnResize is called immediately, and OnResize is just by calling the appropriate resize method for the engine and flutterview.
(Of course, additional work is required, such as preventing duplicate calls by checking the size in the callback.)
At this time, the evas event API calls of the container_ and 'image_` of the callback function should be separated into separate functions.

My opinion is complicated and can be wrong. This opinion does not affect the actual behavior. And it can be applied later through additional refactoring.(If you need).
Please don't feel pressured

Signed-off-by: Boram Bae <boram21.bae@samsung.com>
@bbrto21 bbrto21 merged commit 45cc5b5 into flutter-tizen:flutter-3.0.0-tizen Jul 11, 2022
swift-kim pushed a commit that referenced this pull request Aug 5, 2022
…Resize (#309)

* `TizenViewEventHandlerDelegate` only provides a limited interface for delegating event handling.
* `FlutterDesktopViewResize` should work on all types of views.
* Implementations of `TizenViewBase::SetGeometry` only manipulate top-level raw object such as window, container.
  other components in the view are manipulated on a resize callback.
* window channel use `SetGeometry` normally.
   
Signed-off-by: Boram Bae <boram21.bae@samsung.com>
swift-kim pushed a commit that referenced this pull request Sep 1, 2022
…Resize (#309)

* `TizenViewEventHandlerDelegate` only provides a limited interface for delegating event handling.
* `FlutterDesktopViewResize` should work on all types of views.
* Implementations of `TizenViewBase::SetGeometry` only manipulate top-level raw object such as window, container.
  other components in the view are manipulated on a resize callback.
* window channel use `SetGeometry` normally.
   
Signed-off-by: Boram Bae <boram21.bae@samsung.com>
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