Page MenuHomePhabricator

Efl.Ui.Calendar, elm_calendar: Code fixed to support auto repeat feature.
ClosedPublic

Authored by CHAN on Jul 17 2018, 10:47 PM.

Details

Summary
  1. Why there is a edje signal callback in elm_calendar? and do we need to maintain?

    We used edje part like a button before (3 years ago?), So there is a callbacks to get edje signal.

    Im pretty sure it is not use anymore. but we need to maintain backward compatibility.
  1. elm_calendar using using repeat feature in efl_ui_button for it. why did i change it to manual timer?

    We opend elm_calendar_interval_set() APIs. Support this API the manual timer is proper then using button's feature.
  1. why scroll freeze? and why only elm_calendar does it?

    When the user long press calendar button area and then move the scroll will be activated. it can prevent that weird action.

    efl_ui_calendar using button's feature. i not sure scroll freeze is deserve to attached in button side as a feature or not.

    So i will consider more for this case.
  1. Why efl_ui_calendar doesn't have year buttons (double spinner case)

    After interface work, we don't accept style change in the runtime. so that featrue will be supported as API.

    The year_button_set/get() property should be added. but i dont know is it really needed...

    If the app developer want use year inc/dec button for efl_ui_calendar. they can inherit the class and make it easily.
Test Plan

View, Action, API test in the elementary-test sample App.

Ps. The issue of the calendar2 crash when it closed. It's not relative with this commit. its focus. i will look around.

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.Jul 17 2018, 10:47 PM

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.Jul 17 2018, 10:47 PM
CHAN edited the summary of this revision. (Show Details)Jul 17 2018, 11:03 PM
CHAN edited the test plan for this revision. (Show Details)
CHAN added reviewers: Hermet, zmike, ManMower, segfaultxavi.
CHAN edited the summary of this revision. (Show Details)
CHAN added a reviewer: devilhorns.
CHAN edited the summary of this revision. (Show Details)Jul 17 2018, 11:10 PM
CHAN edited the summary of this revision. (Show Details)Jul 17 2018, 11:13 PM
zmike added a comment.Jul 18 2018, 7:23 AM

I don't really like this implementation. It seems like we're modifying the efl.ui.calendar widget to support bad choices made in elm_calendar. I don't think this is a reason to block the patch, but can we at least note (in the code) places where these kinds of decisions are being made so that we can clean things up later?

Thanks for all the detail provided in the commit log. Can you try to wrap your lines a bit? These seem unusually long...

@zmike, efl_ui_canlendar redesigned is ok unless it breaks any compatibility. I think he already mentioned summary here "fixed to support auto repeat feature." and more details in commit message.

CHAN edited the summary of this revision. (Show Details)Jul 18 2018, 9:38 PM
CHAN edited the summary of this revision. (Show Details)
CHAN edited the summary of this revision. (Show Details)
CHAN edited the summary of this revision. (Show Details)
CHAN edited the summary of this revision. (Show Details)Jul 18 2018, 9:45 PM
CHAN edited the summary of this revision. (Show Details)
CHAN edited the summary of this revision. (Show Details)
CHAN added a comment.EditedJul 18 2018, 9:53 PM

A description of the this features for Legacy support is given earlier in elm calendar.c.

zmike added a comment.Jul 19 2018, 5:58 AM

@zmike, efl_ui_canlendar redesigned is ok unless it breaks any compatibility. I think he already mentioned summary here "fixed to support auto repeat feature." and more details in commit message.

Right, as I said the thing I don't like is that we have to continue supporting this kind of compatibility. I think we should begin making either TODO tickets or comments in the code for cases like this so that the extra compatibility codepaths can eventually be removed.

Hermet accepted this revision.Jul 22 2018, 11:48 PM
In D6626#113746, @zmike wrote:

@zmike, efl_ui_canlendar redesigned is ok unless it breaks any compatibility. I think he already mentioned summary here "fixed to support auto repeat feature." and more details in commit message.

Right, as I said the thing I don't like is that we have to continue supporting this kind of compatibility. I think we should begin making either TODO tickets or comments in the code for cases like this so that the extra compatibility codepaths can eventually be removed.

It's not obvious the decision which one (continuing supporting compatibility or not) is better by contributors, right moment. Things better than before. TODO tickets is none of contributors' mandatory. but it should've been done when re-redesigned this calendar interface. Means, that is the actual maintainer of calender's job(who will improve it by himself) now or later.

This revision is now accepted and ready to land.Jul 22 2018, 11:48 PM
Closed by commit rEFL62a09f69a111: Efl.Ui.Calendar, elm_calendar: Code fixed to support auto repeat feature. (authored by Woochan Lee <wc0917.lee@samsung.com>, committed by Hermet). · Explain WhyJul 22 2018, 11:49 PM
This revision was automatically updated to reflect the committed changes.