Fix Xfixes event handling

Calling the callback on receptions of an EXTERNAL xfixes event was a weird idea from the
start. However, since adding the protection trap around it it made no sense at all because
it effectivly made the callback a noop for that case.

So let's drop all the trap nonsense and implement it properly. The callback will only be
used for actions by internal clients.

5 files changed, 137 insertions, 148 deletions

diff --git a/nx-X11/programs/Xserver/hw/nxagent/Clipboard.c b/nx-X11/programs/Xserver/hw/nxagent/Clipboard.c
index 20a380817..4a0bf9cb5 100644
--- a/nx-X11/programs/Xserver/hw/nxagent/Clipboard.c
+++ b/nx-X11/programs/Xserver/hw/nxagent/Clipboard.c
@@ -873,14 +873,13 @@ void invalidateTargetCaches(void)
 /*
  * This is called from Events.c dispatch loop on reception of a
  * SelectionClear event. We receive this event if someone on the real
- * X server claims the selection ownership.
+ * X server claims the selection ownership we have/had.
+ * Three versions of this routine with different parameter types.
  */
-void nxagentHandleSelectionClearFromXServer(XEvent *X)
+void nxagentHandleSelectionClearFromXServerByIndex(int index)
 {
   #ifdef DEBUG
-  fprintf(stderr, "---------\n%s: SelectionClear event for selection [%lu][%s] window [0x%lx] time [%lu].\n",
-              __func__, X->xselectionclear.selection, NameForRemoteAtom(X->xselectionclear.selection),
-                  X->xselectionclear.window, X->xselectionclear.time);
+  fprintf(stderr, "%s: SelectionClear event for selection index [%u].\n", __func__, index);
   #endif
 
   if (!agentClipboardInitialized)
@@ -899,43 +898,56 @@ void nxagentHandleSelectionClearFromXServer(XEvent *X)
     return;
   }
 
-  int index = nxagentFindRemoteSelectionIndex(X->xselectionclear.selection);
-  if (index != -1)
+  if (IS_LOCAL_OWNER(index))
   {
-    if (IS_LOCAL_OWNER(index))
-    {
-      /* Send a SelectionClear event to (our) previous owner. */
-      xEvent x = {0};
-      x.u.u.type = SelectionClear;
-      x.u.selectionClear.time = GetTimeInMillis();
-      x.u.selectionClear.window = lastSelectionOwner[index].window;
-      x.u.selectionClear.atom = CurrentSelections[index].selection;
+    /* Send a SelectionClear event to (our) previous owner. */
+    xEvent x = {0};
+    x.u.u.type = SelectionClear;
+    x.u.selectionClear.time = GetTimeInMillis();
+    x.u.selectionClear.window = lastSelectionOwner[index].window;
+    x.u.selectionClear.atom = CurrentSelections[index].selection;
 
-      sendEventToClient(lastSelectionOwner[index].client, &x);
+    sendEventToClient(lastSelectionOwner[index].client, &x);
 
-      /*
-       * Set the root window with the NullClient as selection owner. Our
-       * clients asking for the owner via XGetSelectionOwner() will get
-       * this for an answer.
-       */
-      CurrentSelections[index].window = screenInfo.screens[0]->root->drawable.id;
-      CurrentSelections[index].client = NullClient;
+    /*
+     * Set the root window with the NullClient as selection owner. Our
+     * clients asking for the owner via XGetSelectionOwner() will get
+     * this for an answer.
+     */
+    CurrentSelections[index].window = screenInfo.screens[0]->root->drawable.id;
+    CurrentSelections[index].client = NullClient;
 
-      clearSelectionOwnerData(index);
+    clearSelectionOwnerData(index);
 
-      setClientSelectionStage(index, SelectionStageNone);
+    setClientSelectionStage(index, SelectionStageNone);
 
-      invalidateTargetCache(index);
-    }
-    #ifdef DEBUG
-    else
-    {
-      fprintf(stderr, "%s: selection already cleared - doing nothing.\n", __func__);
-    }
-    #endif
+    invalidateTargetCache(index);
+  }
+}
+
+void nxagentHandleSelectionClearFromXServerByAtom(XlibAtom sel)
+{
+  #ifdef DEBUG
+  fprintf(stderr, "---------\n%s: SelectionClear event for remote selection atom [%lu][%s].\n", __func__, sel, NameForRemoteAtom(sel));
+  #endif
+
+  int index = nxagentFindRemoteSelectionIndex(sel);
+  if (index != -1)
+  {
+   nxagentHandleSelectionClearFromXServerByIndex(index);
   }
 }
 
+void nxagentHandleSelectionClearFromXServer(XEvent *X)
+{
+  #ifdef DEBUG
+  fprintf(stderr, "---------\n%s: SelectionClear event for selection [%lu][%s] window [0x%lx] time [%lu].\n",
+              __func__, X->xselectionclear.selection, NameForRemoteAtom(X->xselectionclear.selection),
+                  X->xselectionclear.window, X->xselectionclear.time);
+  #endif
+  nxagentHandleSelectionClearFromXServerByAtom(X->xselectionclear.selection);
+}
+
 /*
  * Send a SelectionNotify event as reply to the RequestSelection
  * event X. If success is True take the property from the event, else
@@ -2202,25 +2214,11 @@ static void resetSelectionOwnerOnXServer(void)
 #ifdef NXAGENT_CLIPBOARD
 
 /*
- * The callback is called from dix. This is the normal operation
- * mode. The callback is also called when nxagent gets XFixes events
- * from the real X server. In that case the Trap is set and the
- * callback will do nothing.
+ * The callback is called from dix.
  */
-
 void nxagentSetSelectionCallback(CallbackListPtr *callbacks, void *data,
                                    void *args)
 {
-  /*
-   * Only act if the trap is unset. The trap indicates that we are
-   * triggered by an XFixes clipboard event originating from the real
-   * X server. In that case we do not want to propagate back changes
-   * to the real X server, because it already knows about them and we
-   * would end up in an infinite loop of events. If there was a better
-   * way to identify that situation during callback processing we
-   * could get rid of the Trap...
-  */
-
   SelectionInfoRec *info = (SelectionInfoRec *)args;
 
   #ifdef DEBUG
@@ -2249,58 +2247,66 @@ void nxagentSetSelectionCallback(CallbackListPtr *callbacks, void *data,
   if (index == -1)
   {
     #ifdef DEBUG
-    fprintf(stderr, "%s: selection [%s] will not be handled by the clipboard code\n", __func__, NameForLocalAtom(pCurSel->selection));
+    fprintf(stderr, "%s: selection [%s] can/will not be handled by the clipboard code\n", __func__, NameForLocalAtom(pCurSel->selection));
     #endif
     return;
   }
 
   /*
-   * Always invalidate the target cache for the relevant selection,
-   * even if the trap is set. This ensures not having invalid data in
-   * the cache.
+   * Always invalidate the target cache for the relevant selection.
+   * This ensures not having invalid data in the cache.
    */
   invalidateTargetCache(index);
 
-  if (nxagentExternalClipboardEventTrap)
-  {
-    #ifdef DEBUG
-    fprintf(stderr, "%s: Trap is set, doing nothing\n", __func__);
-    #endif
-    return;
-  }
-
-
   #ifdef DEBUG
+  fprintf(stderr, "%s: pCurSel->window [0x%x]\n", __func__, pCurSel->window);
+  fprintf(stderr, "%s: pCurSel->pWin [0x%x]\n", __func__, WINDOWID(pCurSel->pWin));
+  fprintf(stderr, "%s: pCurSel->selection [%s]\n", __func__, NameForLocalAtom(pCurSel->selection));
   fprintf(stderr, "%s: pCurSel->lastTimeChanged [%u]\n", __func__, pCurSel->lastTimeChanged.milliseconds);
+  fprintf(stderr, "%s: pCurSel->client [%s]\n", __func__, nxagentClientInfoString(pCurSel->client));
   #endif
 
-  if (info->kind == SelectionSetOwner)
+  if (nxagentOption(Clipboard) != ClipboardNone) /* FIXME: shouldn't we also check for != ClipboardClient? */
   {
-    #ifdef DEBUG
-    fprintf(stderr, "%s: pCurSel->pWin [0x%x]\n", __func__, WINDOWID(pCurSel->pWin));
-    fprintf(stderr, "%s: pCurSel->selection [%s]\n", __func__, NameForLocalAtom(pCurSel->selection));
-    #endif
+    Selection *pSel = NULL;
+    Selection nullSel = {
+                         .client = NullClient,
+                         .window = None,
+                         .pWin = NULL,
+                         .selection = pCurSel->selection,
+                         .lastTimeChanged = pCurSel->lastTimeChanged
+    };
+
+    if (info->kind == SelectionSetOwner)
+    {
+      pSel = pCurSel;
+    }
+    else if (info->kind == SelectionWindowDestroy)
+    {
+      if (pCurSel->window == lastSelectionOwner[index].window)
+      {
+        pSel = &nullSel;
+      }
+    }
+    else if (info->kind == SelectionClientClose)
+    {
+      if (pCurSel->client == lastSelectionOwner[index].client)
+      {
+        pSel = &nullSel;
+      }
+    }
+    else
+    {
+    }
 
-    if (pCurSel->pWin != NULL &&
-        nxagentOption(Clipboard) != ClipboardNone && /* FIXME: shouldn't we also check for != ClipboardClient? */
-        (pCurSel->selection == localSelelectionAtoms[nxagentPrimarySelection] ||
-         pCurSel->selection == localSelelectionAtoms[nxagentClipboardSelection]))
+    if (pSel)
     {
       #ifdef DEBUG
       fprintf(stderr, "%s: calling setSelectionOwnerOnXServer\n", __func__);
       #endif
-      setSelectionOwnerOnXServer(pCurSel);
+      setSelectionOwnerOnXServer(pSel);
     }
   }
-  else if (info->kind == SelectionWindowDestroy)
-  {
-  }
-  else if (info->kind == SelectionClientClose)
-  {
-  }
-  else
-  {
-  }
 }
 #endif
 
diff --git a/nx-X11/programs/Xserver/hw/nxagent/Clipboard.h b/nx-X11/programs/Xserver/hw/nxagent/Clipboard.h
index aca8d94af..4518595d1 100644
--- a/nx-X11/programs/Xserver/hw/nxagent/Clipboard.h
+++ b/nx-X11/programs/Xserver/hw/nxagent/Clipboard.h
@@ -57,11 +57,14 @@ extern void nxagentClearClipboard(ClientPtr pClient, WindowPtr pWindow);
 extern int nxagentConvertSelection(ClientPtr client, WindowPtr pWin, Atom selection,
                                       Window requestor, Atom property, Atom target, Time time);
 
+extern void nxagentHandleSelectionClearFromXServerByIndex(int index);
 #ifdef XEvent
+extern void nxagentHandleSelectionClearFromXServerByAtom(XlibAtom sel);
 extern void nxagentHandleSelectionClearFromXServer(XEvent *X);
 extern void nxagentHandleSelectionRequestFromXServer(XEvent *X);
 extern void nxagentHandleSelectionNotifyFromXServer(XEvent *X);
 #else
+extern void nxagentHandleSelectionClearFromXServerByAtom();
 extern void nxagentHandleSelectionClearFromXServer();
 extern void nxagentHandleSelectionRequestFromXServer();
 extern void nxagentHandleSelectionNotifyFromXServer();
diff --git a/nx-X11/programs/Xserver/hw/nxagent/Events.c b/nx-X11/programs/Xserver/hw/nxagent/Events.c
index 906915ad0..a678e3841 100644
--- a/nx-X11/programs/Xserver/hw/nxagent/Events.c
+++ b/nx-X11/programs/Xserver/hw/nxagent/Events.c
@@ -2841,69 +2841,64 @@ int nxagentHandleXFixesSelectionNotify(XEvent *X)
       return 0;
   }
 
+  #ifdef DEBUG
+  fprintf(stderr, "---------\n");
+  #endif
+
   #ifdef TEST
   fprintf(stderr, "%s: Handling event.\n", __func__);
   #endif
 
-  if (SelectionCallback)
-  {
-    Atom local = nxagentRemoteToLocalAtom(xfixesEvent -> xfixesselection.selection);
-
-    int index = nxagentFindCurrentSelectionIndex(local);
-    if (index != -1)
-    {
-      if (CurrentSelections[index].client != 0)
-      {
-        #ifdef TEST
-        fprintf(stderr, "%s: Doing nothing.\n", __func__);
-        #endif
-
-        return 1;
-      }
-
-      #ifdef TEST
-      fprintf(stderr, "%s: Calling callbacks for %d [%s] selection.\n", __func__,
-                       CurrentSelections[index].selection, NameForAtom(CurrentSelections[index].selection));
-      #endif
+  #ifdef DEBUG
+  fprintf(stderr, "%s: Event timestamp [%ld]\n", __func__, xfixesEvent->xfixesselection.timestamp);
+  fprintf(stderr, "%s: Event selection timestamp [%ld]\n", __func__, xfixesEvent->xfixesselection.selection_timestamp);
+  fprintf(stderr, "%s: Event selection window [0x%lx]\n", __func__, xfixesEvent->xfixesselection.window);
+  fprintf(stderr, "%s: Event selection owner [0x%lx]\n", __func__, xfixesEvent->xfixesselection.owner);
+  fprintf(stderr, "%s: Event selection [%s]\n", __func__, NameForAtom(nxagentRemoteToLocalAtom(xfixesEvent->xfixesselection.selection)));
 
-      #ifdef DEBUG
-      fprintf(stderr, "%s: CurrentSelections[%d].lastTimeChanged [%u]\n", __func__, index, CurrentSelections[index].lastTimeChanged.milliseconds);
-      fprintf(stderr, "%s: Event timestamp [%ld]\n", __func__, xfixesEvent->xfixesselection.timestamp);
-      fprintf(stderr, "%s: Event selection timestamp [%ld]\n", __func__, xfixesEvent->xfixesselection.selection_timestamp);
-      fprintf(stderr, "%s: Event selection window [0x%lx]\n", __func__, xfixesEvent->xfixesselection.window);
-      fprintf(stderr, "%s: Event selection owner [0x%lx]\n", __func__, xfixesEvent->xfixesselection.owner);
-      fprintf(stderr, "%s: Event selection [%s]\n", __func__, NameForAtom(local));
+  fprintf(stderr, "%s: Subtype ", __func__);
 
-      fprintf(stderr, "%s: Subtype ", __func__);
+  switch (xfixesEvent -> xfixesselection.subtype)
+  {
+    case SelectionSetOwner:       fprintf(stderr, "SelectionSetOwner.\n");      break;
+    case SelectionWindowDestroy:  fprintf(stderr, "SelectionWindowDestroy.\n"); break;
+    case SelectionClientClose:    fprintf(stderr, "SelectionClientClose.\n");   break;
+    default:                      fprintf(stderr, ".\n");                       break;
+  }
+  #endif
 
-      switch (xfixesEvent -> xfixesselection.subtype)
-      {
-        case SelectionSetOwner:       fprintf(stderr, "SelectionSetOwner.\n");      break;
-        case SelectionWindowDestroy:  fprintf(stderr, "SelectionWindowDestroy.\n"); break;
-        case SelectionClientClose:    fprintf(stderr, "SelectionClientClose.\n");   break;
-        default:                      fprintf(stderr, ".\n");                       break;
-      }
-      #endif
+  if (xfixesEvent->xfixesselection.owner && xfixesEvent->xfixesselection.owner == nxagentWindow(screenInfo.screens[0]->root))
+  {
+    /*
+     * This is an event that must have been triggered by nxagent itself
+     * - by calling XSetSelectionOwner(). As this is no news for us we
+     * can ignore the event.
+     */
 
-      SelectionInfoRec info = {
-        .selection = &CurrentSelections[index],
-        .kind = xfixesEvent->xfixesselection.subtype
-      };
+    #ifdef DEBUG
+    fprintf(stderr, "%s: (new) owner is nxagent (window is [0x%lx]) - ignoring it.\n", __func__, xfixesEvent->xfixesselection.window);
+    #endif
+    return 0;
+  }
 
-      /*
-       * The trap indicates that we are triggered by a clipboard event
-       * originating from the real X server. In that case we do not
-       * want to propagate back changes to the real X server, because
-       * it already knows about them and we would end up in an
-       * infinite loop of events. If there was a better way to
-       * identify that situation during Callback processing we could
-       * get rid of the Trap...
-       */
-      nxagentExternalClipboardEventTrap = True;
-      CallCallbacks(&SelectionCallback, &info);
-      nxagentExternalClipboardEventTrap = False;
-    }
+  if (xfixesEvent -> xfixesselection.subtype == SelectionSetOwner||
+      xfixesEvent -> xfixesselection.subtype == SelectionWindowDestroy ||
+      xfixesEvent -> xfixesselection.subtype == SelectionClientClose)
+  {
+    /*
+     * Reception of an owner change on the real X server is - for nxagent - the same as
+     * receiving a SelectionClear event. We just need to tell a (possible) internal
+     * owner that is no longer owning the selection.
+     */
+    nxagentHandleSelectionClearFromXServerByAtom(xfixesEvent -> xfixesselection.selection);
+  }
+  else
+  {
+    #ifdef DEBUG
+    fprintf(stderr, "%s: WARNING unexpected xfixesselection subtype [%d]\n", __func__, xfixesEvent -> xfixesselection.subtype);
+    #endif
   }
+
   return 1;
 }
 
diff --git a/nx-X11/programs/Xserver/hw/nxagent/Trap.c b/nx-X11/programs/Xserver/hw/nxagent/Trap.c
index 6eade2073..c0a1fdde0 100644
--- a/nx-X11/programs/Xserver/hw/nxagent/Trap.c
+++ b/nx-X11/programs/Xserver/hw/nxagent/Trap.c
@@ -103,11 +103,3 @@ Bool nxagentXkbCapsTrap = False;
  */
 
 Bool nxagentXkbNumTrap = False;
-
-/*
- * Set to indicate we are processing a clipboard event triggered by
- * the real X server. This is used to avoid endless loops if callbacks
- * would trigger another event by the real X server
- */
-
-Bool nxagentExternalClipboardEventTrap = False;
diff --git a/nx-X11/programs/Xserver/hw/nxagent/Trap.h b/nx-X11/programs/Xserver/hw/nxagent/Trap.h
index 1c10f00ea..02ad48f99 100644
--- a/nx-X11/programs/Xserver/hw/nxagent/Trap.h
+++ b/nx-X11/programs/Xserver/hw/nxagent/Trap.h
@@ -101,11 +101,4 @@ extern Bool nxagentXkbCapsTrap;
  */
 extern Bool nxagentXkbNumTrap;
 
-/*
- * Set to indicate we are processing a clipboard event triggered by
- * the real X server. This is used to avoid endless loops if callbacks
- * would trigger another event by the real X server
- */
-extern Bool nxagentExternalClipboardEventTrap;
-
 #endif /* __Trap_H__ */