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 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