Page MenuHomePhabricator

efl_ui_format: remove supporting default TM format in interface.
ClosedPublic

Authored by CHAN on Aug 20 2018, 3:27 AM.

Details

Summary

efl_ui_format printed ERR log in calendar use case.

calendar only accept format as "B,b,h,m,y,Y"

But it doesn't cover that and not supporting TM type.

If there is other widget which one using format interface, It also has own accpeted format.

So i think it should impelment on widget side.

Test Plan

elementary_test -> efl_ui_calendar.

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.
CHAN created this revision.Aug 20 2018, 3:27 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/

CHAN requested review of this revision.Aug 20 2018, 3:27 AM
CHAN edited the summary of this revision. (Show Details)Aug 20 2018, 3:32 AM
CHAN edited the test plan for this revision. (Show Details)
CHAN added reviewers: Hermet, singh.amitesh, Jaehyun.
zmike requested changes to this revision.Aug 20 2018, 9:51 AM

I don't think removing the default implementation of this is a good idea. If calendar has an issue here then this should be resolved in calendar alone. Interfaces API should not be changed like this to work around inconsistencies in specific widgets...

This revision now requires changes to proceed.Aug 20 2018, 9:51 AM
CHAN added a comment.EditedAug 21 2018, 12:20 AM

@zmike Hello. Thank you for giving advice here.

But i think, this is not workaround code.

To properly support the datetime format, we need to validate all date-time formats in the mixin that implements the default behavior of the interface.

For the calendar, the API doc said that only the month, year format is allowed.
In the current implementation, however, ERR logs are printed regardless of the datetime format.

Additionally, using the Format interface widgets to use a Datetime format also would allow only a specific format can not be all covered.

The enum for the TM is not defined, it is not considered in the initial design, but the implementation is included in the Mixin.

The TM case seems to be the correct implementation to override in each widget like this way.

@segfaultxavi do we have plans to do anything related to datetime format strings in interfaces?

Apologies for the delay, I had to read through the code to understand what is going on.

There is no plan for format strings in the interfaces that I am aware of, we'll take our own decisions...

I also think like @CHAN, the implementation for Efl.Ui.Format is suspicious: First _format_string_check() rejects the %b string and sets the internal type to FORMAT_TYPE_INVALID. But then, _default_format_cb() accepts the Eina.Value parameter with EINA_VALUE_TYPE_TM, before even looking at the internal type.

So, as long as the Eina.Value used has type EINA_VALUE_TYPE_TM, it does not matter what format string you used. This is all very convoluted and misleading, at best.

I see two options:

  1. Accept the proposed diff: Efl.Ui.Format only handles "simple" types (integers, floats, strings, ...) and widgets requiring more advanced formatting do it themselves. CAUTION:: Please, @CHAN, make sure that no other widget is using the EINA_VALUE_TYPE_TM shortcut.
  2. Add the complex formatting to Efl.Ui.Format, so it starts accepting %b, %B, etc.

If Efl.Ui.Calendar is the only one requiring the advanced formatting, I prefer option 1.

Efl.Ui.Clock does not seem to use the Efl.Ui.Format mixin at all. It implements all formatting on its own.

CHAN added a comment.Sep 12 2018, 11:23 PM

@segfaultxavi Thank you for reviewing this.

Yes, there is only calendar using TM formats for the format string so far.

Let's take this way.

@zmike Could you please push this commit?

segfaultxavi accepted this revision.Nov 20 2018, 12:57 AM

@zmike won't be available for a while, but I think his concerns have been addressed. Let's proceed with this.

This revision was not accepted when it landed; it landed in state Needs Revision.Nov 20 2018, 1:23 AM
Closed by commit rEFLa1478261b5df: efl_ui_format: remove supporting default TM format in interface. (authored by Woochanlee <wc0917.lee@samsung.com>, committed by segfaultxavi). · Explain Why
This revision was automatically updated to reflect the committed changes.