Model XGetDefault, XrmGetDatabase, XrmSetDatabase

[Airblader]

The functions XGetDefault, XrmGetDatabase and XrmSetDatabase should have an equivalent in xcb-util-xrm.

[Airblader]

@psychon Actually I don't think the two Xrm functions make sense here. The display equivalent in xcb would be the connection, but that object simply has no "database" and we can't modify it.

The only thing we can do, IMHO, is offer a convenience function like const char *xcb_xrm_get_default(db_t **database, char *app, char *name) which returns the string value and uses the passed database. If the database is NULL, it initializes it according to the same rules as XGetDefault. That way, this would work and only load the database once:

xcb_xrm_database *database = NULL;
const char *foo = xcb_xrm_get_default(&database, "myapp", "foo"); // database == NULL, so it will be loaded
const char *baz = xcb_xrm_get_default(&database, "myapp", "baz"); // database != NULL, so it will be reused

[psychon]

I never mentioned the two Xrm-functions, you came up with that. ;-)
However, you are right. What I was actually thinking of was XrmGetDatabase(). Looking at it now, I don't understand what XGetDefault() actually does. The function that I looked at in Xlib is InitDefaults(), but that function is not exported. I just assumed that XGetDefault() can be used for calling this. Sorry.

The following code is ugly, taken (more or less) from awesome and is used to get the default database:

database = XrmGetDatabase(display)
if (database == NULL) {
  XGetDefault(display, "", ""); /* Initialize DB */
  database = XrmGetDatabase(display);
}

I'd like to have some non-ugly replacement for the above (or you can tell me that awesome is doing it wrong).

[Airblader]

@psychon Yes, you're right that you didn't mention them in the first place. :)

Looking at it now, I don't understand what XGetDefault() actually does.

It is a somewhat weird function. It's both a convenience wrapper around some Xrm calls, but also includes the functionality to load the "correct" database if necessary (resource manager atom, Xdefaults, XENVIRONMENT, …).

XrmGetDatabase doesn't do any of that, which is why you do the workaround to call XGetDefault once to load the "correct" database into display and then call XrmGetDatabase again.

However, in XCB we simply don't have the connection between xcb_xrm_connection_t and a database (and no possibility to introduce it (not that I'd want to)). So some of this stuff just doesn't make sense here.

I think what we need is:

• A function which loads the database according to the same pattern (resource manager string and so on and so forth).
• A convenience function that queries a resource and returns the string value without any intermediate steps of creating a resource, strduping the resource value and freeing the resource again. I don't see why we would need to artificially restrict this to two-component strings like XGetDefault does, though.

So, to summarize, we'd have something like xcb_xrm_database_from_environment(conn) and xcb_xrm_resource_get_string(database, resource_name, resource_class)

[Airblader]

or you can tell me that awesome is doing it wrong

There's nothing wrong per se, but I think you could just as well do this:

XGetDefault(display, "", ""); // ensure a database has been set
database = XrmGetDatabase(display);

without the NULL check around it. At worst, it results in a single unnecessary, trivial resource lookup during the lifetime of display.

[psychon]

👍 For xcb_xrm_database_from_environment (but I'm not totally sure about the name... something with default in its name? Dunno).
For xcb_xrm_resource_get_string, I'm not totally convinced that this is needed and it sounds like it does something quite wasteful (construct a DB, query it, throw the DB away) and thus encourages bad API usage. Ultimately, it's your call if you want such a function.

Sorry for my initial confusion. I wanted to say "something like xcb_xrm_database_from_environment should be added".

[Airblader]

For xcb_xrm_resource_get_string, I'm not totally convinced that this is needed and it sounds like it does something quite wasteful (construct a DB, query it, throw the DB away) and thus encourages bad API usage. Ultimately, it's your call if you want such a function.

No, that's not my intention. It would use an existing database that you pass to it, it just simply wraps this:

const char *value = NULL;
xcb_xrm_resource_t *resource;
if (xcb_xrm_resource_get(database, "Foo", "Foo", &resource) >= 0)
    value = strdup(xcb_xrm_resource_value(resource));
xcb_xrm_resource_free(resource);

into this:

const char *value = xcb_xrm_resource_get_string(database, "Foo", "Foo");

And getting a string resource with a NULL fallback is by far the most common usage AFAICT, so I think it makes sense.

but I'm not totally sure about the name... something with default in its name? Dunno

I'm open to other names. :) xcb_xrm_database_from_default?

[Airblader]

Frankly, right now I'm starting to wonder whether we should ditch xcb_xrm_resource_value* altogether and go with xcb_xrm_resource_get_[string|…] instead, completely eliminating the xcb_xrm_resource_t type for the caller. What are they ever going to do with the resource other than getting its value?

[psychon]

True... And yeah, that wrapper seems useful, but perhaps adding _with_default() to its name to make it clearer what some of the arguments are?

Random slightly off-topic thing: psychon/awesome@6ed40af https://travis-ci.org/psychon/awesome/jobs/119765242
The instructions in your README (last line) are wrong and only work for you.
(I'll update this comment with edits for when I get this to work)

Edit: psychon/awesome@master...psychon:xcb-util-xrm
(The build succeeded (mostly), no idea if things work correctly since I didn't actually try it yet. However, it's only ever used as 'xrdb_get_value("", "background")andxrb_get_value("", "foreground")` right now...)

[Airblader]

And yeah, that wrapper seems useful, but perhaps adding _with_default() to its name to make it clearer what some of the arguments are?

If the name contained that, I'd expect to pass another argument that is used as the default value (rather than NULL). The two arguments I had in there right now are resource name + class.

Random slightly off-topic thing: psychon/awesome@6ed40af https://travis-ci.org/psychon/awesome/jobs/119765242
The instructions in your README (last line) are wrong and only work for you.

Nice. :) Yeah, you'll probably just need to use the https link of the repository. I'll change it. (Edit: fixed)

[Airblader]

I'm going to summarize (mostly for myself) what needs to happen here now:

• Remove xcb_xrm_resource_t from exposed library.
• Remove xcb_xrm_resource_value
• Remove xcb_xrm_resource_value_long
• Remove xcb_xrm_resource_value_bool
• Remove xcb_xrm_resource_get
• Add const char *xcb_xrm_resource_get_string(xcb_xrm_database_t *database, const char *res_name, const char *res_class)
• Add long xcb_xrm_resource_get_long(xcb_xrm_database_t *database, const char *res_name, const char *res_class)
• Add bool xcb_xrm_resource_get_bool(xcb_xrm_database_t *database, const char *res_name, const char *res_class)
• Add xcb_xrm_database_from_default(xcb_connection_t *conn)

@psychon Does that sound good to you?

[psychon]

Yup, sounds good to me. Thanks for clearing up the confusion that I caused.

[Airblader]

Small corrections:

• We cannot return const char * now since the string now really is owned by the caller and must be free'd.
• I wonder if, next to directly fetching a resource as long / bool, we should also expose those utility functions on string arguments. I think some callers may not care if a resource is not configured or configured as "false", but others might. And if you do care, *_get_bool won't tell you the difference. Exposing a "from string to bool/long" converter allows the caller to query the string value, check if it's NULL and convert it if not.

[Airblader]

I've opened #37 for the database function.

[Airblader]

closed by #38