add support for the simpler functions in Xresources.h

[djerius]

This PR adds support for the more straightforward X resource manager functions in Xresources.h.

It doesn't expose the Quark versions of the functions, or the enumeration capability.

It adds new test dependencies on Test::TempDir::Tiny as well as a version of Test:::More which supports subtest (which has been around for a while!).

I've tested it on Perl 5.10.1 and 5.30.0.

[nrdvana]

Hi, thanks for the contribution!
I probably won't have time to do a full review for a week or two , but I gave it a brief read. Here are some thoughts:

• I'm fine with requiring a newer Test::More. subtests are good.
• I'll probably try to maintain 5.8 compatibility. I don't have any vested interest in 5.8, but the module supported 5.8 when I took over maintaining it and I prefer not to break things.
• In the rest of the dist, for the object-oriented methods/attributes I have used more friendly Perl names and calling conventions. However that is more work and it doesn't hurt anything to make the Xwhatever available as methods. Friendly wrappers could always be added later.
• X11 has a lot of "sharp edges" that I try to safeguard when possible. Do you know if calling XrmGetDatabase twice on the same Display would return the same pointer? If so, then would it get double-freed if those perl references are dropped? and for that matter, should it be allowed to be freed if the Display might be using it? I solved those types of problems for other X11 objects with the PerlXlib_obj_for_display_innerptr and typemap O_X11_Xlib_Opaque, however those assume that there is always a dpy reference in scope when the pointer is encountered, which is not the case for XrmDatabase. Any insight you can share about XrmDatabse pointer lifecycle would be helpful.
• Maybe my XS-fu is rusty, but don't you need to call SvPV_nolen before SvCUR? (because PV has a coercing effect for non-string or string-like objects, but I don't think CUR does) For that matter, I'd guess SvPVbyte is more appropriate unless X11 understands UTF-8.
• I noticed this: "The XrmInitialize function initialize the resource manager. It must be called before any other Xrm functions are used". Not sure if that is important enough to bother cluttering the module with it, since it seems you didn't need it.

[djerius]

Thanks for the quick reply. To your points:

• I'll test against 5.8.
• I'll follow your conventions for Perl friendly vs. original API, but will need some guidance as to what they should be. My preference is to mirror the original API so that translating from C into Perl is simplified and so that examples in the original documentation work. Most of the Xresources functions are independent of the X server, so could get moved into a separate module, but then there are things like XScreenResourceString, XResourceManagerString, XrmSetDatabase, XrmGetDatabase which seem to more appropriately live into the packages for displays and screens. Xrm is weird. There is no way of directly creating an empty database. Some functions (like XrmPutResource) take the database handle passed by reference, and if the handle is NULL, will create a database for you (hence the XrmDatabaseMaybe typemap). In my tests I create an empty database by having it parse an empty string. Not having to support that interface would simplify the code, but makes it more difficult to translates existing C code into Perl, as there are now two API's to worry about.
• You are correct, XrmGetDatabase and XrmSetDatabase have lifecycle issues. I'll look at your work to see if that's a viable option here. In other cases, because of Xrm's autovivification of databases, it's hard to prevent an XrmDatabase handle which is explicitly invalidated by calling XrmDestroyDatabase from being reused (Xrm will happily create another database if passed a NULL pointer). I believe I've safely handled all of the cases where Xrm will destroy a database in the course of manipulating it, but because of the autovivification, can't prevent the footgun of inadvertently reusing the invalidated XrmDatabase handle.
• My XS-fu is really rusty, so I'll double check on the SvCUR. To be honest, I had a devil of a time not getting segv's out of the IN_OUT code for XrmDatabase and XrmDatabaseMaybe. For some reason if a blessed object in the XS stack was updated more than once, it trampled memory (even though they dumped out as what I expected) and it would segv in the Xrm code. That's why the OUTPUT typemap code for those two types only runs sv_setrev_pv if the original entry wasn't an object.

I noticed this: "The XrmInitialize function initialize the resource manager. It must be called before any other Xrm functions are used". Not sure if that is important enough to bother cluttering the module with it, since it seems you didn't need it.

I'm not sure I follow you here; it is required, (and is implemented).

[nrdvana]

Let me elaborate a bit on O_X11_Xlib_Opaque, since it's a bit complicated and unusual. For a lot of Xlib opaque structs (where they give you the pointer but no further knowledge about it) the pointer is associated with a Display and can be used as long as the Display exists and should not be used once the Display is destroyed. Also, various functions can return those pointers, and might return them more than once, and ideally a Xlib function returning the same pointer would return a second reference to the same Perl object.

So, I came up with a cache of objects in the Display, so that any time the typemap is trying to wrap a pointer, it checks the Display object's cache to see if that pointer was already wrapped. To make things a little more flexible I decided that I would represent the objects with a blessed hashref and then attach the C pointer with Magic. That gives me the option to store my own attributes in the object, like a flag whether the perl object should be in charge of freeing it. I managed to wrap up most of that in a reusable function.

For the x resource managers, it is almost the same, but the pool of objects would need to be global instead of associated per-display. Most of the code can probably be re-used, and just need to add some kind of flag to tell it to look in the global pool.

If you use a hashref for the objects, you could then add a flag that tracks whether it needs freed or not. Then you could set or unset that flag depending on which API calls the user makes.

[nrdvana]

For the other points:

• I also try to match the original API as much as possible, for the exported functions. I'm just saying that nobody porting from C will be calling methods, only functions. So, XrmPutResource(my $xrm, .....) is great, but $xrm->XrmPutResource(....) seems a little awkward compared to $xrm->put(...). In a few places I also change the name to something more common, like $window->show as the wrapper for XMapWindow. But again, I'm not requesting this, just suggesting that if you felt like it, feel free to make more freindly methods.
• For conditionally destroying things, I use autofree as the attribute for that.
• I've actually never used IN_OUT before. I tend to write my own boilerplate for those cases because there are usually sticky edge cases to handle. Maybe the XrmDatabase destructor was running accidentally? Combining it with an object cache and a autofree flag should resolve some of the problems.
• I see now where you call XrmInitialize in the test case. My thought was whether the other API functions should check a global flag and call it automatically... but that is probably making too many assumptions for the user. However, a friendly-named method constructor in XrmDatabase might decide to do that.

[djerius]

I also try to match the original API as much as possible, for the exported functions. I'm just saying that nobody porting from C will be calling methods, only functions. So, XrmPutResource(my $xrm, .....) is great, but $xrm->XrmPutResource(....) seems a little awkward compared to $xrm->put(...). In a few places I also change the name to something more common, like $window->show as the wrapper for XMapWindow. But again, I'm not requesting this, just suggesting that if you felt like it, feel free to make more freindly methods.

I think we're on the same page with this; I've implemented this.

For conditionally destroying things, I use autofree as the attribute for that.

I haven't had time to look through this yet.

I've actually never used IN_OUT before. [...] Maybe the XrmDatabase destructor was running accidentally?

That was exactly the problem. Thanks for the suggestion.

I see now where you call XrmInitialize in the test case. My thought was whether the other API functions should check a global flag and call it automatically... but that is probably making too many assumptions for the user. However, a friendly-named method constructor in XrmDatabase might decide to do that.

The initialization it performs is really minor. It creates a couple of entries in a hash table for fast string lookup, so the first time it is called there are some minor allocations, but after that calling it again is essentially a no-op. One approach may be to have it run from X11::Xlib when that is loaded, avoiding any extra machinery.

[nrdvana]

I completely reworked the way Xlib objects get their Magic assigned. Now they all get a struct of fields attached and we can add fields as needed. The objects no longer need to be associated with a Display, and there is a more generic "dependency" concept where if a parent C-level object becomes invalid, all the dependent C-level pointers get set to NULL and their host objects can reliably handle what to do about that. (such as if a Resource manager was attached to the life of a Display* connection)

[mohawk2]

Is this PR still applicable given that structural change?

[djerius]

Is this PR still applicable given that structural change?

I can't answer to that, but I'm no longer working on the project this was attached to, so am not able to provide any resources on this going forward.

[nrdvana]

As I remember, the XResources API requires some awkward memory management, and would take a bit of effort to really ensure that the wrapped API wasn't going to leak things or crash. Meanwhile, the concept behind XResources is basically just a serializable Hashref, and could more ideally be implemented in pure perl. (unless there's some function somewhere that specifically requires an XResources pointer and does something nontrivial with it, which there could be)

I hate to let the work here go to waste, but I don't really have time or motivation for it either, yet. I was just going to leave it here for whoever needs the feature next to pick up where we left off.