Adding meta queries to support combinations of queries as queries.

[favilo]

Adding structs:

• AndQuery
• OrQuery
• NotQuery

And combinator methods in the Query trait in order to make these usable.

[sminez]

I'm not entirely sure I'm convinced by the ergonomics of this or why this being raised as a PR without first raising an issue to discuss the feature request.

[favilo]

I'm personally using these with an extension trait to make it more ergonomic.

Apologies for not raising an issue first. I've raise #294 just now.

[sminez]

It looks like the structs you are defining aren't being given a consistent set of derives? They should all really be deriving the same common set of Debug, Copy, Clone, PartialEq and Eq.

The compiler issue you're showing here is down to the structs you've defined not having any trait bounds I suspect? Taking the NotQuery as an example:

#[derive(Debug, Copy, Clone, PartialEq, Eq)]
pub struct NotQuery<Q>(pub Q);

Here Q can be anything at all. Nothing is constraining it to be only something that implements Query. I've had a quick play around with seeing what might work without resorting to impl Query<X> everywhere and I think something like this works?

/// A query to be run against client windows for identifying specific windows
/// or programs.
pub trait Query<X: XConn> {
    /// Run this query for a given window ID.
    fn run(&self, id: Xid, x: &X) -> Result<bool>;

    /// The negation of this query
    fn not(self) -> NotQuery<X, Self>
    where
        Self: Sized,
    {
        NotQuery {
            inner: self,
            _phantom: std::marker::PhantomData,
        }
    }
}

/// The logical negation of `Q`.
#[derive(Debug, Copy, Clone, PartialEq, Eq)]
pub struct NotQuery<X: XConn, Q: Query<X>> {
    inner: Q,
    _phantom: std::marker::PhantomData<X>,
}

impl<X, Q> Query<X> for NotQuery<X, Q>
where
    X: XConn,
    Q: Query<X>,
{
    fn run(&self, id: Xid, x: &X) -> Result<bool> {
        Ok(!self.inner.run(id, x)?)
    }
}

Exposing the inner Query as public for these structs isn't really that valuable when they should be getting constructed by calling the combinator methods (and becomes actively annoying to do once we need PhantomData). Also, if this is going to be added as a feature from the main Penrose crate itself these should just be methods on the Query trait directly rather than adding an extension trait that then needs to be imported.

All that said, I'm still not convinced that I fully see the utility of the All and Any structs? I can see the logical operator structs being useful for simple combinations, but for anything sufficiently complicated that you start to use a lot of these I think it is probably going to be easier to write explicit queries instead.

[favilo]

Thanks for the pointers. Not sure why I'd not thought of PhantomData. Of course we'd need the types to ensure it is the same XConn

This looks much simpler, and I like the concrete types. Though I was wondering if returning Box<dyn Query<X>> might be good for the combinator methods. Not that dynamic dispatch is that great, but you do use it a lot in this crate. And it would allow for future updates to the underlying types without changing the public interface of these combinator methods.

EDIT: In defense of my prior implementation, I'd added the methods to an extension trait because it didn't make sense to allow people to override them just by implementing Query, since they are pretty standard logical constructs. But if you'd prefer they were here, that's all good to me. This way you don't need to use a new trait just to combine queries.

[sminez]

Box<dyn Query<X>> could work potentially but I quite like that the and method must return an AndQuery rather than an arbitrary query, and the behaviour of these combinator structs is a standard logical construct like you say. An extension trait does make sense as a way of ensure that the implementation can't be overwritten (something that I've done in the codebase already with the XConnExt trait) but here I think this feels more like the situation you have in the standard library with Iterator where there are a whole bunch of methods in the trait that return types you can't directly construct, making the methods effectively non-overwritable anyway.

Once thing I'm not sure about is whether using Box<dyn Query<X>> inside of the combinator structs would be worth it? So that we have AndQuery<X> rather than AndQuery<X, Q1, Q2> where Q1: Query<X>, Q2: Query<X>. The dynamic dispatch side of things is another lookup but compared to the interactions with the X server to actually run the queries I very much doubt that it will be having any noticeable performance impact, and the resulting API would be a lot nicer to work with.

@favilo do you have any preference on that side of things?

[favilo]

Hmm. The idea of just AndQuery<X> is pretty nice. I can get behind internal dynamic dispatch. That would make the public API slightly less confusing for people new to rust.

And that's a great point about not being able to construct the types outside of this crate. You've convinced me that we don't need an extension trait.

I'll go ahead and make that change tomorrow morning.

[sminez]

Looks good to go after addressing the two comments! 🙂

The larger point to decide on is explicit generics vs trait objects for the wrapped queries. Happy to discuss here in the PR comments or if you'd prefer we can use the discord and then summarise what we decide here for future reference.

[sminez]

Looking good! Thank you for this @favilo 😄