Page MenuHomePhabricator

Summary: code refactoring - get rid of unneccessary Efl.Access.Value interface.
ClosedPublic

Authored by mdenys on Feb 10 2020, 2:04 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.
mdenys created this revision.Feb 10 2020, 2:04 AM

It seems that this patch has no reviewers specified. If you are unsure who can review your patch, please check this wiki page and see if anyone can be added: https://phab.enlightenment.org/w/maintainers_reviewers/

mdenys requested review of this revision.Feb 10 2020, 2:04 AM
l.oleksak accepted this revision.Feb 11 2020, 1:20 AM

Verified successfully in elementary_test application, for both legacy and new widget's API.

This revision is now accepted and ready to land.Feb 11 2020, 1:20 AM
zmike requested changes to this revision.Feb 11 2020, 6:11 AM
zmike added a subscriber: zmike.

This patch needs to be rebased. It does not apply.

This revision now requires changes to proceed.Feb 11 2020, 6:11 AM
bu5hm4n requested changes to this revision.Feb 13 2020, 1:03 AM

This looks overall like a very good idea. We just have to remember that we still need to support atspi for legacy widgets (and thus also in the bridge).

src/lib/elementary/efl_ui_progressbar.eo
56

If you remove the implementation line here, you can also remove the actual implementation .c (same for the other .eo files)

src/lib/elementary/elm_atspi_bridge.c
2182

This change (and all the changes below have to support legacy as well as Efl.Ui.

The easiest would be to have :

if (elm_widget_legacy_is(obj))
  efl_access_value_and_text_get(obj, &value, NULL);
else
  value = efl_ui_range_value_get(obj);

Same applies to ELM_ATSPI_OBJ_CHECK_OR_RETURN_DBUS_ERROR.

2527–2530

This here is a little bit more complex, as we need to support legacy, we need to defer this if up to a point where we know for what kind of widget we are calling this, legacy or efl.ui.

3205

This needs to support both, as we still need support in legacy for EFL_ACCESS_VALUE_INTERFACE.

src/lib/elementary/elm_spinner_eo.c
333 ↗(On Diff #28921)

I think this change needs to be removed. Elm_Slider is a legacy widget, that needs to keep the ACCESS functionality.

This looks overall like a very good idea. We just have to remember that we still need to support atspi for legacy widgets (and thus also in the bridge).

OK, I understand that we cannot fully remove Efl.Access.Value interface, since elm widgets don't implement range_display interface, which had to replace Efl.Access.Value. But what should we do in case when the widget doesn't have legacy _as well as_ range_display? Shouldn't I add after

if (elm_widget_legacy_is(obj))

another if statement, like this:

else if(efl_isa(obj, EFL_UI_RANGE_DISPLAY_INTERFACE))

I think that cannot happen, the APIs below are guarded with calls, that *should* only allow objects to reach that point that are either Efl.Access.Value or Efl.Ui.Range_Display. However, if you hit a case where this is not true, then you can just check with 2 if's if its Efl.Access.Value or Efl.Ui.Range, if neither, you can just raise a error. That should be enought. :)

(Btw. i also answered that via mail, did my mail not arrive?)

mdenys marked 2 inline comments as done.Feb 20 2020, 5:08 AM

(Btw. i also answered that via mail, did my mail not arrive?)

Unfortunately, not.

mdenys updated this revision to Diff 29149.Feb 20 2020, 8:16 AM
mdenys marked 3 inline comments as done.
  • get rid of efl_access_value_changed_signal_emit function where it is possible
  • modified -> legacy support issue
mdenys updated this revision to Diff 29177.Feb 21 2020, 6:34 AM
  • rebasing onto origin/master
  • get rid of efl_access_value_changed_signal_emit function
  • modified - legacy support issue
  • legacy issue + atspi_bridge
  • corrected version of legacy commit

This patch needs to be rebased. It does not apply.

OK, I've rebased the patch.

This looks overall like a very good idea. We just have to remember that we still need to support atspi for legacy widgets (and thus also in the bridge).

I've modified the patch to include this remark.

mdenys updated this revision to Diff 29190.Feb 24 2020, 7:56 AM
  • comments -> deletions
bu5hm4n accepted this revision.Feb 24 2020, 8:17 AM

That looks good - thank you.

This revision was not accepted when it landed; it landed in state Needs Review.Feb 24 2020, 8:22 AM
Closed by commit rEFLad001028424f: Summary: code refactoring - get rid of unneccessary Efl.Access.Value interface. (authored by mdenys, committed by Marcel Hollerbach <mail@marcel-hollerbach.de>). · Explain Why
This revision was automatically updated to reflect the committed changes.