[xmonad] darcs patch: broadcastMessage now uses Operations.windows to update...

[David Roundy]

Hi Andrea,

Your understanding sounds fundamentally right.  My concern with the
number of refreshes is a question of how many refreshes are called for
a single message.  Ideally if a single message results in every
workspace layout being modified (which is not an unreasonable
possibility), then it makes sense that refresh (which calls doLayout
on all visible workspaces) should be called just once, rather than
once for each workspace, which could be rather a waste if there are
many workspaces, and doLayout is expensive.

I do not think, however, that the number of refreshes is a critical
issue, since it's "only" an efficiency concern (+ a flickering
concern, which is actually more important), and is pretty easily
fixable without changing anything outside of core.

I don't see that your explanation invalidates any of my suggestions.
Is there a problem that you see with the slightly different approach I
suggested?

David

On Thu, Feb 21, 2008 at 11:36 AM, Andrea Rossato
<andrea.rossato at unibz.it> wrote:
>
> On Thu, Feb 21, 2008 at 09:43:30AM -0500, David Roundy wrote:
>  > I think broadcastMessage needs reworking, and should be reworked somewhat
>  > along the lines of your sendMessageToWorkspace idea that you previously
>  > implemented (but in a buggy way).
>  >
>  > The trouble here is that if any of these doLayouts modify the windowset,
>  > I'm afraid we'll run into trouble as we continue sending the Message to the
>  > other workspaces, but passing those funcitons a W.Workspace that's obsolete
>  > (e.g. maybe its layout changed because of an action by a previous
>  > handleMessage).  This *may* not cause trouble, but I think it's safer to
>  > write
>  >
>  > sendMessageToWorkspaceWithoutNecessarilyRefreshing :: Message a => a -> WorkspaceId -> X ()
>  >
>  > (but ideally with a shorter name)
>  >
>  > and then
>  >
>  > broadcastMessage :: Message a => a -> X ()
>  > broadcastMessage a = withWindowSet $ \ws -> do
>  >     sendMessage a
>  >     mapM_ (sendMessageToWorkspaceWithoutNecessarilyRefreshing a)
>  >           (map W.tag $ (map W.workspace $ W.visible ws) ++ W.hidden ws)
>  >
>  > It seems like with this function, each workspace that exists will get the
>  > message, unless its WorkspaceId changes first.  It's possible that a
>  > workspace will get the message never, or more than once, if its WorkspaceId
>  > changes, but that's almost unavoidable.  But the key feature of this
>  > approach is:  if the state is changed by a handleMessage (including changes
>  > to layouts), then that change will not be lost... unless a layout both
>  > changes the windowset *and* returns a modified layout.
>  >
>  > But the key is that any loss of that sort must happen within a single
>  > sendMessageToWorkspaceWithoutNecessarilyRefreshing call (or in the
>  > sendMessage call).  So those two functions (each of which call a single
>  > handleMessage) are the only two that place a limit on what is "safe" for a
>  > handleMessage to do.  And with just a bit of care they can actually be
>  > pretty forgiving.  As you point out, a handleMessage that returns Nothing
>  > should be able to make *any* X calls, with the worst danger being the
>  > introduction of an infinite loop (a real danger, but also unavoidable).
>  >
>
>  I think I was not able to make myself entirely clear, so I'll
>  elaborate a bit more, also to give you the possibility to spot a
>  mistake in my reasoning. In the meanwhile I'll also try to answer to
>  some of the issues you rose.
>
>  The problem is not whether we can call 'sendMessage' or
>  'broadcastMessage' from within doLayout. We know that doLayout is
>  called within 'windows', and we cannot call anything calling 'windows'
>  inside 'windows'.
>
>  The real issue, I think, can be thus summarized:
>
>  1. why we are able to make *any* X call from within 'handleMessage' if
>    we return Nothing?
>
>  2. what shall we do if 'handleMessage' returns Just a new layout?
>    Shall we do the same for the current, the visible and the hidden
>    layouts?
>
>
>  Let's start with 'broadcastMessage': it gets the windowset and
>  collects each workspace record (that is to say, the tag, the layout
>  and the stack of each workspace, the current, the visible and the
>  hidden ones).
>
>  'broadcastMessage' then uses the layout filed of the workspace record
>  to call 'handleMessage' with it. If 'handleMessage' returns Just a new
>  layout, this new layout is used to update the previously retrieved
>  workspace record, and this workspace record is used to update the
>  windowset component of the X state.
>
>  What happens if, in 'handleMessage', we call 'focus'?
>
>  'focus' will call 'windows' to modify the stack of the workspace
>  record whose layout field is being used to run 'handleMessage'. After
>  that 'windows' will use that very layout field of that workspace
>  record to run doLayout and refresh the screen. 'doLayout' may return a
>  new layout and that field of the workspace record will be updated by
>  runOnWorkspaces.
>
>  Now, all those changes, made within 'handleMessage' can be saved (and
>  are safe) if, *after* returning Nothing the windowset is *not*
>  modified anymore - it had been in 'windows' as we have seen.
>
>  And what is doing 'broadcastMessage' instead? If we return Nothing in
>  'handleMessage' the layout field of the *previously* retrieved
>  workspace record will be pushed back, even if it had been changed in
>  'handleMessage'. And the very *same* happens with the stack!
>  Everything we did in 'handleMessage' is overwritten with the *old*
>  workspace record.
>
>  And if we return Just? 'broadcastMessage' will just update the layout
>  field ('sendMessage', instead, would call 'windows' to run 'doLayout'
>  with the stack and the *new* layout - the right thing to do, I think).
>
>  This is why we cannot call 'focus' and return Just in 'handleMessage'.
>  If we did, the layout returned by 'doLayout' (called by 'windows'
>  called by 'focus' - see above) would be lost because overwritten by
>  the one returned by 'handleMessage'.
>
>  And now a question: if we return Just should we refresh the screen?
>  Definitely yes for the current and visible layouts. I think it should
>  be safe to call 'doLayout' also for hidden layouts.
>
>  Finally: keep in mind that the case we are really interested in, the
>  one in which we call 'focus' in 'handleMessage', is a case in which
>  'windows' is *not* called afterward. Which means that we are not going
>  to increase the number of screen refreshes. Not in this case at least.
>
>  I hope this clarifies a bit my understanding of the issue.
>
>  Cheers,
>  Andrea