[education/rkward] rkward: Fix crash related to short-lived graphics windows (in previews)
Thomas Friedrichsmeier
null at kde.org
Mon Jun 24 22:13:02 BST 2024
Git commit 9fdf8e34543c0a5ac7faf71d7c7e8531ee645789 by Thomas Friedrichsmeier.
Committed on 22/06/2024 at 19:38.
Pushed by tfry into branch 'master'.
Fix crash related to short-lived graphics windows (in previews)
M +50 -0 rkward/autotests/core_test.cpp
M +5 -3 rkward/rbackend/rkwarddevice/rkgraphicsdevice.cpp
M +1 -0 rkward/rbackend/rkwarddevice/rkgraphicsdevice.h
M +5 -1 rkward/rbackend/rpackages/rkward/R/internal_graphics.R
M +1 -0 rkward/windows/rkcommandeditorwindow.h
M +5 -1 rkward/windows/rkwindowcatcher.cpp
M +1 -1 rkward/windows/rkwindowcatcher.h
M +1 -2 rkward/windows/rkworkplace.cpp
https://invent.kde.org/education/rkward/-/commit/9fdf8e34543c0a5ac7faf71d7c7e8531ee645789
diff --git a/rkward/autotests/core_test.cpp b/rkward/autotests/core_test.cpp
index 34a6f5ced..2edfd9872 100644
--- a/rkward/autotests/core_test.cpp
+++ b/rkward/autotests/core_test.cpp
@@ -12,6 +12,8 @@ SPDX-License-Identifier: GPL-2.0-or-later
#include <QDir>
#include <QLoggingCategory>
#include <QRegularExpression>
+#include <QTemporaryFile>
+#include <QActionGroup>
#include <KAboutData>
#include <KLocalizedString>
@@ -29,6 +31,7 @@ SPDX-License-Identifier: GPL-2.0-or-later
#include "../core/renvironmentobject.h"
#include "../misc/rkcommonfunctions.h"
#include "../misc/rkcommandlineargs.h"
+#include "../misc/rkxmlguipreviewarea.h"
QElapsedTimer _test_timer;
@@ -380,6 +383,13 @@ private Q_SLOTS:
}
void RKConsoleHistoryTest() {
+ QTemporaryFile oldhist;
+ QTemporaryFile emptyhist;
+ emptyhist.open();
+ emptyhist.close();
+ RInterface::issueCommand(new RCommand("savehistory(" + RObject::rQuote(oldhist.fileName()) + "); loadhistory(" + RObject::rQuote(emptyhist.fileName()) + ")", RCommand::App));
+ waitForAllFinished();
+
# define UNIQUE_STRING "unique_command_string"
auto console = RKConsole::mainConsole();
console->pipeUserCommand("if (FALSE) " UNIQUE_STRING "()");
@@ -389,6 +399,9 @@ private Q_SLOTS:
console->pipeUserCommand("timestamp(prefix=\"" UNIQUE_STRING "\")");
waitForAllFinished();
QCOMPARE(console->commandHistory().filter(UNIQUE_STRING).size(), 3);
+
+ RInterface::issueCommand(new RCommand("loadhistory(" + RObject::rQuote(oldhist.fileName()) + ")", RCommand::App));
+ waitForAllFinished();
}
void RKDeviceTest() {
@@ -412,6 +425,43 @@ private Q_SLOTS:
waitForAllFinished(5000); // priority_command_done must remain in scope until done
}
+ void ScriptWindowTest() {
+ QCOMPARE(RKWorkplace::mainWorkplace()->getObjectList(RKMDIWindow::CommandEditorWindow).size(), 0);
+
+ // pretty basic check: don't crash or assert on opening script window
+ QTemporaryFile f;
+ f.open();
+ f.write("plot(1,1)\n"); // Using a plot(), here is interesting in that it a) allows a plot preview b) a plot will also be generated, and
+ // immediately discarded for R console previews, which used to be prone to crashing
+ f.close();
+ auto w = RKWorkplace::mainWorkplace()->openScriptEditor(QUrl::fromLocalFile(f.fileName()));
+ auto wins = RKWorkplace::mainWorkplace()->getObjectList(RKMDIWindow::CommandEditorWindow);
+ QCOMPARE(wins.size(), 1);
+ auto win = qobject_cast<RKCommandEditorWindow*>(wins[0]);
+ QVERIFY(win == w);
+ // opening the same url again shall re-use the window
+ auto w2 = RKWorkplace::mainWorkplace()->openScriptEditor(QUrl::fromLocalFile(f.fileName()));
+ QVERIFY(win == w2);
+
+ // pretty basic check: don't crash or assert on switching between previews
+ // NOTE: first action is "no preview"
+ auto actions = win->preview_modes->actions();
+ QVERIFY(actions.size() > 4);
+ for (int i = actions.size() - 1; i >= 0; --i) {
+ auto a = actions[i];
+ if (a->isCheckable()) {
+ qDebug("action %s", qPrintable(a->text()));
+ a->trigger(); // NOTE: Using setChecked(true), here, would not emit the require QActionGroup::triggered() inside RKCommandEditorWindow
+ QVERIFY(a->isChecked());
+ win->doRenderPreview(); // don't wait for debounce timeout
+ waitForAllFinished();
+ // TODO: check that a preview was actually generated
+ }
+ }
+ win->close(RKMDIWindow::NoAskSaveModified);
+ waitForAllFinished();
+ }
+
void restartRBackend() {
RInterface::issueCommand(new RCommand("x <- 1", RCommand::User));
waitForAllFinished();
diff --git a/rkward/rbackend/rkwarddevice/rkgraphicsdevice.cpp b/rkward/rbackend/rkwarddevice/rkgraphicsdevice.cpp
index 5d9ff19c6..20b61bffb 100644
--- a/rkward/rbackend/rkwarddevice/rkgraphicsdevice.cpp
+++ b/rkward/rbackend/rkwarddevice/rkgraphicsdevice.cpp
@@ -212,10 +212,12 @@ RKGraphicsDevice* RKGraphicsDevice::newDevice (int devnum, double width, double
}
void RKGraphicsDevice::closeDevice (int devnum) {
- RK_TRACE (GRAPHICS_DEVICE);
+ RK_TRACE(GRAPHICS_DEVICE);
- RK_ASSERT (devices.contains (devnum));
- devices.take (devnum)->deleteLater ();
+ RK_ASSERT(devices.contains (devnum));
+ auto dev = devices.take(devnum);
+ dev->deleteLater();
+ Q_EMIT dev->deviceClosed(devnum);
}
void RKGraphicsDevice::clear(const QBrush& brush) {
diff --git a/rkward/rbackend/rkwarddevice/rkgraphicsdevice.h b/rkward/rbackend/rkwarddevice/rkgraphicsdevice.h
index 8900982b2..220b38d9e 100644
--- a/rkward/rbackend/rkwarddevice/rkgraphicsdevice.h
+++ b/rkward/rbackend/rkwarddevice/rkgraphicsdevice.h
@@ -113,6 +113,7 @@ Q_SIGNALS:
void locatorDone (bool ok, double x, double y);
void newPageConfirmDone (bool accepted);
void captionChanged (const QString &caption);
+ void deviceClosed(int devnum);
private Q_SLOTS:
void updateNow ();
void newPageDialogDone (int result);
diff --git a/rkward/rbackend/rpackages/rkward/R/internal_graphics.R b/rkward/rbackend/rpackages/rkward/R/internal_graphics.R
index 609cb6df9..a9898921b 100644
--- a/rkward/rbackend/rpackages/rkward/R/internal_graphics.R
+++ b/rkward/rbackend/rpackages/rkward/R/internal_graphics.R
@@ -60,7 +60,11 @@ assign(".rk.preview.devices", list (), envir=.rk.variables)
a <- .rk.variables$.rk.preview.devices[[x]]
if (!is.null (a)) {
if (a$devnum %in% dev.list ()) {
- dev.off (a$devnum)
+ # It is actually important to fetch dev.off() from grDevices-environment, rather than just using dev.off().
+ # The latter may apparently be inlined to be the default version, which does not notify the frontend about the killed device.
+ # TODO: That actually signifies, our procedure of replacing dev.off() is hitting problems at last: We might miss that
+ # call, if it is wrapped in packages.
+ grDevices::dev.off(a$devnum)
}
.rk.variables$.rk.preview.devices[[x]] <- NULL
}
diff --git a/rkward/windows/rkcommandeditorwindow.h b/rkward/windows/rkcommandeditorwindow.h
index 98047bfb9..8b5f0a3f0 100644
--- a/rkward/windows/rkcommandeditorwindow.h
+++ b/rkward/windows/rkcommandeditorwindow.h
@@ -160,6 +160,7 @@ private Q_SLOTS:
/** document was saved. Update preview, if appropriate */
void documentSaved ();
private:
+friend class RKWardCoreTest;
void urlChanged();
KTextEditor::Cursor saved_scroll_position;
KTextEditor::Document *m_doc;
diff --git a/rkward/windows/rkwindowcatcher.cpp b/rkward/windows/rkwindowcatcher.cpp
index 5133fe2cb..001e70592 100644
--- a/rkward/windows/rkwindowcatcher.cpp
+++ b/rkward/windows/rkwindowcatcher.cpp
@@ -282,6 +282,7 @@ RKCaughtX11Window::RKCaughtX11Window(RKGraphicsDevice* rkward_device, int device
xembed_container->layout ()->addWidget (rk_native_device->viewPort ());
connect (rkward_device, &RKGraphicsDevice::captionChanged, this, &RKCaughtX11Window::setCaption);
connect (rkward_device, &RKGraphicsDevice::goingInteractive, this, &RKCaughtX11Window::deviceInteractive);
+ connect(rkward_device, &RKGraphicsDevice::deviceClosed, this, [](int devnum) { RKWindowCatcher::instance()->killDevice(devnum); });
stop_interaction->setVisible (true);
stop_interaction->setEnabled (false);
setCaption (rkward_device->viewPort ()->windowTitle ());
@@ -382,7 +383,10 @@ RKCaughtX11Window::~RKCaughtX11Window () {
void RKCaughtX11Window::commonClose(bool in_destructor) {
RK_TRACE(MISC);
- if (rk_native_device) rk_native_device->stopInteraction();
+ if (rk_native_device) {
+ rk_native_device->stopInteraction();
+ rk_native_device = nullptr;
+ }
QString status = i18n("Closing device (saving history)");
if (!(close_attempted || killed_in_r)) {
diff --git a/rkward/windows/rkwindowcatcher.h b/rkward/windows/rkwindowcatcher.h
index ae6d93173..636fc60e6 100644
--- a/rkward/windows/rkwindowcatcher.h
+++ b/rkward/windows/rkwindowcatcher.h
@@ -114,7 +114,7 @@ public:
RKCaughtX11Window (QWindow* window_to_embed, int device_number);
RKCaughtX11Window (RKGraphicsDevice *rkward_device, int device_number);
/** dtor */
- ~RKCaughtX11Window ();
+ ~RKCaughtX11Window() override;
/** reimplemented from RKMDIWindow to switch to fixed size mode, and disable the dynamic_size_action */
void prepareToBeAttached () override;
diff --git a/rkward/windows/rkworkplace.cpp b/rkward/windows/rkworkplace.cpp
index d93ece5be..6edfe2895 100644
--- a/rkward/windows/rkworkplace.cpp
+++ b/rkward/windows/rkworkplace.cpp
@@ -239,7 +239,6 @@ void RKWorkplace::detachWindow (RKMDIWindow *window, bool was_attached) {
void RKWorkplace::setWindowNotManaged(RKMDIWindow* window) {
RK_TRACE (APP);
- RK_ASSERT(window->state == RKMDIWindow::Attached);
window->state = RKMDIWindow::Detached;
}
@@ -277,7 +276,7 @@ void RKWorkplace::addWindow (RKMDIWindow *window, bool attached) {
if (nw.window) { // kill existing window (going to be replaced)
// TODO: this is not really elegant, yet, as it will change tab-placement (for attached windows), and discard / recreate container (for detached windows)
disconnect (nw.window, &QObject::destroyed, this, &RKWorkplace::namedWindowDestroyed);
- nw.window->deleteLater ();
+ nw.window->close(RKMDIWindow::NoAskSaveModified);
}
nw.window = window;
connect (nw.window, &QObject::destroyed, this, &RKWorkplace::namedWindowDestroyed);
More information about the rkward-tracker
mailing list