setxkbmap: Refactoring for better style and maintainability?

Van de Bugger van.de.bugger at gmail.com
Thu Feb 10 16:55:16 PST 2011


Hi,

I am looking into setxkbmap source. It looks like it written more than
30 years ago. I would like to refactor the code a bit. For example, I
would like to combine scattered data. Let us consider existing code:

/**
 * human-readable versions for RULES_NDX, CONFIG_NDX, etc. Used for
error
 * reporting.
 */
static char *svName[NUM_STRING_VALS] = {
    "rules file", "config file", "X display", "locale",
    "keyboard model", "keyboard layout", "layout variant",
    "keycodes", "types", "compatibility map", "symbols", "geometry",
    "keymap"
};
/**
 * Holds the source for each of RULES, CONFIG, DISPLAY, etc.
 * i.e. if svSrc[LAYOUT_NDX] == FROM_SERVER, then the layout has been
fetched
 * from the server.
 */
static int svSrc[NUM_STRING_VALS];
/**
 * Holds the value for each of RULES, CONFIG, DISPLAY, etc.
 */
static char *svValue[NUM_STRING_VALS];

Properties of one setting (setting name, source, and value) are
scattered into 3 independent arrays. I would like to convert it to:

struct setting {
	/**
	 * Human-readable versions for RULES_NDX, CONFIG_NDX, etc.
	 * Used for error reporting.
	 */
	char const * name; 
	/**
	 * Holds the value for each of RULES, CONFIG, DISPLAY, etc.
	 */
	char *       value;
	/**
	 * Holds the source for each of RULES, CONFIG, DISPLAY, etc.
	 * i.e. if svSrc[LAYOUT_NDX] == FROM_SERVER, then the layout has been
fetched
	 * from the server.
	 */
	int          source;
};

typedef struct setting setting_t;

struct settings {
	setting_t rules;	      /* rules file */
	setting_t config;	      /* config file (if used) */
	setting_t display;	      /* X display name */
	setting_t locale;	      /* machine's locale */
	setting_t model;	
	setting_t layout;	
	setting_t variant;	
	setting_t keycodes;	
	setting_t types;	
	setting_t compat;	
	setting_t symbols;	
	setting_t geometry;	
	setting_t keymap;	
};

typedef struct settings settings_t;

static settings_t settings = {
    { "rules file",        NULL, UNDEFINED },
    { "config file",       NULL, UNDEFINED },
    { "X display",         NULL, UNDEFINED },
    { "locale",            NULL, UNDEFINED },
    { "keyboard model",    NULL, UNDEFINED },
    { "keyboard layout",   NULL, UNDEFINED },
    { "layout variant",    NULL, UNDEFINED },
    { "keycodes",          NULL, UNDEFINED },
    { "types",             NULL, UNDEFINED },
    { "compatibility map", NULL, UNDEFINED },
    { "symbols",           NULL, UNDEFINED },
    { "geometry",          NULL, UNDEFINED },
    { "keymap",            NULL, UNDEFINED }
};

The information is the same, but combined in more object-oriented way.
It allows changing interface of the trySetString function from

void trySetString(int which, char *newVal, int src);

to

void trySetString(setting_t * setting, char *newVal, int src);

Advantages are obvious: better maintainability due to (1) better type
control from compiler (because the first argument is of unique type, not
widely used "faceless" int), (2) not using global variables, etc.

Who is the maintainer of setxkbmap? Would you accept such fix?

Thanks,
Van.



More information about the xorg-devel mailing list