Commit c01860e5 authored by Harald Sitter's avatar Harald Sitter 🌈
Browse files

fix multiple opening of properties

in the qml port I incorrectly registered the cpp types as singletons,
but from a registration POV (which is application-wide) they aren't
singleton. they are bound to the context of the plugin instance that
created them. alas, this is a bit awkward to present because none of the
classes can deal with post-ctor initialization (that is: qml would ctor
the objects and then configure them through the properties) so we can't
use them through QML directly. we could construct them per-engine with
the qmlRegisterSingletonType callback system, but that's a bit tricky
because Model, UserManager and ShareContext are tightly related, so we'd
have to do extra fiddeling to get them to link up.

instead simply expose these three as properties on the plugin and set
the plugin as contextproperty. longer-term it may be worthwhile to
change the three classes to support qml-style property construction so
we can construct them from QML as that would then also resolve some
asyncness concerns I had about the fact that the ctors are doing data
processing (even though it is very lightweight data).

this also makes the root widget of the page a unique_ptr. because of the
child ordering here the widget is actually a sibling of the plugin
instance so it can and will get deleted after the plugin which results
in bogus warnings as the context property (the plugin) gets destroyed
and all bindings get reevaluated against the now null property

BUG: 425591
FIXED-IN: 20.12
parent d9692b4c
......@@ -9,7 +9,7 @@ import QtQuick.Layouts 1.14
import org.kde.kirigami 2.4 as Kirigami
import org.kde.filesharing.samba 1.0 as Samba
// NOTE: Samba.ShareContext is a singleton its properties cannot be bound and need manual syncing back.
// NOTE: sambaPlugin.shareContext is a singleton its properties cannot be bound and need manual syncing back.
Item {
// NOTE: we cannot use a Kirigami.Page for this because it adds excessive padding that we can't disable
......@@ -54,10 +54,10 @@ when the Share access rules would allow it.`)
QQC2.CheckBox {
id: shareEnabled
text: i18nc("@option:check", "Share this folder with other computers on the local network")
checked: Samba.ShareContext.enabled
checked: sambaPlugin.shareContext.enabled
onToggled: {
Samba.ShareContext.enabled = checked
Samba.Plugin.dirty = true
sambaPlugin.shareContext.enabled = checked
sambaPlugin.dirty = true
}
}
......@@ -76,23 +76,23 @@ when the Share access rules would allow it.`)
QQC2.TextField {
id: nameField
Layout.fillWidth: true
text: Samba.ShareContext.name
text: sambaPlugin.shareContext.name
onTextEdited: {
if (text.length > Samba.ShareContext.maximumNameLength) {
if (text.length > sambaPlugin.shareContext.maximumNameLength) {
tooLongMessage.visible = true;
// This is a soft limit, do not return.
} else {
tooLongMessage.visible = false
}
if (!Samba.ShareContext.isNameFree(text)) {
if (!sambaPlugin.shareContext.isNameFree(text)) {
alreadyUsedError.visible = true;
return
}
alreadyUsedError.visible = false;
Samba.ShareContext.name = text
Samba.Plugin.dirty = true
sambaPlugin.shareContext.name = text
sambaPlugin.dirty = true
}
}
}
......@@ -117,10 +117,10 @@ when the Share access rules would allow it.`)
QQC2.CheckBox {
text: i18nc("@option:check", "Allow guests")
checked: Samba.ShareContext.guestEnabled
checked: sambaPlugin.shareContext.guestEnabled
onToggled: {
Samba.ShareContext.guestEnabled = checked
Samba.Plugin.dirty = true
sambaPlugin.shareContext.guestEnabled = checked
sambaPlugin.dirty = true
}
}
......@@ -146,7 +146,7 @@ when the Share access rules would allow it.`)
anchors.margins: Kirigami.Units.smallSpacing
clip: true
interactive: false
model: Samba.UserPermissionModel
model: sambaPlugin.userPermissionModel
columnWidthProvider: function (column) {
// Give 2/3 of the width to the access column for better looks.
......@@ -206,7 +206,7 @@ when the Share access rules would allow it.`)
if (currentValue === 'D') {
denialSheet.maybeOpen()
}
Samba.Plugin.dirty = true
sambaPlugin.dirty = true
}
Component.onCompleted: currentIndex = indexOfValue(edit)
}
......@@ -220,7 +220,7 @@ when the Share access rules would allow it.`)
QQC2.Button {
Layout.fillWidth: true
text: i18nc("@button", "Show Samba status monitor")
onClicked: Samba.Plugin.showSambaStatus()
onClicked: sambaPlugin.showSambaStatus()
}
}
}
......@@ -10,12 +10,23 @@ import org.kde.kirigami 2.12 as Kirigami
import org.kde.filesharing.samba 1.0 as Samba
Kirigami.PlaceholderMessage {
Samba.Installer {
id: installer
onInstalledChanged: {
if (!installer.installed) {
return
}
stack.push("RebootPage.qml", { "installer": this })
}
}
text: i18nc("@label", "Samba must be installed before folders can be shared.")
helpfulAction: Kirigami.Action {
iconName: "install"
text: i18nc("@button", "Install Samba")
onTriggered: Samba.Installer.install()
enabled: !Samba.Installer.installing && !Samba.Installer.installed
onTriggered: installer.install()
enabled: !installer.installing && !installer.installed
}
QQC2.Label {
......@@ -23,23 +34,13 @@ Kirigami.PlaceholderMessage {
Layout.fillWidth: true
text: i18nc("@label", "The Samba package failed to install.")
wrapMode: Text.Wrap
visible: Samba.Installer.failed
visible: installer.failed
}
QQC2.ProgressBar {
Layout.alignment: Qt.AlignHCenter
Layout.fillWidth: true
Layout.margins: Kirigami.Units.largeSpacing * 2
indeterminate: true
visible: Samba.Installer.installing
}
Connections {
target: Samba.Installer
onInstalledChanged: {
if (!Samba.Installer.installed) {
return
}
stack.push("RebootPage.qml")
}
visible: installer.installing
}
}
\ No newline at end of file
}
......@@ -10,10 +10,12 @@ import org.kde.kirigami 2.12 as Kirigami
import org.kde.filesharing.samba 1.0 as Samba
Kirigami.PlaceholderMessage {
property Samba.Installer installer
text: i18nc("@label", "Restart the computer to complete the installation.")
helpfulAction: Kirigami.Action {
iconName: "system-restart"
text: i18nc("@button restart the system", "Restart")
onTriggered: Samba.Installer.reboot()
onTriggered: installer.reboot()
}
}
......@@ -38,13 +38,13 @@ Kirigami.ScrollablePage {
onAccepted: {
enabled = false
busy = true
Samba.UserManager.currentUser().addToSamba(password)
sambaPlugin.userManager.currentUser().addToSamba(password)
}
}
Connections {
// ChangePassword being a sheet it's being crap to use and can't even connect to nothing.
target: Samba.UserManager.currentUser()
target: sambaPlugin.userManager.currentUser()
onInSambaChanged: changePassword.userCreated(target.inSamba)
onAddToSambaError: changePassword.errorMessage = error
}
......
......@@ -18,7 +18,7 @@ QQC2.StackView {
property var pendingStack: []
initialItem: QQC2.BusyIndicator {
running: !Samba.Plugin.ready
running: !sambaPlugin.ready
onRunningChanged: {
if (running) {
......@@ -26,11 +26,11 @@ QQC2.StackView {
}
pendingStack.push("ACLPage.qml")
if (!Samba.UserManager.currentUser().inSamba) {
if (!sambaPlugin.userManager.currentUser().inSamba) {
pendingStack.push("UserPage.qml")
}
if (!Samba.Plugin.isSambaInstalled()) {
// NB: Samba.Installer may be not set when built without installer support
if (!sambaPlugin.isSambaInstalled()) {
// NB: the plugin may be built without installer support!
if (Samba.Installer === undefined) {
pendingStack.push("MissingSambaPage.qml")
} else {
......
......@@ -18,6 +18,7 @@
#include <QVBoxLayout>
#include <KLocalizedString>
#include <QTimer>
#include <QQmlContext>
#include <QPushButton>
#include <KMessageBox>
......@@ -162,23 +163,26 @@ SambaUserSharePlugin::SambaUserSharePlugin(QObject *parent, const QList<QVariant
// TODO: this could be made to load delayed via invokemethod. we technically don't need to fully load
// the backing data in the ctor, only the qml view with busyindicator
// TODO: relatedly if we make ShareContext and the Model more async vis a vis construction we can init them from
// QML and stop holding them as members in the plugin
m_context = new ShareContext(properties->item().mostLocalUrl(), this);
// FIXME maybe the manager ought to be owned by the model
qmlRegisterSingletonInstance("org.kde.filesharing.samba", 1, 0, "UserManager", m_userManager);
qmlRegisterUncreatableType<User>("org.kde.filesharing.samba", 1, 0, "User", QStringLiteral("Only created by UserManager"));
qmlRegisterAnonymousType<UserManager>("org.kde.filesharing.samba", 1);
qmlRegisterAnonymousType<User>("org.kde.filesharing.samba", 1);
m_model = new UserPermissionModel(m_context->m_shareData, m_userManager, this);
#ifdef SAMBA_INSTALL
auto installer = new SambaInstaller;
qmlRegisterSingletonInstance("org.kde.filesharing.samba", 1, 0, "Installer", installer);
qmlRegisterType<SambaInstaller>("org.kde.filesharing.samba", 1, 0, "Installer");
#endif
qmlRegisterSingletonInstance("org.kde.filesharing.samba", 1, 0, "UserPermissionModel", m_model);
qmlRegisterSingletonInstance("org.kde.filesharing.samba", 1, 0, "Plugin", this);
qmlRegisterSingletonInstance("org.kde.filesharing.samba", 1, 0, "ShareContext", m_context);
auto page = new QWidget(qobject_cast<KPropertiesDialog *>(parent));
page->setAttribute(Qt::WA_TranslucentBackground);
auto widget = new QQuickWidget(page);
// Need access to the column enum, so register this as uncreatable.
qmlRegisterUncreatableType<UserPermissionModel>("org.kde.filesharing.samba", 1, 0, "UserPermissionModel",
QStringLiteral("Access through sambaPlugin.userPermissionModel"));
qmlRegisterAnonymousType<ShareContext>("org.kde.filesharing.samba", 1);
qmlRegisterAnonymousType<SambaUserSharePlugin>("org.kde.filesharing.samba", 1);
m_page.reset(new QWidget(qobject_cast<KPropertiesDialog *>(parent)));
m_page->setAttribute(Qt::WA_TranslucentBackground);
auto widget = new QQuickWidget(m_page.get());
// Load kdeclarative and set translation domain before setting the source so strings gets translated.
KDeclarative::KDeclarative kdeclarative;
kdeclarative.setDeclarativeEngine(widget->engine());
......@@ -189,13 +193,15 @@ SambaUserSharePlugin::SambaUserSharePlugin(QObject *parent, const QList<QVariant
widget->setFocusPolicy(Qt::StrongFocus);
widget->setAttribute(Qt::WA_AlwaysStackOnTop, true);
widget->quickWindow()->setColor(Qt::transparent);
auto layout = new QVBoxLayout(page);
auto layout = new QVBoxLayout(m_page.get());
layout->addWidget(widget);
widget->rootContext()->setContextProperty(QStringLiteral("sambaPlugin"), this);
const QUrl url(QStringLiteral("qrc:/org.kde.filesharing.samba/qml/main.qml"));
widget->setSource(url);
auto item = properties->addPage(page, i18nc("@title:tab", "Share"));
auto item = properties->addPage(m_page.get(), i18nc("@title:tab", "Share"));
if (qEnvironmentVariableIsSet("TEST_FOCUS_SHARE")) {
QTimer::singleShot(100, [item, this] {
properties->setCurrentPage(item);
......
......@@ -12,6 +12,8 @@
#include <kpropertiesdialog.h>
#include <KSambaShareData>
#include <memory>
class UserPermissionModel;
class ShareContext;
class UserManager;
......@@ -21,6 +23,11 @@ class SambaUserSharePlugin : public KPropertiesDialogPlugin
Q_OBJECT
Q_PROPERTY(bool dirty READ isDirty WRITE setDirty NOTIFY changed) // So qml can mark dirty
Q_PROPERTY(bool ready READ isReady NOTIFY readyChanged) // intentionally not writable from qml
// Expose instance-singleton members so QML may access them.
// They aren't application-wide singletons and also cannot easily be ctor'd from QML.
Q_PROPERTY(UserManager *userManager MEMBER m_userManager CONSTANT)
Q_PROPERTY(UserPermissionModel *userPermissionModel MEMBER m_model CONSTANT)
Q_PROPERTY(ShareContext *shareContext MEMBER m_context CONSTANT)
public:
SambaUserSharePlugin(QObject *parent, const QList<QVariant> &args);
~SambaUserSharePlugin() override = default;
......@@ -44,6 +51,10 @@ private:
UserPermissionModel *m_model = nullptr;
UserManager *m_userManager = nullptr;
bool m_ready = false;
// Hold the qquickwidget so it gets destroyed with us. Otherwise we'd have bogus reference errors
// as the Plugin instance is exposed as contextProperty to qml but the widget is parented to the PropertiesDialog
// (i.e. our parent). So the lifetime of the widget would usually exceed ours.
std::unique_ptr<QWidget> m_page = nullptr;
};
#endif // SAMBAUSERSHAREPLUGIN_H
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment