Page MenuHomePhabricator

Ecore: introduce Ecore_Device and apply to Ecore_Events
AbandonedPublic

Authored by jpeg on Apr 1 2016, 4:08 AM.

Details

Summary

add/delete Ecore_Device in ecore_drm
add/delete Evas_Device in ecore_input_evas

Diff Detail

Branch
devs/jpeg/work
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 2230
Build 2295: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
zmike added inline comments.Apr 4 2016, 9:02 AM
src/lib/ecore/Ecore_Common.h
3102

nit: Don't indent braces for struct/enum/union decls.

3108

What does "special" mean here?

3109

I think POINTER is misleading since the mouse cursor can also be called a pointer. Maybe something like WAND?

3123

HAND_SIZE -> HAND_SIDE

3125

If there's no other part of the pen specified here, does the name really need to be PEN_TIP ?

3143

All functions taking no parameters must be function_name(void);

3168

All these docs should be @since 1.18

3196

Functions which return stringshared strings should return the type Eina_Stringshare *

3230

Please ensure that all doxygen parameter names match the function's parameter names.

3263

It seems to me like class+subclass should be passed as constructor parameters; leaving these functions like this could result in someone misusing them and changing a device's type later on, breaking applications.

src/lib/ecore/ecore_device.c
18

Use Eina_Stringshare * for stringshared strings.

26

devices_num seems to serve no purpose and should probably be removed.

70

nit: No parens around dev->name.

182

nit: Newline before function name.

185

EFL functions, including eina_stringshare_del, do not require null checks for deletion.

src/lib/ecore_drm/Ecore_Drm.h
131

This type can't be changed. It's a public, released struct which users have already written code for as an unsigned int.

src/lib/ecore_drm/ecore_drm_device.c
284

unsigned ints cannot be negative.

507

These initializers will be overwritten immediately and will result in Coverity defect reports.

src/lib/ecore_drm/ecore_drm_evdev.c
294

Same as above.

299

This should be an EINA_SAFETY.

308

Use eina_streq to optimize comparison case of stringshared strings.

src/lib/ecore_drm/ecore_drm_inputs.c
149

Use eina_stringshare_ref for stringshared strings.

171

EINA_LIST_FOREACH handles NULL lists, this check is unnecessary.

178

eina_streq

247

Why use an intermediate variable? Just do if (function())

291

unsigned ints cannot be negative.

src/lib/ecore_input/Ecore_Input.h
59

Missing @since

89

Missing @since

141

Missing docs.

151

Use stringshare for stringshared strings.

216

This breaks ABI. New members of public structs must be added at the end of the struct.

238

Breaks ABI.

276

Breaks ABI.

303

Breaks ABI.

374

Breaks ABI.

src/lib/ecore_input_evas/ecore_input_evas.c
409

Don't initialize variables which are guaranteed to be initialized later.

427

eina_streq

472

Should probably be if (dev) pop()

624

Same as above.

677

Same as above.

736

Same as above.

771

Same as above.

This revision now requires changes to proceed.Apr 4 2016, 9:02 AM
ohduna marked 46 inline comments as done.Apr 5 2016, 6:31 PM
ohduna added inline comments.
src/lib/ecore/Ecore_Common.h
3108

In my opinion, this special pen means "digital pen", different from traditional stylus.
https://en.wikipedia.org/wiki/Digital_pen

Ecore_Device_Classes was named according to Evas_Device_Classes.
If you have any suggestion for Ecore_Device_Classes. Please share your ideas.

3109

I changed POINTER to WAND and changed them in Evas_Device_Class, too.

3123

I changed Evas_Device_Subclass_Hand_Size in Evas_Device_Class, too.

3125

In my opinion, this PEN_TIP subclass is for PEN class.
A digital pen has more features than a stylus, such as input buttons and mouse mode.
Some digital pens can detect the location of the tip during writing and we recognize those pens with PEN_TIP subclass.

3263

Please refer to Evas_Device's APIs setting/getting each member variables.
Ecore_Device would better maintain the consistence with Evas_Device.

src/lib/ecore_drm/Ecore_Drm.h
131

The window value is set as (Evas_Engine_Info_Drm *)einfo->info.buffer_id.
It could be zero if drmModeGetCrtc returns NULL in different kernels.

So, in order to know whether it was initialized value(-1) or valid value(0~) from drm, we need to change data type.

src/lib/ecore_drm/ecore_drm_device.c
284

The window value is initialized as negative value.
When enlightenment loads wl_drm module, it creates ecore_evas and set the window value as the bigger value than zero.

src/lib/ecore_drm/ecore_drm_inputs.c
291

_device_added() is called when

  1. devices already plugged in. (in init time)
  2. devices plugged in after enlightenment loaded. (after init time)

In init time, the window id is not valid. So, we defer adding Ecore_Devices in ecore_drm_device_window_set.
After init time, the window id is valid. we add Ecore_Deice in this function.

src/lib/ecore_input/Ecore_Input.h
141

This enum is unnecessary

ohduna updated this revision to Diff 8843.Apr 5 2016, 7:32 PM
ohduna edited edge metadata.
ohduna marked 9 inline comments as done.

revised according to zmike's comments.
I greatly appreciate your comments that helped improve this patch.

ohduna updated this revision to Diff 8844.Apr 5 2016, 7:46 PM
ohduna edited edge metadata.

rebase on top of upstream

ohduna updated this revision to Diff 8845.Apr 5 2016, 9:40 PM

restore previous diff I lost when rebasing..

ohduna updated this revision to Diff 8846.Apr 5 2016, 9:51 PM

I think this patch are completely updated.
Please let me know your opinion of this commit.

ohduna updated this revision to Diff 8850.Apr 6 2016, 2:58 AM

need to include <Ecore.h> in Ecore_Input.h

ohduna added a comment.May 3 2016, 1:18 AM

Hi,
It's been a month since I made this patch.
I just wonder if this patch needs to be rebased or changed or abandoned.
Or... would you recommend to split this patch into several patches(ecore_device/ecore_drm/ecore_input_evas)?
Thanks for your comments in advance. :)

zmike requested changes to this revision.May 16 2016, 10:11 AM
zmike edited edge metadata.

Sorry, I haven't been getting notifications about this for some reason. I'd like to get at least one more person to review the next revision before it gets merged since this is a moderately large patch.

src/lib/ecore/Ecore_Common.h
3108

Okay, so maybe call it DIGITAL_PEN then to avoid confusion?

src/lib/ecore_drm/Ecore_Drm.h
131

That doesn't matter, it's a defined type in a stable header. Changing this would create new compiler warnings for existing code since the expectation was to use an unsigned int.

If you need to determine whether the value has been set, add a boolean flag to the end of the struct for it.

src/lib/ecore_drm/ecore_drm_inputs.c
171

?

198

Might be worth breaking out this find loop into a helper function to cut down on code duplication.

200

Same as above.

This revision now requires changes to proceed.May 16 2016, 10:11 AM
jpeg added a comment.Jun 7 2016, 12:54 AM

While the intent of this patch is great, I believe we have both been working on a similar topic recently.
Ecore device should be an EO object. In fact, it should be Efl.Input.Device. Now, Evas can depend on Ecore which means we can move Efl.Input.Device to ecore and use that for Evas_Device and Ecore_Device (in other words, have only one type).

Unfortunately, I have no way right now to test this code (drm).
Could you have a look at Efl.Input.Device first?

ohduna added a comment.Jun 7 2016, 1:15 AM

Dear jpeg,
Good to hear that. I knew you have been working on EO-ifying devices and events. I will check out Efl.Input.Device first.
Thanks~

jpeg requested changes to this revision.Jul 4 2016, 10:06 PM
jpeg added a reviewer: jpeg.

Alright, so... this adds new features to a deprecated header: Ecore_Drm.h. There's an explicit #warning that says to use Ecore_Drm2.h instead.

Also, this duplicates evas device APIs in Ecore. This is mostly pointless. We can use the new (from 1.18) Efl_Input_Device class, which is defined in efl, not ecore or evas. Evas_Device is now defined as Efl_Input_Device and all evas_device_ APIs should be working transparently. No need for a whole new set of duplicates.

src/lib/ecore_drm/Ecore_Drm.h
131

@zmike is correct. It's not acceptable to change the type here,

1047

No documentation, invalid naming scheme (verb at the end). Deprecated header.

src/lib/ecore_drm/ecore_drm_device.c
284

Not acceptable. window is defined as an unsigned int and needs to stay this way. Add a bool if you really need to support window id 0 and make sure it's been set.

src/lib/ecore_drm/ecore_drm_inputs.c
291

You can not change the type. Same remark as above.

src/lib/ecore_input/Ecore_Input.h
141

still missing docs and @since

jpeg commandeered this revision.Jul 4 2016, 10:47 PM
jpeg edited reviewers, added: ohduna; removed: jpeg.

I will update it.

jpeg updated this revision to Diff 9486.Jul 4 2016, 10:48 PM
jpeg edited edge metadata.

Rebased on top of master, fixed some ABI issues, now compiles without
warnings besides the use of a deprecated header.

jpeg updated this revision to Diff 9487.Jul 4 2016, 10:53 PM
jpeg edited edge metadata.

Try to revert unwanted changes

jpeg added inline comments.Jul 4 2016, 11:13 PM
src/lib/ecore/Ecore_Common.h
3190

Important: this needs not to exist, return an Eina_Iterator instead. Please don't add a list return in Tizen.

jpeg abandoned this revision.Jul 7 2016, 1:43 AM

Alright. This patch is quite nice, but EFL has now moved on to ecore_drm2 + elput and ecore_wl2.
Also, for the device info structure, I added the EO API in libefl, which can be shared between ecore and evas, simplifying the code a lot.
I pushed some changes to devs/jpeg/ecore_device for this patch to simply build fine, and use the EO data struct.

In order to accept this feature in the future, we will need:

  • porting to ecore_drm2 and elput
  • usage in ecore_wl2
  • wayland extension protocol to pass device information from E to client apps

Until all of this is done, this feature will be put on hold.