[rkward/work/kf5_fix_device_embedding] /: Start working on fixing R native device embedding on X11. Code is still very hacky, but now works most of the time.

Thomas Friedrichsmeier thomas.friedrichsmeier at ruhr-uni-bochum.de
Sun Feb 14 21:22:02 UTC 2016


Git commit e85ae358ef8143bf3466dd6d77d6d614d5141ab2 by Thomas Friedrichsmeier.
Committed on 14/02/2016 at 21:19.
Pushed by tfry into branch 'work/kf5_fix_device_embedding'.

Start working on fixing R native device embedding on X11. Code is still very hacky, but now works most of the time.
Apparently there are some synchronization problems, which cause the window to not be actually embedded.
Currently working around those with timeouts, hoping to arrive at a better solution, step-by-step.

M  +1    -0    TODO
M  +6    -15   rkward/rbackend/rinterface.cpp
M  +54   -72   rkward/windows/rkwindowcatcher.cpp
M  +6    -11   rkward/windows/rkwindowcatcher.h
M  +1    -1    rkward/windows/rkworkplace.cpp
M  +2    -1    rkward/windows/rkworkplace.h

http://commits.kde.org/rkward/e85ae358ef8143bf3466dd6d77d6d614d5141ab2

diff --git a/TODO b/TODO
index 0d82fcc..68a2071 100644
--- a/TODO
+++ b/TODO
@@ -9,6 +9,7 @@ Things to do:
 - Device window embedding / remove qwinhost
 - Grep for "KF5 TODO"
 - Remove katepart status bar from all previews (including in wizard)
+- Replace Q_WS_* and Q_OS_X11 as appropriate
 Things to test at the end:
 - Everthing concerning loading / saving, from recent files, scripts, workspace, etc.
   - Moved installations, moved workspaces.
diff --git a/rkward/rbackend/rinterface.cpp b/rkward/rbackend/rinterface.cpp
index 5964aae..7058fc9 100644
--- a/rkward/rbackend/rinterface.cpp
+++ b/rkward/rbackend/rinterface.cpp
@@ -48,11 +48,6 @@
 
 #include "../windows/rkwindowcatcher.h"
 
-#ifndef DISABLE_RKWINDOWCATCHER
-// putting this here instead of the class-header so I'm able to mess with it often without long recompiles. Fix when it works!
-RKWindowCatcher *window_catcher;
-#endif // DISABLE_RKWINDOWCATCHER
-
 #include "../rkglobals.h"
 #include "../version.h"
 #include "../debug.h"
@@ -81,12 +76,8 @@ int RInterface::na_int;
 
 RInterface::RInterface () {
 	RK_TRACE (RBACKEND);
-	
-#ifndef DISABLE_RKWINDOWCATCHER
-	window_catcher = new RKWindowCatcher ();
-#endif // DISABLE_RKWINDOWCATCHER
 
-// If R_HOME is not set, most certainly the user called the binary without the wrapper script
+	// If R_HOME is not set, most certainly the user called the binary without the wrapper script
 	if (!getenv ("R_HOME")) {
 		RK_DEBUG (RBACKEND, DL_ERROR, "No R_HOME environment variable set. RKWard will quit in a moment. Always start rkward in the default way unless you know what you're doing.");
 	}
@@ -126,7 +117,7 @@ RInterface::~RInterface(){
 	RK_TRACE (RBACKEND);
 
 	if (num_active_output_record_requests) RK_DEBUG (RBACKEND, DL_WARNING, "%d requests for recording output still active on interface shutdown", num_active_output_record_requests);
-	delete window_catcher;
+	RKWindowCatcher::discardInstance ();
 }
 
 bool RInterface::backendIsIdle () {
@@ -727,17 +718,17 @@ void RInterface::processHistoricalSubstackRequest (const QStringList &calllist)
 	// NOTE: WARNING: When converting these to PlainGenericRequests, the occasional "error, figure margins too large" starts coming up, again. Not sure, why.
  	} else if (call == "startOpenX11") {
 		RK_ASSERT (calllist.count () == 2);
-		window_catcher->start (calllist.value (1).toInt ());
+		RKWindowCatcher::instance ()->start (calllist.value (1).toInt ());
  	} else if (call == "endOpenX11") {
 		RK_ASSERT (calllist.count () == 2);
-		window_catcher->stop (calllist.value (1).toInt ());
+		RKWindowCatcher::instance ()->stop (calllist.value (1).toInt ());
 	} else if (call == "updateDeviceHistory") {
 		if (calllist.count () >= 2) {
-			window_catcher->updateHistory (calllist.mid (1));
+			RKWindowCatcher::instance ()->updateHistory (calllist.mid (1));
 		}
 	} else if (call == "killDevice") {
 		RK_ASSERT (calllist.count () == 2);
-		window_catcher->killDevice (calllist.value (1).toInt ());
+		RKWindowCatcher::instance ()->killDevice (calllist.value (1).toInt ());
 #endif // DISABLE_RKWINDOWCATCHER
 	} else if (call == "edit") {
 		RK_ASSERT (calllist.count () >= 2);
diff --git a/rkward/windows/rkwindowcatcher.cpp b/rkward/windows/rkwindowcatcher.cpp
index 0afd545..978ab5d 100644
--- a/rkward/windows/rkwindowcatcher.cpp
+++ b/rkward/windows/rkwindowcatcher.cpp
@@ -25,6 +25,7 @@
 #include <QPushButton>
 #include <QDialogButtonBox>
 #include <QDialog>
+#include <QWindow>
 
 #include <kmessagebox.h>
 #include <klocale.h>
@@ -37,6 +38,15 @@
 #include "../misc/rkstandardicons.h"
 #include "../debug.h"
 
+RKWindowCatcher *RKWindowCatcher::_instance = 0;
+RKWindowCatcher* RKWindowCatcher::instance () {
+	if (!_instance) {
+		RK_TRACE (MISC);
+		_instance = new RKWindowCatcher ();
+	}
+	return _instance;
+}
+
 RKWindowCatcher::RKWindowCatcher () : QObject () {
 	RK_TRACE (MISC);
 }
@@ -60,21 +70,22 @@ void RKWindowCatcher::stop (int new_cur_device) {
 
 	if (!created_window) {
 		// we did not see the window, yet? Maybe the event simply hasn't been processed, yet.
+		qApp->sync ();
 		qApp->processEvents ();
 	}
 	disconnect (KWindowSystem::self(), &KWindowSystem::windowAdded, this, &RKWindowCatcher::windowAdded);
 
 	if (new_cur_device != last_cur_device) {
 		if (created_window) {
-			RKWorkplace::mainWorkplace ()->newX11Window (created_window, new_cur_device);
-#if defined Q_WS_X11
-			// All this syncing looks like a bloody hack? Absolutely. It appears to work around the occasional error "figure margins too large" from R, though.
-			qApp->processEvents ();
-			qApp->syncX ();
-			qApp->processEvents ();
+			qDebug ("using %x", created_window);
+			//qApp->sync ();
 			// this appears to have the side-effect of forcing the captured window to sync with X, which is exactly, what we're trying to achieve.
 			KWindowInfo wininfo = KWindowSystem::windowInfo (created_window, NET::WMName | NET::WMGeometry);
-#endif
+			QWindow *window = QWindow::fromWinId (created_window);
+			RKWorkplace::mainWorkplace ()->newX11Window (window, new_cur_device);
+			// All this syncing looks like a bloody hack? Absolutely. It appears to work around the occasional error "figure margins too large" from R, though.
+			//qApp->processEvents ();
+			//qApp->processEvents ();
 		} else {
 #if defined Q_OS_MAC
 			KMessageBox::information (0, i18n ("You have tried to embed a new R graphics device window in RKWard. However, this is not currently supported in this build of RKWard on Mac OS X. See http://rkward.kde.org/mac for more information."), i18n ("Could not embed R X11 window"), "embed_x11_device_not_supported");
@@ -91,6 +102,7 @@ void RKWindowCatcher::windowAdded (WId id) {
 
 	// KF5 TODO: Note: Previously, on windows we checked for IsWindow (hwnd) and IsWindowVisible (hwnd), as sometimes invisible ghost windows
 	// would be created in addition to the real device window. Is this still needed?
+	// A whole lot of windows appear to get created, but it does look like the last one is the one we need.
 	created_window = id;
 }
 
@@ -103,6 +115,7 @@ void RKWindowCatcher::windowChanged (WId id, NET::Properties properties, NET::Pr
 	watcher->setCaption (KWindowInfo (id, NET::WMName).name ());
 }
 
+// KF5 TODO: should no longer be needed. See QWindow::windowTitleChanged()
 void RKWindowCatcher::registerNameWatcher (WId watched, RKMDIWindow *watcher) {
 	RK_TRACE (APP);
 	RK_ASSERT (!name_watchers_list.contains (watched));
@@ -149,7 +162,7 @@ void RKWindowCatcher::killDevice (int device_number) {
 		window->setKilledInR ();
 		window->close (true);
 		// KF5 TODO: Still needed?
-		//QApplication::syncX ();
+		QApplication::sync ();
 	}
 }
 
@@ -161,12 +174,10 @@ void RKWindowCatcher::killDevice (int device_number) {
 #include <QScrollArea>
 #include <qlabel.h>
 #ifdef Q_OS_WIN
+// KF5 TODO: needed?
 #	include "../qwinhost/qwinhost.h"
 #	include <windows.h>
-#elif defined Q_WS_X11
-#	include <QX11EmbedContainer>
 #endif
-#warning TODO: Q_WS_X11 is simply no longer defined. Adjust this functionality.
 #include <QTimer>
 #include <QCloseEvent>
 #include <QSpinBox>
@@ -186,38 +197,29 @@ void RKWindowCatcher::killDevice (int device_number) {
 // static
 QHash<int, RKCaughtX11Window*> RKCaughtX11Window::device_windows;
 
-RKCaughtX11Window::RKCaughtX11Window (WId window_to_embed, int device_number) : RKMDIWindow (0, X11Window) {
+RKCaughtX11Window::RKCaughtX11Window (QWindow* window_to_embed, int device_number) : RKMDIWindow (0, X11Window) {
 	RK_TRACE (MISC);
-
+// TODO: Actually, the WindowCatcher should pass a QWindow*, not WId.
 	commonInit (device_number);
 	embedded = window_to_embed;
 
-#ifdef Q_OS_WIN
-	// unfortunately, trying to get KWindowInfo as below hangs on windows (KDElibs 4.2.3)
-	WINDOWINFO wininfo;
-	wininfo.cbSize = sizeof (WINDOWINFO);
-	GetWindowInfo (embedded, &wininfo);
-
-	// clip off the window frame and menubar
-	xembed_container->setContentsMargins (wininfo.rcWindow.left - wininfo.rcClient.left, wininfo.rcWindow.top - wininfo.rcClient.top,
-				wininfo.rcClient.right - wininfo.rcWindow.right, wininfo.rcClient.bottom - wininfo.rcWindow.bottom);
-	// set a fixed size until the window is shown
-	xembed_container->setFixedSize (wininfo.rcClient.right - wininfo.rcClient.left, wininfo.rcClient.bottom - wininfo.rcClient.top);
-	setGeometry (wininfo.rcClient.left, wininfo.rcClient.right, wininfo.rcClient.top, wininfo.rcClient.bottom);	// see comment in X11 section
-	move (wininfo.rcClient.left, wininfo.rcClient.top);		// else the window frame may be off scree on top/left.
-#elif defined Q_WS_X11
-	KWindowInfo wininfo = KWindowSystem::windowInfo (embedded, NET::WMName | NET::WMGeometry);
+#if !defined Q_OS_MAC
+	KWindowInfo wininfo = KWindowSystem::windowInfo (embedded->winId (), NET::WMName | NET::WMGeometry);
 	RK_ASSERT (wininfo.valid ());
 
 	// set a fixed size until the window is shown
 	xembed_container->setFixedSize (wininfo.geometry ().width (), wininfo.geometry ().height ());
 	setGeometry (wininfo.geometry ());	// it's important to set a size, even while not visible. Else DetachedWindowContainer will assign a default size of 640*480, and then size upwards, if necessary.
 	setCaption (wininfo.name ());
+#else
+	RK_ASSERT (false);
 #endif
 
+	// KF5 TODO: Still needed?
 	// somehow in Qt 4.4.3, when the RKCaughtWindow is reparented the first time, the QX11EmbedContainer may kill its client. Hence we delay the actual embedding until after the window was shown.
 	// In some previous version of Qt, this was not an issue, but I did not track the versions.
-	QTimer::singleShot (0, this, SLOT (doEmbed()));
+	QTimer::singleShot (2000, this, SLOT (doEmbed()));
+	//doEmbed ();
 }
 
 RKCaughtX11Window::RKCaughtX11Window (RKGraphicsDevice* rkward_device, int device_number) : RKMDIWindow (0, X11Window) {
@@ -242,8 +244,8 @@ void RKCaughtX11Window::commonInit (int device_number) {
 	RK_TRACE (MISC);
 
 	capture = 0;
-	rk_native_device = 0;
 	embedded = 0;
+	rk_native_device = 0;
 	killed_in_r = close_attempted = false;
 	RKCaughtX11Window::device_number = device_number;
 	RK_ASSERT (!device_windows.contains (device_number));
@@ -259,14 +261,15 @@ void RKCaughtX11Window::commonInit (int device_number) {
 	QVBoxLayout *layout = new QVBoxLayout (this);
 	layout->setContentsMargins (0, 0, 0, 0);
 	scroll_widget = new QScrollArea (this);
-	scroll_widget->hide ();
 	layout->addWidget (scroll_widget);
 
 	xembed_container = new QWidget (this);	// QX11EmbedContainer can not be reparented (between the this, and the scroll_widget) directly. Therefore we place it into a container, and reparent that instead.
 	// Also, this makes it easier to handle the various different devices
 	QVBoxLayout *xembed_layout = new QVBoxLayout (xembed_container);
 	xembed_layout->setContentsMargins (0, 0, 0, 0);
-	layout->addWidget (xembed_container);
+	//layout->addWidget (xembed_container);
+	scroll_widget->setWidget (xembed_container);
+	xembed_container->hide ();
 
 	dynamic_size = false;
 	dynamic_size_action->setChecked (false);
@@ -276,27 +279,24 @@ void RKCaughtX11Window::doEmbed () {
 	RK_TRACE (MISC);
 
 	if (embedded) {
-#ifdef Q_OS_WIN
-		capture = new QWinHost (xembed_container);
-		capture->setWindow (embedded);
-		capture->setFocusPolicy (Qt::ClickFocus);
-		capture->setAutoDestruct (true);
-		connect (capture, &QWidget::clientDestroyed, this, &RKCaughtX11Window::deleteLater, Qt::QueuedConnection);
-		connect (capture, &QWidget::clientTitleChanged, this, &RKCaughtX11Window::setCaption, Qt::QueuedConnection);
-
-		setCaption (capture->getClientTitle ());
-#elif defined Q_WS_X11
-		capture = new QX11EmbedContainer (xembed_container);
-		capture->embedClient (embedded);
-		connect (capture, &QWidget::clientClosed, this, &RKCaughtX11Window::deleteLater);
-
-		RKWindowCatcher::registerNameWatcher (embedded, this);
-#endif
+/*		if (capture) {
+			embedded->setParent (0);
+			capture->deleteLater ();
+		} */
+		qApp->sync ();
+		KWindowInfo wininfo = KWindowSystem::windowInfo (embedded->winId (), NET::WMName | NET::WMGeometry);
+		capture = QWidget::createWindowContainer (embedded, xembed_container);
+		xembed_container->layout ()->addWidget (capture);
+		QTimer::singleShot (100, xembed_container, SLOT (show ()));
+		// KF5 TODO: Will this detect closed device, correclty? No, will probably need assistance from KWindowSystem.
+		connect (embedded, &QObject::destroyed, this, &RKCaughtX11Window::deleteLater);
+		// KF5 TODO: Window caption?
 	}
+
 	if (!isAttached ()) {
 		// make xembed_container resizable, again, now that it actually has a content
 		dynamic_size_action->setChecked (true);
-		fixedSizeToggled ();
+		QTimer::singleShot (1000, this, SLOT (fixedSizeToggled ()));
 	}
 
 	// try to be helpful when the window is too large to fit on screen
@@ -313,9 +313,7 @@ RKCaughtX11Window::~RKCaughtX11Window () {
 	device_windows.remove (device_number);
 
 	close (false);
-#ifdef Q_WS_X11
-	if (embedded) RKWindowCatcher::unregisterNameWatcher (embedded);
-#endif
+	if (capture) RKWindowCatcher::instance ()->unregisterNameWatcher (capture->winId ());
 	error_dialog->autoDeleteWhenDone ();
 }
 
@@ -332,13 +330,11 @@ void RKCaughtX11Window::setWindowStyleHint (const QString& hint) {
 
 void RKCaughtX11Window::forceClose () {
 	killed_in_r = true;
-	if (capture) {
-#ifdef Q_WS_X11
-		// HACK: Somehow (R 3.0.0alpha), the X11() window is surpisingly die-hard, if it is not close "the regular way".
+	if (embedded) {
+		// HACK: Somehow (R 3.0.0alpha), the X11() window is surpisingly die-hard, if it is not closed "the regular way".
 		// So we expurge it, and leave the rest to the user.
-		capture->discardClient ();
+		embedded->setParent (0);
 		qApp->processEvents ();
-#endif
 	}
 	RKMDIWindow::close (true);
 }
@@ -369,26 +365,12 @@ bool RKCaughtX11Window::close (bool also_delete) {
 	return false;
 }
 
-void RKCaughtX11Window::reEmbed () {
-	RK_TRACE (MISC);
-
-#ifdef Q_WS_X11
-	if (!capture) return;
-// somehow, since some version of Qt, the QX11EmbedContainer would loose its client while reparenting. This allows us to circumvent the problem.
-	capture->discardClient ();
-	capture->deleteLater ();
-	RKWindowCatcher::unregisterNameWatcher (embedded);
-	QTimer::singleShot (0, this, SLOT(doEmbed()));
-#endif
-}
-
 void RKCaughtX11Window::prepareToBeAttached () {
 	RK_TRACE (MISC);
 
 	dynamic_size_action->setChecked (false);
 	fixedSizeToggled ();
 	dynamic_size_action->setEnabled (false);
-	reEmbed ();
 }
 
 void RKCaughtX11Window::prepareToBeDetached () {
@@ -397,7 +379,6 @@ void RKCaughtX11Window::prepareToBeDetached () {
 	dynamic_size_action->setEnabled (true);
 	dynamic_size_action->setChecked (true);
 	fixedSizeToggled ();
-	reEmbed ();
 }
 
 void RKCaughtX11Window::deviceInteractive (bool interactive, const QString& prompt) {
@@ -425,6 +406,7 @@ void RKCaughtX11Window::stopInteraction () {
 void RKCaughtX11Window::fixedSizeToggled () {
 	RK_TRACE (MISC);
 
+	if (embedded && !capture) return;  // while in the middle of embedding, don't mess with any of this, it seems to cause trouble
 	if (dynamic_size == dynamic_size_action->isChecked ()) return;
 	dynamic_size = dynamic_size_action->isChecked ();
 
diff --git a/rkward/windows/rkwindowcatcher.h b/rkward/windows/rkwindowcatcher.h
index bd53c3f..32f6ccd 100644
--- a/rkward/windows/rkwindowcatcher.h
+++ b/rkward/windows/rkwindowcatcher.h
@@ -83,12 +83,15 @@ public:
 	void registerNameWatcher (WId watched, RKMDIWindow *watcher);
 	/** remove a watch created with registerNameWatcher */
 	void unregisterNameWatcher (WId watched);
+	static RKWindowCatcher *instance ();
+	static void discardInstance () { delete _instance; };
 private slots:
 	void windowAdded (WId id);
 	void windowChanged (WId id, NET::Properties properties, NET::Properties2 properties2);
 private:
 	int last_cur_device;
 	WId created_window;
+	static RKWindowCatcher* _instance;
 	QMap<WId, RKMDIWindow*> name_watchers_list;
 };
 
@@ -116,7 +119,7 @@ public:
 /** ctor
 @param window_to_embed the Window id of the R X11 device window to embed
 @param device_number the device number corresponding to that window */
-	RKCaughtX11Window (WId window_to_embed, int device_number);
+	RKCaughtX11Window (QWindow* window_to_embed, int device_number);
 	RKCaughtX11Window (RKGraphicsDevice *rkward_device, int device_number);
 /** dtor */
 	~RKCaughtX11Window ();
@@ -171,25 +174,17 @@ private slots:
 private:
 	void forceClose ();
 	void commonInit (int device_number);
-	void reEmbed ();
 	friend class RKCaughtX11WindowPart;	// needs access to the actions
 	int device_number;
 	bool killed_in_r;
 	bool close_attempted;
-	WId embedded;
 	QWidget *xembed_container;
 	QScrollArea *scroll_widget;
 	RKProgressControl *error_dialog;
 
 	static QHash<int, RKCaughtX11Window*> device_windows;
-#ifdef Q_OS_WIN
-	QWinHost *capture;
-#elif defined Q_WS_X11
-	QX11EmbedContainer *capture;
-#else
-	// a dummy to make things compile for now
-	QWidget *capture;
-#endif
+	QWindow *embedded;
+	QWidget *capture;  // The captured window (0, if using an rk native device)
 	RKGraphicsDevice *rk_native_device;
 
 	bool dynamic_size;
diff --git a/rkward/windows/rkworkplace.cpp b/rkward/windows/rkworkplace.cpp
index 6b55f4c..7d38e75 100644
--- a/rkward/windows/rkworkplace.cpp
+++ b/rkward/windows/rkworkplace.cpp
@@ -497,7 +497,7 @@ RKMDIWindow* RKWorkplace::openOutputWindow (const QUrl &url) {
 	return (w);
 }
 
-void RKWorkplace::newX11Window (WId window_to_embed, int device_number) {
+void RKWorkplace::newX11Window (QWindow* window_to_embed, int device_number) {
 	RK_TRACE (APP);
 
 	RKCaughtX11Window *window = new RKCaughtX11Window (window_to_embed, device_number);
diff --git a/rkward/windows/rkworkplace.h b/rkward/windows/rkworkplace.h
index aac3e29..88bfc5f 100644
--- a/rkward/windows/rkworkplace.h
+++ b/rkward/windows/rkworkplace.h
@@ -40,6 +40,7 @@ class RKToolWindowBar;
 class RKMDIWindowHistoryWidget;
 class RKGraphicsDevice;
 class KMessageWidget;
+class QWindow;
 
 #define TOOL_WINDOW_BAR_COUNT 4
 
@@ -114,7 +115,7 @@ public:
 @param url currently ignored! */
 	RKMDIWindow* openOutputWindow (const QUrl &url=QUrl ());
 
-	void newX11Window (WId window_to_embed, int device_number);
+	void newX11Window (QWindow* window_to_embed, int device_number);
 	void newRKWardGraphisWindow (RKGraphicsDevice *dev, int device_number);
 	void newObjectViewer (RObject *object);
 



More information about the rkward-tracker mailing list