Introduce convertible module #49
Loading…
Reference in New Issue
No description provided.
Delete Branch "rafspiny/enlightenment:master"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Close #43
See inline comments... only going to mentuion the same issue the first 1 or 2 times - same applies to other instances of it :)
@ -0,0 +7,5 @@
#include "dbus_acceleration.h"
#include "e_mod_main.h"
#include "input_rotation.h"
DbusAccelerometer* accelerometer_dbus;
shouldnt accelerometer_dbus be static?
Made it static.
@ -0,0 +9,5 @@
#include "input_rotation.h"
DbusAccelerometer* accelerometer_dbus;
DbusAccelerometer* sensor_proxy_init() {
functions in C that take NO arguments are (void) not (). () means "i accept any number of arguments of an undefined type". :)
also some funcs are
some are
with return type on previous line - be consistent and maybe do the latter?
Corrected the linting for function to be
Also in the headers
Do we have a linting configi file somewhere? So we can get this kind of comments automatically? Like clang
@ -0,0 +16,5 @@
{
INF("We already have a struct filled");
return accelerometer_dbus;
}
accelerometer_dbus = calloc(1, sizeof(DbusAccelerometer));
if calloc fails?
I think in most case I can use EINA_SAFETY_ON_NULL_RETURN_VAL. Am I correct?
@ -0,0 +22,5 @@
// The next line is probably redundant
accelerometer_dbus->orientation = undefined;
DBG("Before eldbus initialization");
int initialization = eldbus_init();
do we really need this? eldnbus should alredy be initted by e before any module loads...
Removed it.
@ -0,0 +57,5 @@
return accelerometer_dbus;
}
void sensor_proxy_shutdown()
functions in C that take NO arguments are (void) not (). () means "i accept any number of arguments of an undefined type". :)
I think I fixed all of them.
@ -0,0 +109,5 @@
return sensor_proxy;
}
enum screen_rotation access_string_property(const Eldbus_Message *msg, Eldbus_Message_Iter **variant, Eina_Bool* result)
wouldnt this be better to just have one of the screen_orientation enum type of undefined that is already there and not have a bool result return ptr arg?
btw - in e/efl we dont use enum xxxx we typedef like typedef enum Screen_Orientation Screen_Orientation
also functions like access_string_property are only accessed within this file - shouldnt they be static? and maybe be named access_string_property (with an underscore at the front) to show they are local only to that file? there are various other funcs that are not needed outside of this file .. so consider moving them all to static?
@ -0,0 +111,5 @@
}
enum screen_rotation access_string_property(const Eldbus_Message *msg, Eldbus_Message_Iter **variant, Eina_Bool* result)
{
const char *type = NULL;
this should not be cosnt... :)
Fixed.
@ -0,0 +138,5 @@
{
WARN("Expected type is string(s).");
*result = EINA_FALSE;
}
const char **string_property_value = calloc(PATH_MAX, sizeof(char));
if calloc fails?
Same thing. I think in most case I can use EINA_SAFETY_ON_NULL_RETURN_VAL. Am I correct?
@ -0,0 +146,5 @@
*result = EINA_FALSE;
}
enum screen_rotation rotation = undefined;
if (strcmp(ACCELEROMETER_ORIENTATION_RIGHT, *string_property_value) == 0)
shouldnt all the below if's be else if () ? also e/efl use the if (!strcmp()) convention :)
Changed all of them.
@ -0,0 +155,5 @@
rotation = flipped;
if (strcmp(ACCELEROMETER_ORIENTATION_NORMAL, *string_property_value) == 0)
rotation = normal;
free((void *) type);
you dont have to cast to void * for free in C.
Thanks. Remove the cast in all 3 places.
@ -0,0 +163,5 @@
Eina_Bool
access_bool_property(const Eldbus_Message *msg, Eldbus_Message_Iter **variant, Eina_Bool *boolean_property_value)
{
const char *type;
shouldn't be const... :)
Done.
@ -0,0 +194,5 @@
{
WARN("error in eldbus_message_iter_arguments_get()");
res = EINA_FALSE;
}
free((void *) type);
no need for void * cast
@ -0,0 +211,5 @@
ERR("Error: %s %s", errname, errmsg);
}
access_bool_property(msg, &variant, &has_accelerometer);
DbusAccelerometer *accelerometer = (DbusAccelerometer *) data;
dont need to cast to DbusAccelerometer * ... C casts anything to/from void * automatically as void * accepts any ptr (well const is another matter - separate).
Removed cast
@ -0,0 +232,5 @@
const char *errname, *errmsg;
enum screen_rotation orientation;
Eldbus_Message_Iter *variant = NULL;
Eina_Bool* result = calloc(1, sizeof(Eina_Bool));
waiiit.. whay do we calloc a bool? why? why not just have it on the stack?
I got rid of
result
@ -0,0 +262,5 @@
EINA_LIST_FOREACH(inst->randr2_ids, l, randr_id)
{
_fetch_and_rotate_screen(randr_id, orientation);
}
free((void *)randr_id);
this doersn't make sense... to free this when it's a const char * and you loop over the list stored in randr2_ids ? this smells of all kinds of wrong and bugs to me... i haven't seen how you create this list of strings yet...
Ok. This should not be const. Removed that.
@ -0,0 +298,5 @@
}
return transformation;
}
int _fetch_X_device_input_number()
functions in C that take NO arguments are (void) not (). () means "i accept any number of arguments of an undefined type". :)
also shouldnt this be static - like many others. it's local only to this file?
@ -0,0 +302,5 @@
int _fetch_X_device_input_number()
{
// I should get the touchscreen associated with the screen probably by looking at the classes of the input devices
// I need to submit my patch to add getters for other XIDeviceInfo fields, like raster mentioned in his commit.
const char *dev_name = NULL;
don't have to set to NULL... always set below
Done, for both lines.
@ -0,0 +303,5 @@
{
// I should get the touchscreen associated with the screen probably by looking at the classes of the input devices
// I need to submit my patch to add getters for other XIDeviceInfo fields, like raster mentioned in his commit.
const char *dev_name = NULL;
char **property_name = NULL;
same as above - dont need to set to NULL
@ -0,0 +307,5 @@
char **property_name = NULL;
int dev_num = ecore_x_input_device_num_get();
int dev_number = -1;
for (int dev_counter=0; dev_counter<dev_num; dev_counter++)
stytle-wise this is mixed. most of e/efl uses spaces like 'int x = 0' not 'int x=0'
Done.
@ -0,0 +320,5 @@
int is_correct_device = _is_device_a_touch_pointer(dev_counter, num_properties, iterator);
if (is_correct_device == EINA_FALSE)
continue;
iterator = property_name;
for (int i=0; i<num_properties; i++)
spaces as above
Done.
@ -0,0 +322,5 @@
continue;
iterator = property_name;
for (int i=0; i<num_properties; i++)
{
if (strcmp(*iterator, CTM_name) == 0)
!strcmp...
Changed all 5 of them.
@ -0,0 +346,5 @@
if (strstr(*iterator, "Axis Labels") != NULL)
{
int num_ret, unit_size_ret;
Ecore_X_Atom format_ret;
char *result = NULL;
int result = ecore_x.... ? better
char* result = ecore_x...
@ -0,0 +348,5 @@
int num_ret, unit_size_ret;
Ecore_X_Atom format_ret;
char *result = NULL;
result = ecore_x_input_device_property_get(dev_counter, *iterator, &num_ret, &format_ret, &unit_size_ret);
if (result != NULL) {
shorter:
Fixed all
(xxx != NULL)
conditions.@ -0,0 +386,5 @@
int num_ret, unit_size_ret;
Ecore_X_Atom format_ret;
char *result = NULL;
TransformationMatrix *matrix = calloc(1, sizeof(TransformationMatrix));
calloc fails?
Same thing. I think in most case I can use EINA_SAFETY_ON_NULL_RETURN_VAL. Am I correct?
@ -0,0 +9,5 @@
#define EFL_DBUS_ACC_PATH "/net/hadess/SensorProxy"
#define EFL_DBUS_ACC_IFACE "net.hadess.SensorProxy"
// This enum represents the 4 states of screen rotation plus undefined
enum screen_rotation {undefined, normal, right_up, flipped, left_up};
normally er ur all caps for enums like UNDEFINED NORMAL etc.
Made them all caps.
@ -0,0 +43,5 @@
* @param result 1 if result is ok, 0 if it failed
* @return Enum specifying the orientation
*/
enum screen_rotation
access_string_property(const Eldbus_Message *msg, Eldbus_Message_Iter **variant, Eina_Bool* result);
wouldnt a lot fo funcs like this just be static internal to the .c file and not in the .h?
@ -0,0 +7,5 @@
/* LIST OF INSTANCES */
static Eina_List *instances = NULL;
void _update_instances(const Instance *current_instance)
shouldnt this be a static func? same with many others...
@ -0,0 +138,5 @@
return cfd;
}
void
econvertible_config_init()
functions in C that take NO arguments are (void) not (). () means "i accept any number of arguments of an undefined type". :)
I think I addressed them all now.
@ -0,0 +152,5 @@
DBG("Config loaded");
}
void econvertible_config_shutdown()
functions in C that take NO arguments are (void) not (). () means "i accept any number of arguments of an undefined type". :)
I think I addressed them all now.
@ -0,0 +289,5 @@
if (!accelerometer->dbus_property_changed_sh)
ERR("Error: could not add the signal handler for PropertiesChanged");
// Screen related part
E_Zone *zone = NULL;
don't have to set to NULL
Done.
@ -0,0 +306,5 @@
if (screen->info.can_rot_90 == EINA_TRUE)
{
int max_screen_length = 300;
char *randr2_id = malloc(sizeof(char) * max_screen_length);
int copied_chars = eina_strlcpy(randr2_id, zone->randr2_id, max_screen_length);
why al of the above? why not just strdup? :)
I thing I tried 2 years ago and got some weird issue. Re-introducing it.
Introduce convertible moduleto WIP: Introduce convertible module@raster I made some changes to address your comments. Ready to give it another go.
See inline comments. :) there's a bit of consistency work to be done as a general thing - like consistently choosing the same var names e.gh. one place had Evas_Object *o; and one Evas_Object *evas_object;
@ -0,0 +60,5 @@
int num_ret, unit_size_ret;
Ecore_X_Atom format_ret;
char *result = ecore_x_input_device_property_get(dev_counter, *iterator, &num_ret, &format_ret, &unit_size_ret);
if (result) {
// TODO Shall check for the value "Abs MT Position"
You should free result.... or you will be leaking.
@ -0,0 +99,5 @@
{
if (!strcmp(*iterator, CTM_name))
{
dev_number = dev_counter;
DBG("Setting device: %d", dev_number);
Any reason ytou keep looping and don't just break the loop here?
@ -0,0 +161,5 @@
memcpy(matrix->values, rotation_matrix_2d, 6 * sizeof(*rotation_matrix_2d));
ecore_x_input_device_property_set(x_dev_num, CTM_name, matrix->values, num_ret, format_ret, unit_size_ret);
DBG("Input device %d rotated to %d", x_dev_num, rotation);
} else {
free result again as above.
@ -0,0 +227,5 @@
res = EINA_FALSE;
return res;
}
if (type[1])
correctness. checking type[1] without type[0] checked first is assuming type will never be "" (empty string) as then type[1] wll be beyond the end of this string. shouldn't you restructure this to be sure type[0] != 0 first so type[1] will exist?
@ -0,0 +405,5 @@
return rotation;
}
type = eldbus_message_iter_signature_get((*variant));
if (type[1])
same here again - checking type[1] before type[0] is known to be 0 or not.
@ -0,0 +413,5 @@
if (type[0] != 's')
{
WARN("Expected type is string(s).");
}
const char **string_property_value = calloc(PATH_MAX, sizeof(char));
this calloc jusyt screams "wrong". char **x = calloc(...); ... as opposed to char *x = calloc(...);
@ -0,0 +438,5 @@
void
on_accelerometer_orientation(void *data, const Eldbus_Message *msg, Eldbus_Pending *pending EINA_UNUSED)
{
INF("New orientation received");
Instance *inst = (Instance *) data;
youy know you don't have to cast to/from void * right... ? everything auto-casts to/from void * ... :)
@ -0,0 +11,5 @@
static void
_update_instances(const Instance *current_instance)
{
Eina_List *l;
Instance *instance = NULL;
you know you dont have to set instance to NULL... :)
@ -0,0 +10,5 @@
// Definition of the data structure to hold the gadget configuration
typedef struct _Convertible_Config Convertible_Config;
struct _Convertible_Config
{
E_Module *module;
And reasons module is here inb config structs and not just a global for the module for the handle to the module data? you have convertible_module already as a global...
@ -0,0 +216,5 @@
static Evas_Object *
_gc_icon(const E_Gadcon_Client_Class *client_class EINA_UNUSED, Evas *evas)
{
Evas_Object *o;
char buf[PATH_MAX];
consistency - here its PATH_MAX = below its 4096... :)
@ -0,0 +227,5 @@
static const char *
_gc_id_new(const E_Gadcon_Client_Class *client_class EINA_UNUSED)
{
static char buf[4096];
consistency - here it's 4096, above it's PATH_MAXw its 4096... :)
@ -0,0 +9,5 @@
#define EFL_DBUS_ACC_PATH "/net/hadess/SensorProxy"
#define EFL_DBUS_ACC_IFACE "net.hadess.SensorProxy"
// This enum represents the 4 states of screen rotation plus UNDEFINED
enum screen_rotation {UNDEFINED, NORMAL, RIGHT_UP, FLIPPED, LEFT_UP};
style-wise - in e and efl we never just use struct xxx or enum xxxx we typedef to a full type. it's just style/consistency.
Changed into typedef enum {UNDEFINED, NORMAL, RIGHT_UP, FLIPPED, LEFT_UP} screen_rotation;
Adapted the type whenever it was used.
@ -0,0 +25,5 @@
/**
* Fetch the DBUS interfaces and fill the DbusAccelerometer struct
* */
DbusAccelerometer* sensor_proxy_init();
func() in c means zero OR more arguments - unknown. (void) means no arguments at all. different to c++ and other langs.
Done
@ -0,0 +28,5 @@
* */
DbusAccelerometer* sensor_proxy_init();
void sensor_proxy_shutdown();
func() in c means zero OR more arguments - unknown. (void) means no arguments at all. different to c++ and other langs.
Done
@ -0,0 +45,5 @@
{
DBG("Keyboard: Signal %s received from %s", sig, src);
}
void
The way you use this later... you could just make Eina_List *instances declared in the e-gadget-convertible.h file instead of static and lose this function entirely... it'd be less code.
I removed the function and moved
instances
to the header. Also removed static. That should do.@raster should it not be static? It is used in e_mod_main.c too.
If you expose the Eina_List *list; then it shouldn't be static. static == local to that file only and not visible outside of that file (for functions and global vars. for vars within a function it means they keep their value between calls to that function).
@ -0,0 +9,5 @@
// Definition of the data structure to hold the gadget configuration
typedef struct _Convertible_Config Convertible_Config;
struct _Convertible_Config
{
If you look at other config structs = i normally put an int version in in advance. this allows me to version the config and if in future new thing are added or removed ... the version can help clear up how old the config file is and "upgrade it". i'd suggest putting that in and well - removing the module field as its not part of config really :)
@ -0,0 +83,5 @@
// evas_object_smart_callback_add(parent, "gadget_site_anchor", _anchor_change, inst);
// Adding callback for EDJE object
INF("Adding callback for creation and other events from EDJE");
edje_object_signal_callback_add(evas_object, "lock,rotation", "tablet", _rotation_signal_cb, instance);
ok - this is an edje signal api thing. in e all signals he a "source" of "e" for namspacing. this allows you to differentiate between signals that are just sent within an edje obj from part to part and signals that go to/from e. you should probably adapt the edj to use "e" source namespaced signals to be clean.
@ -0,0 +359,5 @@
DBG("Removing the logger");
eina_log_domain_unregister(_convertible_log_dom);
_convertible_log_dom = -1;
return 1;
missing e_gadcon_provider_unregister(); here - you don't unregister the gadcons on module unload. you'll crash and not delete the convertible gadgets if you unload the module (try unloading the module with convirtble gadgets floating about...)/
I do use it. Right before unregistering the log domain.
WIP: Introduce convertible moduleto Introduce convertible module@raster I addressed your comments. It would be great to have another review from you whenever you have some spare time.
efa2c17235
to3def50bd6a