fix(glossary): handle save errors and improve playlist controls

autotests/glossarystoretest.cpp

@@ -2,6 +2,8 @@
 
 #include "glossarystore.h"
 
+#include <QDir>
+#include <QFile>
 #include <QStandardPaths>
 #include <QTest>
 using namespace Qt::Literals::StringLiterals;
@@ -24,6 +26,36 @@ private Q_SLOTS:
         QCOMPARE(merged.entries.at(0).target, u"Lâm"_s);
         QCOMPARE(merged.entries.at(1).source, u"Sect"_s);
     }
+
+    void saveFailureReturnsError()
+    {
+        QTemporaryDir temp;
+        QVERIFY(temp.isValid());
+        QStandardPaths::setTestModeEnabled(true);
+
+        const QString appData = QStandardPaths::writableLocation(QStandardPaths::AppDataLocation);
+        const QString glossaries = appData + u"/glossaries"_s;
+        QDir(glossaries).removeRecursively();
+        QVERIFY(QDir().mkpath(appData));
+        QFile blocker(glossaries);
+        QVERIFY(blocker.open(QIODevice::WriteOnly));
+        blocker.write("blocked");
+        blocker.close();
+
+        GlossaryStore::Glossary glossary{temp.path(), {{u"Original"_s, u"Translation"_s}}};
+        QString error;
+        QVERIFY(!GlossaryStore::save(glossary, &error));
+        QVERIFY(!error.isEmpty());
+
+        QFile::remove(glossaries);
+    }
+
+    void saveRejectsIncompleteEntry()
+    {
+        QString error;
+        QVERIFY(!GlossaryStore::save({QDir::tempPath(), {{u"Original"_s, {}}}}, &error));
+        QVERIFY(error.contains(u"both"_s));
+    }
 };
 
 QTEST_MAIN(GlossaryStoreTest)

src/qml/GlossaryDialog.qml

@@ -14,10 +14,20 @@ Kirigami.Dialog {
     required property var translator
 
     readonly property real removeButtonWidth: Kirigami.Units.iconSizes.smallMedium + Kirigami.Units.smallSpacing * 2
+    readonly property bool hasIncompleteTerms: {
+        for (let i = 0; i < glossaryModel.count; ++i) {
+            const entry = glossaryModel.get(i)
+            if (!entry.source.trim() || !entry.target.trim()) {
+                return true
+            }
+        }
+        return false
+    }
+    property string saveError: ""
     // One inset for the header, content and footer so every edge lines up.
     readonly property real inset: Kirigami.Units.largeSpacing
 
-    title: KI18n.i18nc("@title:window", "Translation Glossary")
+    title: KI18n.i18nc("@title:window", "Edit Translation Glossary")
     standardButtons: Kirigami.Dialog.NoButton
     width: Math.min(parent.width * 0.9, Kirigami.Units.gridUnit * 38)
     height: Math.min(parent.height - Kirigami.Units.largeSpacing * 2, Kirigami.Units.gridUnit * 30)
@@ -53,25 +63,28 @@ Kirigami.Dialog {
         })
     }
 
+    function saveGlossary() {
+        const entries = []
+        for (let i = 0; i < glossaryModel.count; ++i) {
+            const entry = glossaryModel.get(i)
+            entries.push({ source: entry.source, target: entry.target })
+        }
+        if (!translator.saveGlossary(entries)) {
+            saveError = translator.glossaryError || KI18n.i18nc("@info", "Could not save the translation glossary. Check that the storage location is writable.")
+            return
+        }
+        saveError = ""
+        root.accept()
+    }
+
     onOpened: {
+        saveError = ""
         glossaryModel.clear()
         for (const entry of translator.glossaryEntries) {
             glossaryModel.append(entry)
         }
         glossaryList.positionViewAtBeginning()
     }
-    onAccepted: {
-        const entries = []
-        for (let i = 0; i < glossaryModel.count; ++i) {
-            const entry = glossaryModel.get(i)
-            entries.push({
-                source: entry.source,
-                target: entry.target
-            })
-        }
-        translator.saveGlossary(entries)
-    }
-
     ListModel {
         id: glossaryModel
     }
@@ -94,7 +107,8 @@ Kirigami.Dialog {
             Button {
                 text: KI18n.i18nc("@action:button", "Save")
                 icon.name: "document-save"
-                onClicked: root.accept()
+                enabled: !root.hasIncompleteTerms
+                onClicked: root.saveGlossary()
             }
 
             Button {
@@ -112,9 +126,42 @@ Kirigami.Dialog {
         onTriggered: root.addTerm()
     }
 
-    contentItem: ColumnLayout {
+    contentItem: ScrollView {
+        id: contentScroll
+        clip: true
+        contentWidth: availableWidth
+        contentHeight: contentColumn.height
+        ScrollBar.horizontal.policy: ScrollBar.AlwaysOff
+
+        // Fill the dialog normally; only scroll as a whole when the window is too
+        // short for the list's minimum height.
+        ColumnLayout {
+            id: contentColumn
+            width: contentScroll.availableWidth
+            height: Math.max(contentScroll.availableHeight, implicitHeight)
             spacing: Kirigami.Units.largeSpacing
 
+        Kirigami.InlineMessage {
+            Layout.fillWidth: true
+            visible: root.saveError.length > 0
+            type: Kirigami.MessageType.Error
+            text: root.saveError
+            showCloseButton: true
+            actions: Kirigami.Action {
+                text: KI18n.i18nc("@action:button", "Retry")
+                icon.name: "view-refresh"
+                onTriggered: root.saveGlossary()
+            }
+        }
+
+        Kirigami.InlineMessage {
+            Layout.fillWidth: true
+            visible: root.hasIncompleteTerms
+            type: Kirigami.MessageType.Warning
+            text: KI18n.i18nc("@info", "Complete both fields for each term, or remove incomplete rows, before saving.")
+            showCloseButton: false
+        }
+
         RowLayout {
             Layout.fillWidth: true
             spacing: Kirigami.Units.smallSpacing
@@ -169,6 +216,8 @@ Kirigami.Dialog {
         Item {
             Layout.fillWidth: true
             Layout.fillHeight: true
+            Layout.minimumHeight: Kirigami.Units.gridUnit * 6
+            Layout.preferredHeight: Kirigami.Units.gridUnit * 6
 
             ScrollView {
                 id: listScroll
@@ -240,6 +289,8 @@ Kirigami.Dialog {
                             Layout.preferredWidth: 1
                             Layout.minimumWidth: 0
                             text: termRow.source
+                            // Show the start of long terms, not the end.
+                            Component.onCompleted: cursorPosition = 0
                             placeholderText: KI18n.i18nc("@info:placeholder glossary source term", "Original term")
                             Accessible.name: KI18n.i18nc("@label:textbox glossary source term", "Original term")
                             onTextEdited: glossaryModel.setProperty(termRow.index, "source", text)
@@ -250,12 +301,14 @@ Kirigami.Dialog {
                             Layout.preferredWidth: 1
                             Layout.minimumWidth: 0
                             text: termRow.target
+                            // Show the start of long terms, not the end.
+                            Component.onCompleted: cursorPosition = 0
                             placeholderText: KI18n.i18nc("@info:placeholder glossary translated term", "Translation")
                             Accessible.name: KI18n.i18nc("@label:textbox glossary translated term", "Translation")
                             onTextEdited: glossaryModel.setProperty(termRow.index, "target", text)
                         }
 
-                        ToolButton {
+                        Button {
                             Layout.preferredWidth: root.removeButtonWidth
                             Layout.minimumWidth: root.removeButtonWidth
                             Layout.maximumWidth: root.removeButtonWidth
@@ -264,6 +317,7 @@ Kirigami.Dialog {
                             icon.name: "edit-delete"
                             icon.color: Kirigami.Theme.textColor
                             display: AbstractButton.IconOnly
+                            Accessible.name: text
                             text: KI18n.i18nc("@action:button remove glossary term", "Remove")
                             ToolTip.text: text
                             ToolTip.visible: hovered
@@ -284,4 +338,5 @@ Kirigami.Dialog {
             }
         }
     }
+    }
 }

src/qml/Playlist.qml

@@ -20,13 +20,13 @@ Page {
     property bool fullscreen: false
 
     readonly property int edge: QtPotatoSettings.playlistPosition === "left" ? Qt.LeftEdge : Qt.RightEdge
-    readonly property int buttonSize: Kirigami.Units.iconSizes.small
+    readonly property int buttonSize: Kirigami.Units.iconSizes.smallMedium
     property real customWidth: QtPotatoSettings.playlistWidth
 
     signal addFilesRequested()
 
     function limitWidth(width: real): real {
-        return Math.min(Math.max(width, 260), mainWindowWidth - 50)
+        return Math.min(Math.max(width, Kirigami.Units.gridUnit * 13), mainWindowWidth - Kirigami.Units.gridUnit * 2.5)
     }
 
     function toggle() {
@@ -63,7 +63,7 @@ Page {
                 icon.name: "list-add"
                 icon.width: root.buttonSize
                 icon.height: root.buttonSize
-                focusPolicy: Qt.NoFocus
+                focusPolicy: Qt.StrongFocus
                 onClicked: root.addFilesRequested()
             }
 
@@ -73,7 +73,7 @@ Page {
                 icon.width: root.buttonSize
                 icon.height: root.buttonSize
                 display: AbstractButton.IconOnly
-                focusPolicy: Qt.NoFocus
+                focusPolicy: Qt.StrongFocus
                 enabled: root.playlist.playingIndex >= 0
                 onClicked: playlistView.positionViewAtIndex(root.playlist.playingIndex, ListView.Center)
 
@@ -88,7 +88,7 @@ Page {
                 icon.width: root.buttonSize
                 icon.height: root.buttonSize
                 display: AbstractButton.IconOnly
-                focusPolicy: Qt.NoFocus
+                focusPolicy: Qt.StrongFocus
                 enabled: root.playlist.count > 0
                 onClicked: root.playlist.clear()
 
@@ -141,7 +141,7 @@ Page {
         ResizeHandler {
             id: resizeHandlerTall
             anchors.horizontalCenter: parent.horizontalCenter
-            width: 8
+            width: Kirigami.Units.smallSpacing * 2
             height: parent.height
             onMoved: delta => root.customWidth = root.limitWidth(root.customWidth + (root.edge === Qt.RightEdge ? -delta : delta))
             onReleased: {
@@ -152,8 +152,8 @@ Page {
 
         Rectangle {
             anchors.centerIn: parent
-            width: 5
+            width: Kirigami.Units.smallSpacing
-            height: 50
+            height: Kirigami.Units.gridUnit * 2.5
             color: resizeHandlerTall.hovered ? Kirigami.Theme.highlightColor : Kirigami.Theme.alternateBackgroundColor
             radius: Kirigami.Units.cornerRadius
             border.width: 1

src/qml/PlaylistItem.qml

@@ -22,6 +22,25 @@ Item {
 
     readonly property bool hovered: hoverHandler.hovered
     readonly property real fontSize: Kirigami.Theme.defaultFont.pointSize
+    activeFocusOnTab: true
+
+    function activate() {
+        root.playlist.play(root.index)
+    }
+
+    function showContextMenu() {
+        contextMenu.popup()
+    }
+
+    Keys.onReturnPressed: root.activate()
+    Keys.onEnterPressed: root.activate()
+    Keys.onMenuPressed: root.showContextMenu()
+    Keys.onPressed: event => {
+        if (event.key === Qt.Key_F10 && (event.modifiers & Qt.ShiftModifier)) {
+            root.showContextMenu()
+            event.accepted = true
+        }
+    }
 
     function labelColor(): color {
         if (root.isPlaying && !root.hovered) {
@@ -39,6 +58,7 @@ Item {
 
     TapHandler {
         acceptedButtons: Qt.LeftButton
+        onTapped: root.forceActiveFocus(Qt.MouseFocusReason)
         onDoubleTapped: root.playlist.play(root.index)
     }
 
@@ -60,6 +80,9 @@ Item {
             return Qt.alpha(Kirigami.Theme.backgroundColor, root.alpha)
         }
 
+        border.width: root.activeFocus ? 2 : 0
+        border.color: Kirigami.Theme.focusColor
+
         RowLayout {
             anchors.fill: parent
             spacing: Kirigami.Units.largeSpacing
@@ -95,8 +118,8 @@ Item {
                 color: root.labelColor()
                 visible: root.isPlaying
                 Layout.leftMargin: QtPotatoSettings.playlistShowRowNumber ? 0 : Kirigami.Units.largeSpacing
-                Layout.preferredWidth: Kirigami.Units.iconSizes.small
+                Layout.preferredHeight: Kirigami.Units.iconSizes.smallMedium
-                Layout.preferredHeight: Kirigami.Units.iconSizes.small
+                Layout.preferredWidth: Kirigami.Units.iconSizes.smallMedium
             }
 
             LabelWithTooltip {
@@ -105,11 +128,26 @@ Item {
                 color: root.labelColor()
                 elide: Text.ElideRight
                 font.pointSize: root.fontSize
+                font.bold: root.isPlaying
                 verticalAlignment: Qt.AlignVCenter
                 Layout.fillWidth: true
                 Layout.leftMargin: !QtPotatoSettings.playlistShowRowNumber && !root.isPlaying ? Kirigami.Units.largeSpacing : 0
                 Layout.rightMargin: Kirigami.Units.largeSpacing
             }
+
+            Button {
+                text: KI18n.i18nc("@action:button remove playlist item", "Remove")
+                icon.name: "list-remove"
+                display: AbstractButton.IconOnly
+                flat: true
+                Accessible.name: text
+                ToolTip.text: text
+                ToolTip.visible: hovered
+                ToolTip.delay: Kirigami.Units.toolTipDelay
+                Layout.preferredWidth: Kirigami.Units.iconSizes.smallMedium + Kirigami.Units.smallSpacing * 2
+                Layout.rightMargin: Kirigami.Units.smallSpacing
+                onClicked: root.playlist.remove(root.index)
+            }
         }
     }
 

src/translate/glossarystore.cpp

@@ -56,12 +56,28 @@ GlossaryStore::Glossary GlossaryStore::load(const QString &directory)
 bool GlossaryStore::save(const Glossary &glossary, QString *error)
 {
     const QString directory = QDir::cleanPath(glossary.directory);
+    for (const Entry &entry : glossary.entries) {
+        if (entry.source.trimmed().isEmpty() || entry.target.trimmed().isEmpty()) {
+            if (error) {
+                *error = QStringLiteral("Each glossary term must have both an original and a translation.");
+            }
+            return false;
+        }
+    }
     const QString path = pathForDirectory(directory);
-    QDir().mkpath(QFileInfo(path).absolutePath());
+    if (!QDir().mkpath(QFileInfo(path).absolutePath()) && !QFileInfo::exists(QFileInfo(path).absolutePath())) {
+        if (error) {
+            *error = QStringLiteral("Could not create the glossary storage directory.");
+        }
+        return false;
+    }
     QSaveFile file(path);
     if (!file.open(QIODevice::WriteOnly) || file.write(QJsonDocument(glossaryJson({directory, glossary.entries})).toJson(QJsonDocument::Indented)) < 0 || !file.commit()) {
         if (error) {
             *error = file.errorString();
+            if (error->isEmpty()) {
+                *error = QStringLiteral("Could not write the glossary file.");
+            }
         }
         return false;
     }

src/translate/translationservice.cpp

@@ -139,25 +139,51 @@ QString TranslationService::glossaryDirectory() const
     return m_glossary.directory;
 }
 
+QString TranslationService::glossaryError() const
+{
+    return m_glossaryError;
+}
+
 void TranslationService::loadGlossary(const QUrl &videoUrl)
 {
     const QString directory = videoUrl.isLocalFile() ? QFileInfo(videoUrl.toLocalFile()).absolutePath() : QString();
     m_glossary = directory.isEmpty() ? GlossaryStore::Glossary{} : GlossaryStore::load(directory);
+    if (!m_glossaryError.isEmpty()) {
+        m_glossaryError.clear();
+        Q_EMIT glossaryErrorChanged();
+    }
     Q_EMIT glossaryChanged();
 }
 
-void TranslationService::saveGlossary(const QVariantList &entries)
+bool TranslationService::saveGlossary(const QVariantList &entries)
 {
     QList<GlossaryStore::Entry> result;
     for (const QVariant &value : entries) {
         const QVariantMap map = value.toMap();
         result.append({map.value(u"source"_s).toString().trimmed(), map.value(u"target"_s).toString().trimmed()});
     }
-    m_glossary.entries = result;
+
-    if (!m_glossary.directory.isEmpty()) {
+    GlossaryStore::Glossary candidate = m_glossary;
-        GlossaryStore::save(m_glossary);
+    candidate.entries = result;
+    if (!candidate.directory.isEmpty()) {
+        QString error;
+        if (!GlossaryStore::save(candidate, &error)) {
+            const QString message = error.isEmpty() ? i18n("Could not save the translation glossary.")
+                                                     : i18n("Could not save the translation glossary: %1", error);
+            if (m_glossaryError != message) {
+                m_glossaryError = message;
+                Q_EMIT glossaryErrorChanged();
+            }
+            return false;
+        }
+    }
+    m_glossary = candidate;
+    if (!m_glossaryError.isEmpty()) {
+        m_glossaryError.clear();
+        Q_EMIT glossaryErrorChanged();
     }
     Q_EMIT glossaryChanged();
+    return true;
 }
 
 void TranslationService::clearGlossary()

src/translate/translationservice.h

@@ -34,6 +34,7 @@ class TranslationService : public QObject
     Q_PROPERTY(bool fetchingModels READ isFetchingModels NOTIFY fetchingModelsChanged)
     Q_PROPERTY(QVariantList glossaryEntries READ glossaryEntries NOTIFY glossaryChanged)
     Q_PROPERTY(QString glossaryDirectory READ glossaryDirectory NOTIFY glossaryChanged)
+    Q_PROPERTY(QString glossaryError READ glossaryError NOTIFY glossaryErrorChanged)
 
 public:
     explicit TranslationService(QObject *parent = nullptr);
@@ -48,6 +49,7 @@ public:
     bool isFetchingModels() const;
     QVariantList glossaryEntries() const;
     QString glossaryDirectory() const;
+    QString glossaryError() const;
 
     // \a track is an entry of MpvItem::subtitleTracks.
     Q_INVOKABLE void translate(const QUrl &videoUrl, const QVariantMap &track, const QString &mediaTitle);
@@ -56,7 +58,7 @@ public:
     Q_INVOKABLE void fetchModels();
     Q_INVOKABLE void clearError();
     Q_INVOKABLE void loadGlossary(const QUrl &videoUrl);
-    Q_INVOKABLE void saveGlossary(const QVariantList &entries);
+    Q_INVOKABLE bool saveGlossary(const QVariantList &entries);
     Q_INVOKABLE void clearGlossary();
 
 Q_SIGNALS:
@@ -67,6 +69,7 @@ Q_SIGNALS:
     void availableModelsChanged();
     void fetchingModelsChanged();
     void glossaryChanged();
+    void glossaryErrorChanged();
     void translated(const QUrl &subtitleUrl, bool fromCache);
 
 private:
@@ -98,6 +101,7 @@ private:
     double m_progress = 0.0;
     QString m_statusText;
     QString m_errorText;
+    QString m_glossaryError;
     QStringList m_availableModels;
     bool m_fetchingModels = false;
     GlossaryStore::Glossary m_glossary;