Page MenuHomePhabricator

efl_ui_spin, efl_ui_spin_button: Add new spin, spin_button widgets.
ClosedPublic

Authored by CHAN on Nov 3 2017, 1:54 AM.

Details

Summary

https://phab.enlightenment.org/T5900

Creating base class(efl_ui_spin) to support various shape of spinner.

Added button interaction widget efl_ui_spin_button inherited from efl_ui_spin.

Test Plan

Add tests in elementary_test.

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.
There are a very large number of changes, so older changes are hidden. Show Older Changes
jpeg added a comment.Nov 14 2017, 6:01 PM
In D5424#93377, @CHAN wrote:

@Jaehyun_Cho Thanks for ur review.

I changed name the 'wrap' to 'loop'.
I agree with you the word 'wrap' is using for entry text line action.

OK

@jpeg Thanks for reviewing.

I think so. this commit need to seperate up.
How about seperate this commit after you guys reviewing finised?
It's better way to get reviewing and keeping comments.

Yes, I assume you have a dev branch with multiple patches. We will merge the branch, not this Diff.
This Diff is for review comments only.

Basic spin has basic action of change value.
I understand the test app looks weird. Um.. i will add proper sample for it.
But If user want to create spinner without indicators. it will helps. (wearable UX using this basic spin)
And it's expandable. it can support various type of UX.

Even in wearable UX there are way to modify the spinner value. It could be the rotary (key events) or finger scrolling (thumbscroll). On desktop the mouse wheel should react.
But I understand your point now :)

Jaehyun_Cho added inline comments.Nov 14 2017, 6:57 PM
src/lib/elementary/efl_ui_spin.c
11

this is not required.

16

this is not required.

src/lib/elementary/efl_ui_spin_button.c
12

this is not required.

@CHAN @jpeg

It appears that efl_ui_spin and efl_ui_spin_button should be separated from elm_spinner.
If so, it seems that elm_entry should be replaced with efl_ui_text.

What do you think?

jpeg added a comment.Nov 14 2017, 10:30 PM

@CHAN @jpeg

It appears that efl_ui_spin and efl_ui_spin_button should be separated from elm_spinner.
If so, it seems that elm_entry should be replaced with efl_ui_text.

What do you think?

Agreed. But it's ok if this goes in a later patch. No problem.
I'll keep reviewing this today.

src/bin/elementary/test_flipselector.c
192

nevermind then :) let's focus on spinner for now :)

jpeg requested changes to this revision.Nov 14 2017, 11:20 PM

Much better! Only 2 problems:

  1. There is some confusion about the format string/cb API. There should be either a format string (default is "%.0f"), an API can be used to change it (then we use the validity filter, verify that there's only on %i, %d, %f, ... and use it with sprintf. OR we use the format_cb.

I think @singh.amitesh implemented format API as a mixin so that the format_string support does not need to be implemented. But this widget makes extra checks on the value type and range, so I'm not sure if the generic implementation can be used (I think it should work fine).

  1. Smart / legacy things are not required here.

I think Efl.Ui.Spin should react to mouse wheel.
Is there a plan for a "scrollable" spin?

I think changing elm_entry to efl.ui.text is too much work for now. But it is necessary to do later (soon).

src/bin/elementary/test_ui_spin.c
72

not necessary anymore :) eo objects are visible by default
but it may be better to set size here (instead of inside efl_add)

src/lib/efl/interfaces/efl_ui_range.eo
64

So here we have a conceptual problem: default range is min=0, max=1, and step=1, which means only 2 possible values by default.
maybe we just say in the documentation that the default range min/max depends on the widgets?

src/lib/elementary/efl_ui_spin.c
35

no smart callbacks for eo api

118

this code is perfect. how come the format_cb_set() function code was wrong? :)

120

isn't there a format_string api?
it's ok if it's not implemented yet but I thought there was one

237

this is not how the format api should work. the callback should be called everytime the value changes, the returned string is not a format string!

243–244

no this code path does not make sense. format_cb returns the string to display, not a format string

247

same here, invalid logic

257

the values should be checked. if max < min then you should print an ERR message, and invert the values, so that min <= max, always
also you need to use EINA_DBL_EQ here instead of == (to compare double values)

278

what if this is 0? or <0?

295

use EINA_DBL_EQ or EINA_FLT_EQ instead of ==

src/lib/elementary/efl_ui_spin.eo
5

This needs doc :) What is a spin

17

changed by api or changed by user? or both?

src/lib/elementary/efl_ui_spin_button.c
22

this should probably be a elm_config value. eventually. (not in this patch)

44

why do we have smart callbacks? that's just legacy

81

with %d the mode should be changed to INT here
why not support %i, %x and others, then?

99

i think the above logic and the safety code above should be implemented in the generic mixin (efl.ui.format)

770

exact. please remove smart callbacks

src/lib/elementary/efl_ui_spin_button.eo
64

"it's enabled by default"
"editable: bool(false)'"
one of those two is incorrect :)

This revision now requires changes to proceed.Nov 14 2017, 11:20 PM
CHAN marked 12 inline comments as done.Nov 16 2017, 5:04 AM
CHAN added a subscriber: id213sin.

@Jaehyun_Cho thank you for reviewing.

I agree with that. But now efl_ui_text doesn't have filter feature. :( i reported it already to @id213sin.
Hope he will add that feature soon.

And i fixed code as ur comments.

@jpeg thank you for reviewing.

If user sets format_string without format(%d, %f,...) spin is not works.
It's users fault i think. i think current implement work fine.

and if user not sets format_string. we will using default(%0.f).

You said i have to call foramt_set_cb every value changed and when i get a format_string it's not format string.
I ithink i missed something about it? If i got wrong please tell me.

And i fixed code as ur comments.

src/lib/efl/interfaces/efl_ui_range.eo
64

Nice catch. the doc has written in case for slider.

By default, min is equal to 0.0, and max is equal to 1.0.
(max is equal to 100.0 in spin classes)

I added above line for it.

src/lib/elementary/efl_ui_spin.c
118

efl_ui_spin_button need the format string as well.
So i keep the buf string in 'sd->templates' and use it in efl_ui_spin_button class.

120

This else state is for when user did not set a format string.
Um...now I ref other widgets which implemented format interface.
i dont see calling format string in else state.

could you please tell me more detaily about it?

237

Spin need to know what format set from user.

243–244

If the format is not have "%d or %f ~~" current internal logic not works.

257

good opinion.

278

nice catch.

src/lib/elementary/efl_ui_spin.eo
17

Both is correct i think.
Whenever the spin value changed it nees to call.

src/lib/elementary/efl_ui_spin_button.c
81

type of the spin is already set in spin class.

others... It's been missed for a long time : ( thanks.

99

Could you tell me more detaily?

If user set efl_ui_format_string_set(spin, "test %f");
getting "%f" string from format_string action should be implemnted in efl_ui_format?

src/lib/elementary/efl_ui_spin_button.eo
64

nice catch!

CHAN updated this revision to Diff 13044.Nov 16 2017, 5:09 AM
CHAN marked 4 inline comments as done.

Fixed code as comments.

Jaehyun_Cho requested changes to this revision.Nov 16 2017, 9:42 PM

Here are some comments from me and from jpeg's previous comments.

src/lib/efl/interfaces/efl_ui_range.eo
32

I think that mentioning spin class here is not appropriate.

How about this? (The minimum and maximum values are different by classes.)

54

I think that more comment can be added as jpeg commented.

The step value should not be below 0.

src/lib/elementary/efl_ui_spin.c
267

In this case, the given step should not be set and just return this function.

290

you need to use EINA_DBL_EQ(sd->val, sd->val_min) instead of "==" to avoid floating point comparison errors.

292

you need to use EINA_DBL_EQ(sd->val, sd->val_max) instead of "==" to avoid floating point comparison errors.

src/lib/elementary/efl_ui_spin_button.eo
2

As jpeg's comments, please replace the class name to Efl.Ui.Spin.Button.

This revision now requires changes to proceed.Nov 16 2017, 9:42 PM
CHAN updated this revision to Diff 13053.Nov 17 2017, 2:06 AM
CHAN marked 2 inline comments as done.

Code clean up.

Jaehyun_Cho requested changes to this revision.Nov 19 2017, 11:29 PM
Jaehyun_Cho added inline comments.
src/lib/efl/interfaces/efl_ui_range.eo
56

type is here. "must e"
And I think it would be better "should be" instead of "must be" like the actuall error message in efl_ui_spin.c

This revision now requires changes to proceed.Nov 19 2017, 11:29 PM
CHAN updated this revision to Diff 13081.Nov 20 2017, 12:02 AM

Fix typo.

@jpeg

This patch looks good to me. If you do not have any comment, then I can rebase this patch to submit to master branch.

Jaehyun_Cho accepted this revision.Nov 20 2017, 3:56 PM
jpeg added a comment.Nov 20 2017, 5:34 PM

make distcheck fails

  CC       lib/elementary/lib_elementary_libelementary_la-elm_sys_notify.lo
../../../src/lib/elementary/efl_ui_spin.c:12:10: fatal error: 'efl_ui_spin_private.h' file not found
#include "efl_ui_spin_private.h"
         ^~~~~~~~~~~~~~~~~~~~~~~
1 error generated.
make[5]: *** [Makefile:35745: lib/elementary/lib_elementary_libelementary_la-efl_ui_spin.lo] Error 1
make[5]: *** Waiting for unfinished jobs....
../../../src/lib/elementary/efl_ui_spin_button.c:13:10: fatal error: 'efl_ui_spin_button_private.h' file not found
#include "efl_ui_spin_button_private.h"
         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.
jpeg added a comment.Nov 20 2017, 5:38 PM

The default spin (w/o buttons) doesn't implement any keyboard/mouse wheel handling. I think it should. I'll get to the rest of the review now.

jpeg added a comment.Nov 20 2017, 6:40 PM

Looks mostly OK. Rebase, fix distcheck, and we should be able to merge this.
Further work will be required, still:

  • move theme to the proper location. see D5473
  • add proper bg part to the theme. see D5151
  • implement mouse wheel and keyboard support for base spinner. it should be possible to interact with it. maybe this will need the activate feature (for keyboard support), see D4933
src/bin/elementary/test_ui_spin.c
26

this needs an explicit cast

src/lib/efl/interfaces/efl_ui_range.eo
62

step: double(1.0);

62

nevermind. maybe it's not 1 by default

I did rebase and fixed make distcheck error on devs/jaehyun/efl_spinner branch.

In D5424#94340, @jpeg wrote:

Looks mostly OK. Rebase, fix distcheck, and we should be able to merge this.
Further work will be required, still:

  • move theme to the proper location. see D5473
  • add proper bg part to the theme. see D5151
  • implement mouse wheel and keyboard support for base spinner. it should be possible to interact with it. maybe this will need the activate feature (for keyboard support), see D4933

@jpeg
Regarding D5473 and D5151, I guess the modification should be applied after those patches are submitted to master.
So unless those patches are going to be submitted to master before this patch, I think the modification should be applied later (not in this patch).

Should be this patch applied after D5473 and D5151?

rebase the commit

singh.amitesh requested changes to this revision.Nov 22 2017, 8:38 PM
singh.amitesh added inline comments.
src/lib/elementary/efl_ui_spin.c
26

This function will not yield correct result for following format strings.

%4$d
%$4f

Please check.

This revision now requires changes to proceed.Nov 22 2017, 8:38 PM
jpeg added a comment.EditedNov 23 2017, 8:29 PM

I think the format mixin should handle the type detection. And expose a (maybe internal) API to know it: char, int, float, double, etc...

jpeg added inline comments.Nov 23 2017, 8:31 PM
src/lib/elementary/efl_ui_spin.c
26

what are those formats???

CHAN added a comment.EditedNov 23 2017, 11:31 PM

@singh.amitesh Hello.

If user sets invalid format(%4$d %$4f etc...) for spin class.
Spin will print out err msg only.

And it may breaks internal logics.

I think that is totally users fault. but we can support some protection for that as well..

I think it's not blocking point this commit... Let's make a ticket or think it in to do list.

thanks very much.

jpeg added a comment.Nov 23 2017, 11:48 PM

It shouldn't break internal logic, just print ERR and reset to default format string.

Closed by commit rEFLeefcb49419af: efl_ui_spin: Add new spin and spin_button widgets (authored by Woochan Lee <wc0917.lee@samsung.com>, committed by Jaehyun_Cho). · Explain WhyNov 27 2017, 2:55 AM
This revision was automatically updated to reflect the committed changes.

This patch has been submitted to master branch. (eefcb49419af9d0057ba4c03e6c9009a1265e31e)

@CHAN
Please abandon this patch and please consider the following tasks.

  1. Handle the invalid format case without internal logic break. (as @singh.amitesh and @jpeg said)
  2. All legacy APIs should be replaced with efl_ui APIs (e.g. elm_button_add)