Page MenuHomePhabricator

efl_ui: move clickable from efl to efl_ui
ClosedPublic

Authored by bu5hm4n on May 5 2019, 3:00 AM.

Details

Summary

efl_ui_clickable is now a mixin. The mixin now brings two APIs the press
and unpress API can be used to tell the implementation the state of the
presses. Within the implementation the calls to press / unpress are then
converted to longpress / clicked events.

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.May 5 2019, 3:00 AM
bu5hm4n requested review of this revision.May 5 2019, 3:00 AM
segfaultxavi requested changes to this revision.EditedMay 6 2019, 2:44 AM

I see the changes to Efl.Ui.Button are in the next commit.

src/lib/elementary/efl_ui_clickable.c
50

I'd feel safer if you check first if state->timer is not NULL. Users of this mixin might call you twice without calling _unpress...

51

This interval (1 second) is hardcoded here? shouldn't it be #defined at the top of the file, or be a configurable value?

68

Again, I do not like hardcoded values hidden in the code :)

75

I'd feel safer if you checked first that this is not NULL, in case the user called _unpress twice.

src/lib/elementary/efl_ui_clickable.eo
2

Event structures should be suffixed _Event.

25

Docs for this event? I do not see the difference with clicked.

src/lib/elementary/efl_ui_image_zoomable.c
864–869

Why don't you use the new _press and _unpress method from the mixin instead of manually emitting the new event?

This revision now requires changes to proceed.May 6 2019, 2:44 AM
bu5hm4n planned changes to this revision.May 6 2019, 6:57 AM
bu5hm4n added inline comments.
src/lib/elementary/efl_ui_clickable.eo
2

When did we start with this? The API we have released last release is full of none Event suffixed Types.

src/lib/elementary/efl_ui_image_zoomable.c
864–869

This code should be removed again in the efl_ui_image commit. This is just a temp solution.

segfaultxavi added inline comments.May 6 2019, 7:00 AM
src/lib/elementary/efl_ui_clickable.eo
2

You are right, we have released event structs without _Event and I now think that's bad.

bu5hm4n updated this revision to Diff 21955.May 6 2019, 7:41 AM
bu5hm4n edited the summary of this revision. (Show Details)

Rebase & update according to xavi review

segfaultxavi requested changes to this revision.May 6 2019, 8:36 AM

All prior concerns addressed. Just one minor thing (please do not rebase and resubmit the whole patchset!)
I'll take care of polishing the docs (already in the list in T7717).

src/lib/elementary/efl_ui_clickable.c
24

A comment explaining what are these defines, including the units? Pleeeez?

This revision now requires changes to proceed.May 6 2019, 8:36 AM

Rebase.

src/lib/elementary/efl_ui_clickable.c
24

We measure *everything* in efl in seconds ... + this is a double. i don't think anyone will measure ms in doubles ...

bu5hm4n planned changes to this revision.May 6 2019, 8:45 AM

Rebas²

bu5hm4n updated this revision to Diff 22102.May 12 2019, 2:17 AM

Rebase & update

So, it is now commented that it is seconds. Additionally, there is now a info printing on non-legacy widgets. Which is mega usefull for debugging.

zmike accepted this revision.May 13 2019, 11:35 AM
zmike requested changes to this revision.May 13 2019, 12:02 PM
zmike added inline comments.
src/lib/elementary/efl_ui_clickable.c
58

This should also add a mouse-out event on the object and terminate the longpress timer if it is reached.

71

Unpress events can occur on objects which weren't originally pressed, this check should be removed.

93

This event should only occur if the object was previously pressed.

src/lib/elementary/efl_ui_clickable.eo
33

The descriptions for these should be more explicit like "when an object is pressed and then unpressed by the same button".

This revision now requires changes to proceed.May 13 2019, 12:02 PM
segfaultxavi accepted this revision.May 13 2019, 12:59 PM

No further concerns.

The Overall issue here is that only the text widget / button / panes was protected against the "Press->Mouse-out-of-object->Unpress" scenario. The others will do weird things, and for example emit the longpress event, for no reason. I will solve this with adding a abort method to the clickable mixin. This can then be called when the mouse leaves a object. This will reset the internal state. I don't want to solve it with a event handler in the mixin because of two reasons:

  1. Only works for non-theme based listening
  2. Clickable does not use events at all.
src/lib/elementary/efl_ui_clickable.c
58

Mhm i am not so sure if i want to solve it that way, right now the clickable implementation only contains the state. No event handling code or so. I think i will keep it that way, and add another function to it. Which must be called when the longpress stuff etc. should be canceled

71

True.

93

True.

bu5hm4n updated this revision to Diff 22148.May 14 2019, 1:54 AM
bu5hm4n edited the summary of this revision. (Show Details)

Make press,move-out-of-widget,unpress scenarios work

bu5hm4n updated this revision to Diff 22153.May 14 2019, 1:55 AM

Make press,move-out-of-widget,unpress scenarios work take 2

bu5hm4n marked 15 inline comments as done.May 14 2019, 2:00 AM

So, with the latest update, most widgets do work again correctly.
So when a mouse down is followed by the mouse going out of the element, then press and unpress will be emitted (from edje) but no clicked event is emitted from the implementation. Which seems correct.

Additionally, this here now also brings to more class functions. Which can be used to wire up the events from the theme to the object, or from a input object to the object. The input object right now has some issues, but i want to first speak about this approache and the problems i am having. I don't know if i just hit a evas bug, or if i made a general mistake.

bu5hm4n updated this revision to Diff 22169.May 14 2019, 8:00 AM

enhance docs, and update util

segfaultxavi resigned from this revision.May 14 2019, 8:42 AM

No further concerns regarding docs or API, but code should be carefully reviewed.

zmike requested changes to this revision.May 14 2019, 9:39 AM

Looks better, some minor changes still.

src/lib/elementary/efl_ui_clickable.eo
31

This should be renamed button_state_reset or similar. abort is a function call which stops program execution, and we don't want any confusion related to that function.

src/lib/elementary/efl_ui_clickable_util.eo
4

I think this should use bind instead of link.

Also this should state explicitly which signals are being handled.

14

Same as above.

This revision now requires changes to proceed.May 14 2019, 9:39 AM
bu5hm4n updated this revision to Diff 22173.May 14 2019, 10:53 AM

update to new API names

zmike requested changes to this revision.May 14 2019, 10:56 AM
zmike added inline comments.
src/lib/elementary/efl_ui_clickable_util.eo
14

Shouldn't this be bind_to_object? How does this even build?

This revision now requires changes to proceed.May 14 2019, 10:56 AM
bu5hm4n updated this revision to Diff 22184.May 14 2019, 11:21 AM

update to new API names, redo

This revision is now accepted and ready to land.May 14 2019, 12:36 PM
Closed by commit rEFL64923b8db15f: efl_ui: move clickable from efl to efl_ui (authored by Marcel Hollerbach <mail@marcel-hollerbach.de>, committed by zmike). · Explain WhyMay 14 2019, 1:18 PM
This revision was automatically updated to reflect the committed changes.