Page MenuHomePhabricator
Create Task
Maniphest T7901

efl.ui.timepicker
Open, TODOPublic
Actions

Description

class Efl.Ui.Timepicker @beta
├ (P) time
├ (P) ampm
├ (E) changed
  • ampm -> is_24hour (WooHyun)
  • changed -> time,changed

Details

Differential Revisions
D10008: declare first round of classes stable
D9767: efl_ui: change *_min(max) to min(max)_*
Commits
D9905 / rEFL31d197b85be9: efl_ui_timepicker/datepicker : change event name properly
D9811 / rEFLd61cf9df7308: efl_ui_timepicker: change a property name (ampm -> is_24hour)

Related Objects
Search...

bu5hm4n created this task.May 3 2019, 12:05 PM
bu5hm4n triaged this task as TODO priority.
CHAN claimed this task.Jul 7 2019, 10:03 PM
Diffusion closed subtask T7919: enum Efl.Orient as Resolved.Jul 11 2019, 8:15 AM
Diffusion closed subtask T7562: efl.input.interface as Resolved.
woohyun added a comment.Mon, Aug 26, 1:18 AM

@bu5hm4n @segfaultxavi @zmike @CHAN

ampm -> 24hour ? How do you think about this ?

segfaultxavi added a comment.Mon, Aug 26, 5:04 AM

Yes, I like 24hour much better. Also, the parameter to the ampm property is already called 24hour :D

woohyun updated the task description. (Show Details)Wed, Aug 28, 12:59 AM

Follwing the T7869,

"time -> time_current" would be better ?

woohyun updated the task description. (Show Details)Wed, Aug 28, 1:00 AM
segfaultxavi added a comment.Wed, Aug 28, 1:32 AM

I think current_time sounds better. I'll propose it in T7869 too.

woohyun added a comment.Fri, Aug 30, 1:50 AM

@segfaultxavi

Following the comments in T7869, I'll not change to "current_time" :)
I'll just update 24hour, soon.

woohyun added a comment.EditedFri, Aug 30, 2:56 AM

@segfaultxavi

While trying to apply "24hour", I noticed that eolian does not allow to include <number> in a property name !!

eolian: /home/woohyun/EFL/efl/src/lib/elementary/efl_ui_timepicker.eo:26:17: '<value>' expected near '<number>'
       @property 24hour {
                 ^

eolian: could not parse file '../src/lib/elementary/efl_ui_timepicker.eo'
[10/2626] Linking target src/bin/edje/edje_cc.
ninja: build stopped: subcommand failed.

Do you have any idea on this ?

segfaultxavi added a comment.Fri, Aug 30, 3:13 AM

Ooooohhhh... that's interesting. I guess it makes sense, since C does not allow that either.

How about use24hour, use24h, format24hour or format24h ?

woohyun added a comment.Fri, Aug 30, 3:33 AM

Hmm. As you know well, I'm not a good person for naming something. :(

My recommendation -

  1. is_24hour
  2. just keep "ampm" as it is - and change the bool name from "is_24hour" to "ampm" - and make to use "true/false" oppositely.
segfaultxavi added a comment.Fri, Aug 30, 4:46 AM

Let's see what other people think. We have other properties named is_, so is_24hour seems good to me too.

zmike moved this task from Backlog to Evaluating on the efl: api board.Fri, Aug 30, 11:11 AM
zmike added a comment.Fri, Aug 30, 11:28 AM

I like is_24hour since it's very explicit.

woohyun added a comment.Sun, Sep 1, 11:05 PM

Ok. I'll go with is_24hour :)

woohyun updated the task description. (Show Details)Sun, Sep 1, 11:05 PM
woohyun added a comment.Thu, Sep 5, 2:06 AM

I think only one thing remained is "changed" which has the same discussion in T7869.

Or any other opinions on this task ?

woohyun updated the task description. (Show Details)Wed, Sep 11, 12:30 AM
segfaultxavi updated the task description. (Show Details)Thu, Sep 12, 1:34 AM

I think this class is good to go now?

zmike moved this task from Evaluating to Stabilized on the efl: api board.Fri, Sep 13, 11:32 AM