On Tue 04 Dec 2007 at 20:32:48 -0800, Martin Blais wrote:
> This patch fixes a subtle bug where if you move a window in the
> workspace manager that was a window who had been saved as the focus
> window for a workspace, switching to the old workspace would warp into
> the workspace that the window had been moved into.
Yes, I noticed behaviour like that. It was quite mysterious until I read
your patch :-)
With your patch, this loop looks like this:
for (ws = Scr->workSpaceMgr.workSpaceList; ws != NULL; ws = ws->next) {
int mask = 1 << ws->number;
if (oldoccupation & mask) {
if (!(newoccupation & mask)) {
RemoveWindowFromRegion (tmp_win);
if (PlaceWindowInRegion (tmp_win, &final_x, &final_y))
XMoveWindow (dpy, tmp_win->frame, final_x, final_y);
/* If the window being moved workspaces was the window
saved in focus, reset it for that workspace. */
if (Scr->SaveWorkspaceFocus && ws->save_focus == tmp_win) {
ws->save_focus = NULL;
}
}
break;
}
}
Notice the mysterious "break" statement? I'm not quite sure it should be
there, since it limits the main activities in the loop to the first
workspace that a window disappears from.
{{{ aside:
Looking into what this is doing, I discover there is an undocumented
keyword "WindowRegion"
| WINDOW_REGION string DKEYWORD DKEYWORD {
list = AddWindowRegion ($2, $3, $4);
}
win_list
which apparently specifies 1 or more preferred places where windows
should be located.
}}}
The "break" statement could be appropriate for this window region stuff,
since moving the window around multiple times in case the regions have
different allocations in different workspaces.
But unfortunately, for resetting ws->save_focus this will not do.
I see 2 different solutions:
1. create a new loop here to do full checking
(but I don't like loops that loop over "all of" something)
1b. insert the code into some exising, appropriate loop
2. do a check when actually using the value.
That code would then look something like this (workmgr.c:GotoWorkSpace()):
/* If SaveWorkspaceFocus is on, try to restore the focus to the last
window which was focused when we left this workspace. */
if (Scr->SaveWorkspaceFocus && newws->save_focus) {
for (twmWin = &(Scr->TwmRoot); twmWin != NULL; twmWin = twmWin->next) {
if (twmWin == newws->save_focus && OCCUPY(twmWin, newws)) {
^^^^^^^^^^^^^^^^^^^^^^^^^ added
WarpToWindow(twmWin);
break; <<<< also added
}
}
}
I presume the loop over all windows here is needed to make sure the
pointer doesn't point to a now-nonexistent window. Too bad that this is
needed; this can be avoided by solution 1 or 1b, and just to be sure,
also when it is destroyed. The OCCUPY check would then not even be
needed anymore.
Here's a dirty patch which I have so far not tested except that it
compiles. Comments?
#
# old_revision [a8cae659665e273b3e4176d24da78ddfe0e33371]
#
# patch "events.c"
# from [39f3358ff082d672b599845150ceab6806a03603]
# to [6a8eebbbf12517b3adb7b8ddbadfed05bdee067a]
#
# patch "workmgr.c"
# from [8ea886eb10be3f1bdf3ae7010f4eb36cbe9382c5]
# to [4f7d36ae4f863b1a0e2d2096139c3b8f2ef1fef6]
#
============================================================
--- events.c 39f3358ff082d672b599845150ceab6806a03603
+++ events.c 6a8eebbbf12517b3adb7b8ddbadfed05bdee067a
@@ -2178,6 +2178,13 @@ void HandleDestroyNotify(void)
Scr->Focus = (TwmWindow*) NULL;
FocusOnRoot();
}
+ if (Scr->SaveWorkspaceFocus) {
+ struct WorkSpace *ws;
+ for (ws = Scr->workSpaceMgr.workSpaceList; ws != NULL; ws = ws->next) {
+ if (ws->save_focus == Tmp_win)
+ ws->save_focus = NULL;
+ }
+ }
XDeleteContext(dpy, Tmp_win->w, TwmContext);
XDeleteContext(dpy, Tmp_win->w, ScreenContext);
XDeleteContext(dpy, Tmp_win->frame, TwmContext);
============================================================
--- workmgr.c 8ea886eb10be3f1bdf3ae7010f4eb36cbe9382c5
+++ workmgr.c 4f7d36ae4f863b1a0e2d2096139c3b8f2ef1fef6
@@ -624,12 +624,22 @@ void GotoWorkSpace (virtualScreen *vs, W
/* If SaveWorkspaceFocus is on, try to restore the focus to the last
window which was focused when we left this workspace. */
- if ( Scr->SaveWorkspaceFocus && newws->save_focus) {
+ if (Scr->SaveWorkspaceFocus && newws->save_focus) {
+#if 0
for (twmWin = &(Scr->TwmRoot); twmWin != NULL; twmWin = twmWin->next) {
- if (twmWin == newws->save_focus) {
+ if (twmWin == newws->save_focus && OCCUPY(twmWin, newws)) {
WarpToWindow(twmWin);
+ break;
}
}
+#else
+ twmWin = newws->save_focus;
+ if (OCCUPY(twmWin, newws)) { /* check should not even be needed
anymore */
+ WarpToWindow(twmWin);
+ } else {
+ newws->save_focus = NULL;
+ }
+#endif
}
/* keep track of the order of the workspaces across restarts */
@@ -1454,6 +1464,14 @@ void ChangeOccupation (TwmWindow *tmp_wi
RemoveWindowFromRegion (tmp_win);
if (PlaceWindowInRegion (tmp_win, &final_x, &final_y))
XMoveWindow (dpy, tmp_win->frame, final_x, final_y);
+
+#if 0
+ /* If the window being moved workspaces was the window
+ saved in focus, reset it for that workspace. */
+ if (Scr->SaveWorkspaceFocus && ws->save_focus == tmp_win) {
+ ws->save_focus = NULL;
+ }
+#endif
}
break;
}
@@ -1500,10 +1518,14 @@ void ChangeOccupation (TwmWindow *tmp_wi
for (ws = Scr->workSpaceMgr.workSpaceList; ws != NULL; ws = ws->next) {
int mask = 1 << ws->number;
if (changedoccupation & mask) {
- if (newoccupation & mask)
+ if (newoccupation & mask) {
WMapAddToList (tmp_win, ws);
- else
+ } else {
WMapRemoveFromList (tmp_win, ws);
+ if (Scr->SaveWorkspaceFocus && ws->save_focus == tmp_win) {
+ ws->save_focus = NULL;
+ }
+ }
}
}
-Olaf.
--
___ Olaf 'Rhialto' Seibert -- You author it, and I'll reader it.
\X/ rhialto/at/xs4all.nl -- Cetero censeo "authored" delendum esse.
