On Tue, Jul 30, 2013 at 11:07:31PM +0200, Armin K. wrote:
> On 07/30/2013 10:28 PM, Jonas Ådahl wrote:
> > On Tue, Jul 30, 2013 at 09:36:25PM +0200, Armin K wrote:
> >> This patch adds 3 new options to weston.ini to allow
> >> the user to change default constant_accel_factor,
> >> min_accel_factor and max_accel_factor. If no options
> >> are set, it falls back using defaults as it did before.
> >>
> >> v2: create weston_config_section_get_double and use it
> >> instead of manualy converting string to double.
> >>
> >> v3: add default values in weston_config_get_double
> >> instead of using conditionals.
> >
> > One comments on a minor details below:
> >
> >> ---
> >> shared/config-parser.c | 26 ++++++++++++++++++++++++++
> >> shared/config-parser.h | 4 ++++
> >> src/evdev-touchpad.c | 40 +++++++++++++++++++++++++++++++++++-----
> >> weston.ini | 5 +++++
> >> 4 files changed, 70 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/shared/config-parser.c b/shared/config-parser.c
> >> index 4e6cf7f..f98209c 100644
> >> --- a/shared/config-parser.c
> >> +++ b/shared/config-parser.c
> >> @@ -337,6 +337,32 @@ weston_config_section_get_uint(struct
> >> weston_config_section *section,
> >>
> >> WL_EXPORT
> >> int
> >> +weston_config_section_get_double(struct weston_config_section *section,
> >> + const char *key,
> >> + double *value, double default_value)
> >> +{
> >> + struct weston_config_entry *entry;
> >> + char *end;
> >> +
> >> + entry = config_section_get_entry(section, key);
> >> + if (entry == NULL) {
> >> + *value = default_value;
> >> + errno = ENOENT;
> >> + return -1;
> >> + }
> >> +
> >> + *value = strtod(entry->value, &end);
> >> + if (*end != '\0') {
> >> + *value = default_value;
> >> + errno = EINVAL;
> >> + return -1;
> >> + }
> >> +
> >> + return 0;
> >> +}
> >> +
> >> +WL_EXPORT
> >> +int
> >> weston_config_section_get_string(struct weston_config_section *section,
> >> const char *key,
> >> char **value, const char *default_value)
> >> diff --git a/shared/config-parser.h b/shared/config-parser.h
> >> index 794e09c..410a7ef 100644
> >> --- a/shared/config-parser.h
> >> +++ b/shared/config-parser.h
> >> @@ -88,6 +88,10 @@ weston_config_section_get_uint(struct
> >> weston_config_section *section,
> >> const char *key,
> >> uint32_t *value, uint32_t default_value);
> >> int
> >> +weston_config_section_get_double(struct weston_config_section *section,
> >> + const char *key,
> >> + double *value, double default_value);
> >> +int
> >> weston_config_section_get_string(struct weston_config_section *section,
> >> const char *key,
> >> char **value,
> >> diff --git a/src/evdev-touchpad.c b/src/evdev-touchpad.c
> >> index 53300ce..e727e49 100644
> >> --- a/src/evdev-touchpad.c
> >> +++ b/src/evdev-touchpad.c
> >> @@ -26,10 +26,12 @@
> >> #include <math.h>
> >> #include <string.h>
> >> #include <stdbool.h>
> >> +#include <unistd.h>
> >> #include <linux/input.h>
> >>
> >> #include "filter.h"
> >> #include "evdev.h"
> >> +#include "../shared/config-parser.h"
> >>
> >> /* Default values */
> >> #define DEFAULT_CONSTANT_ACCEL_NUMERATOR 50
> >> @@ -670,6 +672,38 @@ struct evdev_dispatch_interface touchpad_interface = {
> >> touchpad_destroy
> >> };
> >>
> >> +static void
> >> +touchpad_parse_config(struct touchpad_dispatch *touchpad, double
> >> *diagonal)
> >
> > Why make the diagonal parameter a pointer? Looks to be like it shouldn't
> > be.
> >
>
> Because it is calculated in the touchpad_init function and is used after
> this function call. Instead of copying the parameter (reserving more
> memory), we just indirectly access it. I don't see the problem.
Jonas is right. diagonal is a double (64 bit) and the pointer to
diagonal is 64 bit (assuming 64 bit os) so they take the same space on
the stack. More importantly, passing the double as a pointer suggests
that touchpad_parse_config() will change the value or return a value
in *diagonal, which is not the case. So it's a little confusing to
pass a pointer here. Other than that, the patch looks good now.
Kristian
> >> +{
> >> + struct weston_config *config;
> >> + struct weston_config_section *s;
> >> + int config_fd;
> >> +
> >> + double constant_accel_factor;
> >> + double min_accel_factor;
> >> + double max_accel_factor;
> >> +
> >> + config_fd = open_config_file("weston.ini");
> >> + config = weston_config_parse(config_fd);
> >> + close(config_fd);
> >> +
> >> + s = weston_config_get_section(config, "touchpad", NULL, NULL);
> >> + weston_config_section_get_double(s, "constant_accel_factor",
> >> + &constant_accel_factor,
> >> + DEFAULT_CONSTANT_ACCEL_NUMERATOR);
> >> + weston_config_section_get_double(s, "min_accel_factor",
> >> + &min_accel_factor,
> >> + DEFAULT_MIN_ACCEL_FACTOR);
> >> + weston_config_section_get_double(s, "max_accel_factor",
> >> + &max_accel_factor,
> >> + DEFAULT_MAX_ACCEL_FACTOR);
> >> +
> >> + touchpad->constant_accel_factor =
> >> + constant_accel_factor / *diagonal;
> >> + touchpad->min_accel_factor = min_accel_factor;
> >> + touchpad->max_accel_factor = max_accel_factor;
> >> +}
> >> +
> >> static int
> >> touchpad_init(struct touchpad_dispatch *touchpad,
> >> struct evdev_device *device)
> >> @@ -710,11 +744,7 @@ touchpad_init(struct touchpad_dispatch *touchpad,
> >> height = abs(device->abs.max_y - device->abs.min_y);
> >> diagonal = sqrt(width*width + height*height);
> >>
> >> - touchpad->constant_accel_factor =
> >> - DEFAULT_CONSTANT_ACCEL_NUMERATOR / diagonal;
> >> -
> >> - touchpad->min_accel_factor = DEFAULT_MIN_ACCEL_FACTOR;
> >> - touchpad->max_accel_factor = DEFAULT_MAX_ACCEL_FACTOR;
> >> + touchpad_parse_config(touchpad, &diagonal);
> >>
> >> touchpad->hysteresis.margin_x =
> >> diagonal / DEFAULT_HYSTERESIS_MARGIN_DENOMINATOR;
> >> diff --git a/weston.ini b/weston.ini
> >> index f2abceb..ff0f3ba 100644
> >> --- a/weston.ini
> >> +++ b/weston.ini
> >> @@ -57,3 +57,8 @@ path=/usr/libexec/weston-keyboard
> >> #name=X1
> >> #mode=1024x768
> >> #transform=flipped-270
> >> +
> >> +#[touchpad]
> >> +#constant_accel_factor = 50
> >> +#min_accel_factor = 0.16
> >> +#max_accel_factor = 1.0
> >> --
> >> 1.8.3.4
> >>
> >
> > Jonas
> > _______________________________________________
> > wayland-devel mailing list
> > wayland-devel@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/wayland-devel
> >
>
> _______________________________________________
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
_______________________________________________
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel
