[education/rkward] rkward: Sanitize button behavior in load libs dialog, cleaing up some really old code along the way.
Thomas Friedrichsmeier
null at kde.org
Mon May 2 20:26:24 BST 2022
Git commit 929cdb575212458becb41b976652127accc2db44 by Thomas Friedrichsmeier.
Committed on 02/05/2022 at 19:25.
Pushed by tfry into branch 'master'.
Sanitize button behavior in load libs dialog, cleaing up some really old code along the way.
M +1 -1 rkward/core/robjectlist.cpp
M +2 -2 rkward/core/robjectlist.h
M +47 -93 rkward/dialogs/rkloadlibsdialog.cpp
M +30 -24 rkward/dialogs/rkloadlibsdialog.h
M +0 -1 rkward/misc/rkstandardicons.h
https://invent.kde.org/education/rkward/commit/929cdb575212458becb41b976652127accc2db44
diff --git a/rkward/core/robjectlist.cpp b/rkward/core/robjectlist.cpp
index 4a91c69a..59c6c91f 100644
--- a/rkward/core/robjectlist.cpp
+++ b/rkward/core/robjectlist.cpp
@@ -63,7 +63,7 @@ QString RObjectList::getObjectDescription () const {
return i18n ("This section contains environments that are not part of <i>.GlobalEnv</i> / your \"workspace\". Most importantly, this includes loaded packages, but also objects added to R's <i>search()<i>-path using <i>attach()</i>.");
}
-QStringList RObjectList::detachPackages (const QStringList &packages, RCommandChain *chain, RKProgressControl* control) {
+QStringList RObjectList::detachPackages (const QStringList &packages, RCommandChain *chain, RKInlineProgressControl* control) {
RK_TRACE (OBJECTS);
QStringList remove;
diff --git a/rkward/core/robjectlist.h b/rkward/core/robjectlist.h
index 96857973..6e7ddc0a 100644
--- a/rkward/core/robjectlist.h
+++ b/rkward/core/robjectlist.h
@@ -21,7 +21,7 @@ class RCommand;
class RCommandChain;
class RKEditor;
class REnvironmentObject;
-class RKProgressControl;
+class RKInlineProgressControl;
class RKOrphanNamespacesObject;
/**
@@ -53,7 +53,7 @@ public:
/** detach the given list of packages (if the packages are loaded, and safe to remove)
@returns a list of error messages (usually empty) */
- QStringList detachPackages (const QStringList &packages, RCommandChain *chain = 0, RKProgressControl *control = 0);
+ QStringList detachPackages (const QStringList &packages, RCommandChain *chain = 0, RKInlineProgressControl *control = 0);
/** A pseudo object containing as children all loaded namespaces which do not belong to a package on the search path */
RKOrphanNamespacesObject* orphanNamespacesObject () const { return orphan_namespaces; };
QString getObjectDescription () const override;
diff --git a/rkward/dialogs/rkloadlibsdialog.cpp b/rkward/dialogs/rkloadlibsdialog.cpp
index 35f1423e..b06a1330 100644
--- a/rkward/dialogs/rkloadlibsdialog.cpp
+++ b/rkward/dialogs/rkloadlibsdialog.cpp
@@ -52,7 +52,9 @@ RKLoadLibsDialog::RKLoadLibsDialog (QWidget *parent, RCommandChain *chain, bool
setWindowTitle (i18n ("Configure Packages"));
setStandardButtons (QDialogButtonBox::Apply | QDialogButtonBox::Close);
disconnect(buttonBox(), &QDialogButtonBox::rejected, this, &RKLoadLibsDialog::reject);
- connect(buttonBox()->button(QDialogButtonBox::Close), &QPushButton::clicked, this, &RKLoadLibsDialog::close);
+ connect(button(QDialogButtonBox::Close), &QPushButton::clicked, this, &RKLoadLibsDialog::queryClose);
+ button(QDialogButtonBox::Apply)->setEnabled(false);
+ connect(button(QDialogButtonBox::Apply), &QPushButton::clicked, this, [this]() { button(QDialogButtonBox::Apply)->setEnabled(false); });
LoadUnloadWidget *luwidget = new LoadUnloadWidget (this);
addChild (luwidget, i18n ("Load / Unload R packages"));
@@ -65,8 +67,6 @@ RKLoadLibsDialog::RKLoadLibsDialog (QWidget *parent, RCommandChain *chain, bool
connect (this, &KPageDialog::currentPageChanged, this, &RKLoadLibsDialog::slotPageChanged);
QTimer::singleShot (0, this, SLOT (slotPageChanged()));
- num_child_widgets = 4;
- was_accepted = false;
RCommand *command = new RCommand(".libPaths()", RCommand::App | RCommand::GetStringVector);
connect(command->notifier(), &RCommandNotifier::commandFinished, this, [this](RCommand *command) {
@@ -83,21 +83,35 @@ RKLoadLibsDialog::~RKLoadLibsDialog () {
RK_TRACE (DIALOGS);
}
-KPageWidgetItem* RKLoadLibsDialog::addChild (QWidget *child_page, const QString &caption) {
+void RKLoadLibsDialog::queryClose() {
+ RK_TRACE (DIALOGS);
+
+ bool changes = false;
+ bool do_close = true;
+ for (int i = 0; i < pages.size(); ++i) {
+ changes |= pages[i]->isChanged();
+ }
+ if (changes) {
+ do_close = (KMessageBox::questionYesNo(this, i18n("Closing will discard pending changes. Are you sure?"), i18n("Discard changes?"), KGuiItem("Discard"), KGuiItem("Do no close")) == KMessageBox::Yes);
+ }
+ if (do_close) close();
+}
+
+KPageWidgetItem* RKLoadLibsDialog::addChild(RKLoadLibsDialogPage *child_page, const QString &caption) {
RK_TRACE (DIALOGS);
// TODO: Can't convert these signal/slot connections to new syntax, without creating a common base class for the child pages
- connect (button (QDialogButtonBox::Apply), SIGNAL (clicked(bool)), child_page, SLOT (apply()));
- connect (this, SIGNAL(rejected()), child_page, SLOT (cancel()));
- connect (child_page, &QObject::destroyed, this, &RKLoadLibsDialog::childDeleted);
- return addPage (child_page, caption);
+ connect(button(QDialogButtonBox::Apply), &QPushButton::clicked, child_page, &RKLoadLibsDialogPage::apply);
+ connect(child_page, &RKLoadLibsDialogPage::changed, this, [this]() { button(QDialogButtonBox::Apply)->setEnabled(true); });
+ pages.append(child_page);
+ return addPage(child_page, caption);
}
void RKLoadLibsDialog::slotPageChanged () {
RK_TRACE (DIALOGS);
- if (!currentPage ()) return;
- QTimer::singleShot (0, currentPage ()->widget (), SLOT (activated()));
+ if (!currentPage()) return;
+ QTimer::singleShot(0, dynamic_cast<RKLoadLibsDialogPage*>(currentPage()->widget()), &RKLoadLibsDialogPage::activated);
}
//static
@@ -127,40 +141,10 @@ void RKLoadLibsDialog::automatedInstall(const QStringList &packages) {
install_packages_widget->trySelectPackages(packages);
}
-void RKLoadLibsDialog::tryDestruct () {
- RK_TRACE (DIALOGS);
-
- if (num_child_widgets <= 0) {
- deleteLater ();
- }
-}
-
-void RKLoadLibsDialog::childDeleted () {
- RK_TRACE (DIALOGS);
- --num_child_widgets;
- tryDestruct ();
-}
-
-void RKLoadLibsDialog::closeEvent (QCloseEvent *e) {
- RK_TRACE (DIALOGS);
- e->accept ();
-
- // do as if cancel button was clicked:
- reject ();
-}
-
-void RKLoadLibsDialog::accept () {
- was_accepted = true;
- hide ();
- // will self-destruct once all children are done
- emit accepted();
-}
-
-void RKLoadLibsDialog::reject () {
- was_accepted = false;
- hide ();
- // will self-destruct once all children are done
- emit rejected();
+void RKLoadLibsDialog::closeEvent(QCloseEvent *e) {
+ RK_TRACE(DIALOGS);
+ e->accept();
+ deleteLater();
}
void RKLoadLibsDialog::addLibraryLocation (const QString& new_loc) {
@@ -343,7 +327,7 @@ void RKLoadLibsDialog::runInstallationCommand (const QString& command, bool as_r
////////////////////// LoadUnloadWidget ////////////////////////////
-LoadUnloadWidget::LoadUnloadWidget (RKLoadLibsDialog *dialog) : QWidget (0) {
+LoadUnloadWidget::LoadUnloadWidget (RKLoadLibsDialog *dialog) : RKLoadLibsDialogPage (0) {
RK_TRACE (DIALOGS);
LoadUnloadWidget::parent = dialog;
@@ -473,6 +457,7 @@ void LoadUnloadWidget::loadButtonClicked () {
item->setSelected (true);
}
}
+ setChanged();
}
void LoadUnloadWidget::detachButtonClicked () {
@@ -492,6 +477,7 @@ void LoadUnloadWidget::detachButtonClicked () {
installed[0]->setSelected (true);
}
}
+ setChanged();
}
void LoadUnloadWidget::updateButtons () {
@@ -501,12 +487,6 @@ void LoadUnloadWidget::updateButtons () {
load_button->setEnabled (!installed_view->selectedItems ().isEmpty ());
}
-void LoadUnloadWidget::ok () {
- RK_TRACE (DIALOGS);
- doLoadUnload ();
- deleteLater ();
-}
-
void LoadUnloadWidget::updateCurrentList () {
RK_TRACE (DIALOGS);
@@ -519,8 +499,8 @@ void LoadUnloadWidget::updateCurrentList () {
void LoadUnloadWidget::doLoadUnload () {
RK_TRACE (DIALOGS);
- RKProgressControl *control = new RKProgressControl (this, i18n ("There has been an error while trying to load / unload packages. See transcript below for details"), i18n ("Error while handling packages"), RKProgressControl::DetailedError);
- connect (this, &LoadUnloadWidget::loadUnloadDone, control, &RKProgressControl::done);
+ RKInlineProgressControl *control = new RKInlineProgressControl(this, false);
+ control->setText(i18n("There has been an error while trying to load / unload packages. See transcript below for details"));
// load packages previously not loaded
for (int i = 0; i < loaded_view->topLevelItemCount (); ++i) {
@@ -548,25 +528,22 @@ void LoadUnloadWidget::doLoadUnload () {
// find out, when we're done
RCommand *command = new RCommand(QString(), RCommand::EmptyCommand);
connect(command->notifier(), &RCommandNotifier::commandFinished, this, [this](RCommand *) {
- emit loadUnloadDone();
+ clearChanged();
});
+ control->addRCommand(command); // this is actually important, in case no commands had been generated, above
RInterface::issueCommand(command, parent->chain);
-
- control->doNonModal(true);
+ control->setAutoCloseWhenCommandsDone(true);
+ control->show(100);
}
void LoadUnloadWidget::apply () {
RK_TRACE (DIALOGS);
+ if (!isChanged()) return;
doLoadUnload ();
updateCurrentList ();
}
-void LoadUnloadWidget::cancel () {
- RK_TRACE (DIALOGS);
- deleteLater ();
-}
-
/////////////////////// InstallPackagesWidget //////////////////////////
#include <QHeaderView>
#include <QStyledItemDelegate>
@@ -633,7 +610,7 @@ signals:
void selectAllUpdates();
};
-InstallPackagesWidget::InstallPackagesWidget (RKLoadLibsDialog *dialog) : QWidget (0) {
+InstallPackagesWidget::InstallPackagesWidget (RKLoadLibsDialog *dialog) : RKLoadLibsDialogPage (0) {
RK_TRACE (DIALOGS);
InstallPackagesWidget::parent = dialog;
@@ -697,6 +674,7 @@ InstallPackagesWidget::InstallPackagesWidget (RKLoadLibsDialog *dialog) : QWidge
settingsbox->addWidget(configure_repos_button);
connect(parent, &RKLoadLibsDialog::installedPackagesChanged, this, &InstallPackagesWidget::initialize);
+ connect(packages_status, &RKRPackageInstallationStatus::changed, this, &InstallPackagesWidget::setChanged);
}
InstallPackagesWidget::~InstallPackagesWidget () {
@@ -731,6 +709,7 @@ void InstallPackagesWidget::initialize () {
packages_view->setFirstColumnSpanned (i, QModelIndex (), true);
}
window()->raise(); // needed on Mac, otherwise the dialog may go hiding behind the main app window, after the progress control window closes, for some reason
+ clearChanged();
});
RInterface::issueCommand(dummy, parent->chain);
}
@@ -807,21 +786,10 @@ void InstallPackagesWidget::doInstall() {
void InstallPackagesWidget::apply () {
RK_TRACE (DIALOGS);
+ if (!isChanged()) return;
doInstall();
}
-void InstallPackagesWidget::ok () {
- RK_TRACE (DIALOGS);
-
- doInstall();
- deleteLater();
-}
-
-void InstallPackagesWidget::cancel () {
- RK_TRACE (DIALOGS);
- deleteLater ();
-}
-
void InstallPackagesWidget::configureRepositories () {
RK_TRACE (DIALOGS);
RKSettings::configureSettings (RKSettings::PageRPackages, this, parent->chain);
@@ -1119,6 +1087,7 @@ bool RKRPackageInstallationStatus::setData (const QModelIndex &index, const QVar
emit dataChanged(index, index);
if (bindex.isValid ()) emit dataChanged(bindex, bindex);
+ emit changed();
return true;
}
@@ -1197,10 +1166,9 @@ void RKRPackageInstallationStatusSortFilterModel::setRKWardOnly (bool only) {
/////////////////////////
#include "../misc/multistringselector.h"
-RKPluginMapSelectionWidget::RKPluginMapSelectionWidget (RKLoadLibsDialog* dialog) : QWidget (dialog) {
+RKPluginMapSelectionWidget::RKPluginMapSelectionWidget (RKLoadLibsDialog* dialog) : RKLoadLibsDialogPage(dialog) {
RK_TRACE (DIALOGS);
model = 0;
- changes_pending = false;
QVBoxLayout *vbox = new QVBoxLayout (this);
vbox->setContentsMargins (0, 0, 0, 0);
@@ -1223,34 +1191,20 @@ void RKPluginMapSelectionWidget::activated () {
selector->setModel (model, 1);
connect (selector, &RKMultiStringSelectorV2::insertNewStrings, model, &RKSettingsModulePluginsModel::insertNewStrings);
connect (selector, &RKMultiStringSelectorV2::swapRows, model, &RKSettingsModulePluginsModel::swapRows);
- connect (selector, &RKMultiStringSelectorV2::listChanged, this, &RKPluginMapSelectionWidget::changed);
+ connect (selector, &RKMultiStringSelectorV2::listChanged, this, &RKPluginMapSelectionWidget::setChanged);
}
}
void RKPluginMapSelectionWidget::apply () {
RK_TRACE (DIALOGS);
- if (!changes_pending) return;
+ if (!isChanged()) return;
RK_ASSERT (model);
RKSettingsModulePlugins::PluginMapList new_list = RKSettingsModulePlugins::setPluginMaps (model->pluginMaps ());
selector->setModel (0); // we don't want any extra change notification for this
model->init (new_list);
selector->setModel (model, 1);
- changes_pending = false;
-}
-
-void RKPluginMapSelectionWidget::cancel () {
- RK_TRACE (DIALOGS);
- deleteLater ();
-}
-
-void RKPluginMapSelectionWidget::ok () {
- RK_TRACE (DIALOGS);
-
- if (!changes_pending) return;
- RK_ASSERT (model);
- RKSettingsModulePlugins::setPluginMaps (model->pluginMaps ());
- deleteLater ();
+ clearChanged();
}
#include "rkloadlibsdialog.moc"
diff --git a/rkward/dialogs/rkloadlibsdialog.h b/rkward/dialogs/rkloadlibsdialog.h
index eded2636..0431acbf 100644
--- a/rkward/dialogs/rkloadlibsdialog.h
+++ b/rkward/dialogs/rkloadlibsdialog.h
@@ -31,6 +31,22 @@ class PackageInstallParamsWidget;
class InstallPackagesWidget;
class RKDynamicSearchLine;
+class RKLoadLibsDialogPage : public QWidget {
+ Q_OBJECT
+public:
+ RKLoadLibsDialogPage(QWidget* parent) : QWidget(parent), _changed(false) {};
+ virtual void apply() = 0;
+ virtual void activated() = 0;
+ bool isChanged() const { return _changed; };
+signals:
+ void changed();
+protected:
+ void setChanged() { _changed = true; emit changed(); };
+ void clearChanged() { _changed = false; };
+private:
+ bool _changed;
+};
+
/**
Dialog which excapsulates widgets to load/unload, update and install R packages
@@ -56,22 +72,20 @@ public:
static void showInstallPackagesModal (QWidget *parent, RCommandChain *chain, const QStringList &package_names);
static void showPluginmapConfig (QWidget *parent=0, RCommandChain *chain=0);
QStringList currentLibraryLocations () const { return library_locations; };
- void accept () override;
- void reject () override;
signals:
void libraryLocationsChanged (const QStringList &liblocs);
void installedPackagesChanged ();
protected:
void closeEvent (QCloseEvent *e) override;
protected slots:
- void childDeleted ();
void automatedInstall (const QStringList &packages);
void slotPageChanged ();
private:
+ void queryClose();
void addLibraryLocation (const QString &new_loc);
void tryDestruct ();
void runInstallationCommand (const QString& command, bool as_root, const QString& message, const QString& title);
- KPageWidgetItem* addChild (QWidget *child_page, const QString &caption);
+ KPageWidgetItem* addChild(RKLoadLibsDialogPage *child_page, const QString &caption);
friend class LoadUnloadWidget;
friend class InstallPackagesWidget;
RCommandChain *chain;
@@ -82,8 +96,7 @@ friend class InstallPackagesWidget;
QStringList library_locations;
- int num_child_widgets;
- bool was_accepted;
+ QList<RKLoadLibsDialogPage*> pages;
QProcess* installation_process;
};
@@ -94,23 +107,21 @@ To be used in RKLoadLibsDialog
@author Thomas Friedrichsmeier
*/
-class LoadUnloadWidget : public QWidget {
+class LoadUnloadWidget : public RKLoadLibsDialogPage {
Q_OBJECT
public:
explicit LoadUnloadWidget (RKLoadLibsDialog *dialog);
~LoadUnloadWidget ();
+ void apply() override;
+ void activated() override;
signals:
void loadUnloadDone ();
public slots:
void loadButtonClicked ();
void detachButtonClicked ();
- void ok ();
- void apply ();
- void cancel ();
void updateInstalledPackages ();
void updateButtons ();
- void activated ();
private:
void updateCurrentList ();
void doLoadUnload ();
@@ -180,6 +191,8 @@ public:
/** reset all installation states to NoAction */
void clearStatus ();
bool initialized () const { return _initialized; };
+signals:
+ void changed();
private slots:
void statusCommandFinished (RCommand *command);
private:
@@ -215,7 +228,7 @@ To be used in RKLoadLibsDialog.
@author Thomas Friedrichsmeier
*/
-class InstallPackagesWidget : public QWidget {
+class InstallPackagesWidget : public RKLoadLibsDialogPage {
Q_OBJECT
public:
explicit InstallPackagesWidget (RKLoadLibsDialog *dialog);
@@ -223,12 +236,10 @@ public:
~InstallPackagesWidget ();
void trySelectPackages (const QStringList &package_names);
void initialize ();
+ void apply() override;
+ void activated() override;
public slots:
- void ok ();
- void apply ();
- void cancel ();
void filterChanged ();
- void activated ();
void markAllUpdates ();
void configureRepositories ();
void rowClicked (const QModelIndex& row);
@@ -248,21 +259,16 @@ private:
#include "../settings/rksettingsmoduleplugins.h"
-class RKPluginMapSelectionWidget : public QWidget {
+class RKPluginMapSelectionWidget : public RKLoadLibsDialogPage {
Q_OBJECT
public:
explicit RKPluginMapSelectionWidget (RKLoadLibsDialog *dialog);
virtual ~RKPluginMapSelectionWidget ();
-public slots:
- void ok ();
- void apply ();
- void cancel ();
- void activated ();
- void changed () { changes_pending = true; };
+ void apply() override;
+ void activated() override;
private:
RKMultiStringSelectorV2* selector;
RKSettingsModulePluginsModel* model;
- bool changes_pending;
};
#endif
diff --git a/rkward/misc/rkstandardicons.h b/rkward/misc/rkstandardicons.h
index ef1d310a..610476c8 100644
--- a/rkward/misc/rkstandardicons.h
+++ b/rkward/misc/rkstandardicons.h
@@ -73,7 +73,6 @@ public:
ActionUnlock,
ActionShowMenu,
- ActionConfigure,
ObjectList,
ObjectFunction,
More information about the rkward-tracker
mailing list