more minor code comments on StackSet.hs
Here are some minor comments on the code in StackSet.hs, which I hope will be helpful to the authors. Please excuse my extreme anal-retentiveness ;-) In general, the code is very nice and very well-commented. I have some gripes with the comments, though: -- As is pointed out in the comments, what "master" means needs to be spelled out. I use full/tabbed tiling only and I really don't understand the master concept very well, or how it relates to the data structures (especially since it's not explicitly represented in them). -- Polymorphic data types (StackSet, Screen, Workspace) could do with comments specifying what the type parameters refer to. Sometimes it's obvious, sometimes less so. -- The comments are sometimes confusing wrt the complexity metrics. O(n), O(s), O(w) etc. are not always very intuitive in terms of what n, s, or w are supposed to be. Seemingly "s" should be "screen" and "w" should be "workspace" or "window" but it isn't always that clear. For instance, "new" says it's O(n) and then it turns out that "n" is really "m". "index" is O(s) even though it calls "integrate" which is O(n), and the focus/swap functions are O(w) (worst-case) where "w" means "window". -- For "differentiate" the comment "texture a list" is not very illuminating. Other comments: -- I find the way that Screen wraps a workspace somewhat icky, though I can't think of a better alternative. It feels like it compromises the symmetry of the StackSet data structure. Maybe I'm just being too picky here. -- Why do we need the StackOrNot type alias? What's wrong with Maybe (Stack a)? IMO the latter is more readable, or more readable when you use Maybe monad functions. -- I think the "member" function would be simpler as
member :: Eq a => a -> StackSet i a s sd -> Bool member a s = isJust $ findIndex a s
instead of
member a s = maybe False (const True) (findIndex a s)
-- I think the "insertUp" function would read better as
insertUp :: Eq a => a -> StackSet i a s sd -> StackSet i a s sd insertUp a s = if member a s then s else insert s where insert = modify (Just $ Stack a [] []) (\(Stack t l r) -> Just $ Stack a l (t:r))
instead of
insertUp :: Eq a => a -> StackSet i a s sd -> StackSet i a s sd insertUp a s = if member a s then s else insert where insert = modify (Just $ Stack a [] []) (\(Stack t l r) -> Just $ Stack a l (t:r)) s
since in the latter case "insert" looks like a function but isn't. Again, these are tiny, tiny issues. Overall the code is really nice. Mike
On Tue, Aug 07, 2007 at 04:57:30PM -0700, Michael Vanier wrote:
Other comments:
-- I find the way that Screen wraps a workspace somewhat icky, though I can't think of a better alternative. It feels like it compromises the symmetry of the StackSet data structure. Maybe I'm just being too picky here.
Agreed, it's very ugly. More symmetric options don't seem to be very popular, thought. It seems to me that the "right" way to do things is to use hierarchical Stacks of one sort or another, and handle the arranging of workspaces on screens in a manner symmetrical to the handling of windows on workspaces (and in a Layout like Combo, or when there are float windows forming a sub-workspace, the handling of sub-workspaces within workspaces). It seems best to me to have a single data structure which is used to describe "a set of things, one of which has focus." Currently, we've got two: StackSet describes a set of Screens, one of which has focus, and Stack describes a set of Windows, one of which has focus. StackSet also describes a set of Workspaces, one of which has focus implicitly, via the mapping between Screens and Workspaces.
-- Why do we need the StackOrNot type alias? What's wrong with Maybe (Stack a)? IMO the latter is more readable, or more readable when you use Maybe monad functions.
It's my fault, and in retrospect it wasn't such a hot idea. My thought was that StackOrNot would indicate that Nothing doesn't indicate an error, just that there are no windows present. I wouldn't object to a patch removing StackOrNot. -- David Roundy Department of Physics Oregon State University
On 8/7/07, David Roundy
On Tue, Aug 07, 2007 at 04:57:30PM -0700, Michael Vanier wrote:
Other comments:
-- I find the way that Screen wraps a workspace somewhat icky, though I can't think of a better alternative. It feels like it compromises the symmetry of the StackSet data structure. Maybe I'm just being too picky here.
Agreed, it's very ugly. More symmetric options don't seem to be very popular, thought. It seems to me that the "right" way to do things is to use hierarchical Stacks of one sort or another, and handle the arranging of workspaces on screens in a manner symmetrical to the handling of windows on workspaces (and in a Layout like Combo, or when there are float windows forming a sub-workspace, the handling of sub-workspaces within workspaces).
It seems best to me to have a single data structure which is used to describe "a set of things, one of which has focus." Currently, we've got two: StackSet describes a set of Screens, one of which has focus, and Stack describes a set of Windows, one of which has focus. StackSet also describes a set of Workspaces, one of which has focus implicitly, via the mapping between Screens and Workspaces.
Yes, I've recently been thinking about this as well. I'd love to see it done - it would be quite elegant. Rather than representing the actual screens, the members of the screen level stackset would be workspaces, much like the way windows are members of workspaces. The main problem with this is that there can be more workspaces than there are screens, so the code switching between screens would have to wrap before the actual end of the stackset. Things like this should probably be figured out soon, so that it doesn't break as many contrib modules. Maybe the release after the next? (I think there's one coming soon). --Michael Sloan
participants (3)
-
David Roundy -
mgsloan -
Michael Vanier