Page MenuHomePhabricator

elm: add unfocus / focus signals to gen* and toolbar
ClosedPublic

Authored by bu5hm4n on Sep 23 2018, 8:10 AM.

Diff Detail

Repository
rEFL core/efl
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
bu5hm4n created this revision.Sep 23 2018, 8:10 AM
bu5hm4n requested review of this revision.Sep 23 2018, 8:10 AM
bu5hm4n updated this revision to Diff 16938.Sep 24 2018, 3:26 AM
bu5hm4n edited the summary of this revision. (Show Details)
YOhoho requested changes to this revision.EditedOct 21 2018, 7:54 PM

genlist emit "focused" "unfocused" signals twice.

You said..
Q. Which widgets need to call legacy_efl_ui_focus_manager_widget_legacy_signals?
A. Those where the canvas focus is not set to the resize_object, and that are a manager object.

But toolbar, genlist, gengrid are SET canvas focus to the resize_object. because they inherit Efl.Ui.Layout.Object. see, _efl_ui_layout_object_efl_ui_focus_object_on_focus_update
(canvas focus means evas_object_focus_set. isn't it?)

This revision now requires changes to proceed.Oct 21 2018, 7:54 PM
bu5hm4n added a comment.EditedOct 21 2018, 11:37 PM

genlist emit "focused" "unfocused" signals twice.

You said..
Q. Which widgets need to call legacy_efl_ui_focus_manager_widget_legacy_signals?
A. Those where the canvas focus is not set to the resize_object, and that are a manager object.

But toolbar, genlist, gengrid are SET canvas focus to the resize_object. because they inherit Efl.Ui.Layout.Object. see, _efl_ui_layout_object_efl_ui_focus_object_on_focus_update
(canvas focus means evas_object_focus_set. isn't it?)

No. This function has a check which checks if they can have focus, if not they don't set canvas focus to it. Also - isn't the _content_ of those windows getting focus ? :)

bu5hm4n updated this revision to Diff 17131.Oct 22 2018, 12:29 AM
bu5hm4n edited the summary of this revision. (Show Details)

hmm,,
Logical node can't get focus from focus manager, so they don't set canvas focus. then, they can't get focus when they don't have focusable content. is it proper understand..?

bu5hm4n added a comment.EditedOct 22 2018, 1:33 AM

hmm,,
Logical node can't get focus from focus manager, so they don't set canvas focus. then, they can't get focus when they don't have focusable content. is it proper understand..?

No - because if there is no focusable content a dummy element is injected. (Efl.Ui.Focus.Manager_Root_Focus).

<Edit> Sorry there was something in the top of the comment that was wrong :)

genlist emit "focused" "unfocused" signals twice.

You said..
Q. Which widgets need to call legacy_efl_ui_focus_manager_widget_legacy_signals?
A. Those where the canvas focus is not set to the resize_object, and that are a manager object.

But toolbar, genlist, gengrid are SET canvas focus to the resize_object. because they inherit Efl.Ui.Layout.Object. see, _efl_ui_layout_object_efl_ui_focus_object_on_focus_update
(canvas focus means evas_object_focus_set. isn't it?)

No. This function has a check which checks if they can have focus, if not they don't set canvas focus to it. Also - isn't the _content_ of those windows getting focus ? :)

It is odd.. genlist set canvas focus to resize_obj. could you check this?
when genlist get focus, efl_ui_focus_layout.c:495 is called (evas_object_focus_set(wd->resize_obj, EINA_TRUE);)

hmm,,
Logical node can't get focus from focus manager, so they don't set canvas focus. then, they can't get focus when they don't have focusable content. is it proper understand..?

No - because if there is no focusable content a dummy element is injected. (Efl.Ui.Focus.Manager_Root_Focus).

<Edit> Sorry there was something in the top of the comment that was wrong :)

But It seems that dummy node is not included in focus candidate if the widget inherit Elm.Interface_Scrollable.

@YOhoho but in gengrid/genlist the canvas focus moves on to the item OR the item content - the goal of the widget is NOT to have the canvas focus on the resize object. (The property setting that you are seeing is going to be overwritten soon after that, canvas-focus will move on)

The overall goal (focus wise) can be clasified in three things:
i) Focus is meant to be on the resize object. (button for example)
ii) Focus is meant to be on the children of the object, and the object is not a manager object. (Fileselector for example)
iii) Focus is meant to be on the children of the object, and the obejct is a manager object. (Gengrid Genlist)

Further more - this revision is for just getting the signals running again and fixing those issues, further issues or questions can be discussed later on or somewhere else.
Don't get me wrong - i like to answer questions, but this revision here is now being up for review very long. Fixing more focus bugs is very painfull this way, as i would like to have those things merged before... so i always have to drag arround a giant patch set. ...

Sorry about my annoying questions. i want to make sure this solution can be applied to other widgets.

I'm wondering why we need duplicated code if the classification can cover all of widgets.
elm_list also need legacy_efl_ui_focus_manager_widget_legacy_signals?

Sorry about my annoying questions. i want to make sure this solution can be applied to other widgets.

This is all good :) Write as many questions as you want - i just feel like those revisions are a bit the wrong place for it :)

I'm wondering why we need duplicated code if the classification can cover all of widgets.

What do you mean with this ?

elm_list also need legacy_efl_ui_focus_manager_widget_legacy_signals?

yep. Its possible that a few slipped through.

I'm wondering why we need duplicated code if the classification can cover all of widgets.

What do you mean with this ?

Oh, i guessed that the purpose of overall goal of classification in three thing is for extensibility, flexibility.
When i make new widget, if the widget belongs among three thing, i will be able to easily implement focus.

Right now those are just legacy flags - in new widgets you likely don't want to have those legacy things at all.
So new widgets don't fall into this at all.For new widgets we should have a discussion on how we can achive something simular. But thats a other topic, unrelated to this here in the beginning :)

YOhoho accepted this revision.Oct 22 2018, 4:14 PM

I'd like to make the code more flexible and extensible. however this patch is acceptable if we don't need to consider that seriously for legacy widgets.

This revision is now accepted and ready to land.Oct 22 2018, 4:14 PM
bu5hm4n updated this revision to Diff 17226.Nov 2 2018, 2:47 AM
bu5hm4n edited the summary of this revision. (Show Details)

...

Closed by commit rEFLc5add3ef4061: elm: add unfocus / focus signals to gen* and toolbar (authored by Marcel Hollerbach <mail@marcel-hollerbach.de>). · Explain WhyNov 13 2018, 8:08 AM
This revision was automatically updated to reflect the committed changes.