Re: [xmonad] my proposal for a Xinerama safe PerWorkspace

On Wed, Feb 27, 2008 at 2:54 PM, David Roundy wrote:

On Wed, Feb 27, 2008 at 12:00:33PM -0500, Brent Yorgey wrote:

...

On Tue, Feb 26, 2008 at 9:27 PM, Spencer Janssen wrote:

...

...

Hi,

this is my proposal for a Xinerama safe PerWorkspace.

It includes a new LayoutClass method:

runLayout :: Workspace WorkspaceId (layout a) a -> Rectangle -> X ([(a, Rectangle)], Maybe (layout a))

This is just some code moved around:

Fri Feb 22 18:58:15 CET 2008 Andrea Rossato < andrea.rossato@unibz.it> * runLayout is now a LayoutClass method and takes the Workspace and

On Fri, Feb 22, 2008 at 07:12:31PM +0100, Andrea Rossato wrote: the screen Rectangle

...

M ./XMonad/Core.hs -7 +7 M ./XMonad/Operations.hs -5 +5

This is the patch to PerWorkspace:

Fri Feb 22 18:59:54 CET 2008 Andrea Rossato <

andrea.rossato@unibz.it>

...

* PerWorkspace: reimplemented using runLayout This way we have a Xinerama safe PerWorkspace and the emptyLayout method for free. M ./XMonad/Layout/PerWorkspace.hs -73 +27

Some lines were left there just because I'm lazy....;)

Cheers, Andrea

I'm not sure that I'm comfortable with this style of extension. Consider the existing pureLayout/doLayout functions in the layout class. Outside consumers of the API can really only use doLayout, because pureLayout often has the garbage default implementation. Here we're just adding another layer, meaning that layout combinators and such can't safely use doLayout anymore, they must be updated to use runLayout. It just seems like this style of API just isn't very nice.

Don, what do you think? Anyone else?

To clarify: "this style of API" is one in which a class contains a chain of several methods, any one of which may be defined, and only one of which may be used. It would be nice if we could enforce this restriction, but alas, we can't.

I find this a useful approach: it eases the creation of new instances, at the expense of introducing an easily-summarized restriction on the use of the class. In fact, it wouldn't be too hard to write a checker that enforces this (by reading the code and seeing if doLayout/pureLayout are ever used apart from an instance definition).

But it's also true that there are other approaches one could use that would be similarly easy. We could define only runLayout in the class and then define helper functions such as runLayoutFromPureLayout or runLayoutFromDoLayout that would allow folks creating new instances or porting old ones to use the simpler API if they like.

...

This provides an elegant solution for PerWorkspace; however, as Spencer points out, it does mean that most instances of LayoutClass (especially those that implement layout combinators of some sort) must be changed to use runLayout instead of doLayout/emptyLayout (note Andrea has already done this I think), and it will become even more unclear to people writing a LayoutClass instance which methods they can/should override.

No, most instances of LayoutClass will be unchanged. It's *users* of LayoutClass that have to be changed, and these are far more rare. The advantage of this approach is that code that merely defines an instance of LayoutClass can be guaranteed not to require updating, nor to have a bug introduced. It's only code that has "LayoutClass l a =>" in it that will need reworking.

I like the runLayout approach, because it unifies and completes the information provided to the layout: it provides all the information we've got about the workspace being displayed, and it won't need to be changed if we start including more information about workspaces (provided we store it in Workspace). I think that's nice. The emptyLayout addition was a much less elegant hack--and had precisely the same downside with regard to breaking layout combinators. In fact, I would assert the rules that must be obeyed by layout combinators are simplified by the introduction of runLayout.

What's the current status on this? Don? Spencer? Summary so far: * Andrea's patch moves runLayout into the LayoutClass, changing the signature slightly so that layouts receive all possible information. * This would necessitate updating all *users* of LayoutClass to use runLayout instead of doLayout/emptyLayout. The patches for these changes are already available. * New instances of LayoutClass can choose whether to implement runLayout, doLayout, or pureLayout, and so on. Existing instances of LayoutClass will not be affected (except those that are also users of it, e.g. LayoutModifier). * This provides a nice solution (IMO the best) for PerWorkspace. Also: * If this patch is applied to the core, I am willing to take responsibility for making sure the contrib library is updated appropriately, and that thorough documentation is made available to guide those wishing to understand the architecture of the LayoutClass. * David and I think this is elegant and should be applied. (See David's prior e-mail for an excellent discussion of some of the issues involved.) * Spencer has voiced discomfort with the style of this change to the API. * Don has not expressed an opinion one way or the other. It would be nice if a decision could be reached on this one way or the other, so we can move ahead with PerWorkspace and other things. I'm not trying to be pushy, just trying to spur things forward in a friendly way. cheers, -Brent