Page MenuHomePhabricator

elm: bring back elm/uiclock
ClosedPublic

Authored by segfaultxavi on Jul 12 2018, 5:01 AM.

Details

Summary

It turns out elm/uiclock (which was removed in 89675c3219) is actually used,
at least by the datetime legacy widget. Removing this widget broke the
datetime_example test.
This commit reverts 89675c3219 and fixes the elm/uiclock part names:

  • Part names are prefixed with 'elm.'
  • efl_ui_clock.c (which is used for both the new efl and the legacy elm widgets) now looks for part names with 'efl.' and 'elm.' prefixes, and without any prefix, for compatibility with older themes.

Fixes T6928

Test Plan

the Datetime elementary_test (and all other clock-related tests) now work.

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.
segfaultxavi created this revision.Jul 12 2018, 5:01 AM
segfaultxavi requested review of this revision.Jul 12 2018, 5:01 AM
zmike added a comment.Jul 12 2018, 6:22 AM

Hm I'm not sure about this one. Shouldn't the datetime widget just use the eo widget internally?

I provided another diff, D6580, which uses only the new efl_ui_clock and completely removes the legacy version efl_ui_clock_legacy.

However, I am unsure of the consequences, so I leave both diffs available until we decide (only one should be applied).

jsuya accepted this revision.EditedJul 13 2018, 8:47 AM

HI :)
As far as I know, efl_ui_clock is a combination of elm_clock and elm_datetime.
I think it's okay to use this patch.
I think If there is a problem with datetime and clock, it is better to merge this patch and create a new patch rather than remaining a broken state.

This revision is now accepted and ready to land.Jul 13 2018, 8:47 AM

Hi @jsuya,

Have you tried the other patch (D6580)? It also fixes the problem with the datetime widget, but removes completely elm_ui_clock and efl_ui_clock_legacy and internally it only uses the new efl_ui_clock.

What do you think, @zmike?

zmike added a comment.Jul 13 2018, 9:09 AM

I would prefer that we not use this patch, as there seems to be no point in keeping an extra file for a legacy widget which can't be used...

CHAN added a comment.EditedJul 16 2018, 3:44 AM

@segfaultxavi Thank you for ur commit.

Even if we already broken backwad compatibility when merged two widget for uiclock.

We need to support previous datetime...

If we dont have "_part_name_snprintf" function. We can't maintain backward compatibility.

Please choose this way. instead of D6580

anyways, "elm" prefix is needed in elm/uiclock.edc? We will not recommend using legacy themes anymore, and past themes are wrote without prefix...

ps. I would made a commit to breaking backwards compatibility when loading theme names with uiclock. if i think its really need on upstream.

CHAN accepted this revision.Jul 18 2018, 9:43 PM

Looks good.

Hermet added a subscriber: Hermet.Jul 18 2018, 11:58 PM

@zmike, what do u mean by "no point in keeping an extra file for a legacy widget which can't be used..." ? extra file means uiclock.edc?

Yes, I do not think elm/uiclock.edc is needed, as the legacy version is elm/clock and the new one is efl/uiclock. If we remove that file, that is one less file to maintain.

Can you explain with a bit more detail what you don't like of D6580? Does it build correctly? Does it run correctly?

zmike requested changes to this revision.Jul 19 2018, 5:54 AM

Unless an example is provided for how this breaks backwards compatibility on released, stable API then I don't think this patch should ever be merged.

This revision now requires changes to proceed.Jul 19 2018, 5:54 AM
CHAN added a subscriber: Jaehyun.Jul 25 2018, 2:42 AM
This comment was removed by CHAN.
CHAN added a comment.EditedJul 25 2018, 3:05 AM

@segfaultxavi @zmike @Hermet @jsuya @Jaehyun_Cho

Hello.

this kinda code has been missing. (Based on the results discussed here, I will decide whether to add the following code.)

if (elm_widget_is_legacy(obj)) elm_widget_theme_klass_set(obj, "datetime");
else elm_widget_theme_klass_set(obj, "uiclock");

I'm against keeping backwards compatibility files, but in this case, we dont have other way to support it.

Now we maintain the elm_datetime API,

and in order to support past edc we need to keep the part name the way of same as this patch.

If some of the apps that customize the datetime theme among released apps will not show up on the upgraded EFL :(

To break the association between clock and datetime, we should regenerate elm_datetime to maintain legacy.

In fact, there is a difference between elm_datetime and efl_ui_clock usability,

because the internal timer increases the clock time. We would consider adding elm_datetime to maintain compatibility, and clocks going into separate forms.

what do you guys think?

  1. Dont care, there is not much customized theme. Just break backward compatibility
  1. Add codes for maintain backward compatibility. like this patch.
  1. Separate up with efl_ui_clock and elm_datetime.

For me? 3. It looks clear and as i mentioned it has diff to usebility. and good to maintain as well.

Thank you.

zmike added a comment.Aug 13 2018, 4:02 AM

Oops I meant to reply on this some time ago but I am too busy and I forgot.

Based on your description, it seems we must continue to provide these edje groups to avoid breaking legacy. I'm not happy about this, but there is no alternative.

zmike accepted this revision.Aug 13 2018, 4:03 AM
This revision is now accepted and ready to land.Aug 13 2018, 4:03 AM
This revision was automatically updated to reflect the committed changes.