From e7760e22e039a144545b64c3801d13fcb8f84c41 Mon Sep 17 00:00:00 2001 From: Tri Vo Khac Date: Sat, 7 Nov 2026 18:48:04 +0700 Subject: [PATCH] fix(glossary): handle save errors and improve playlist controls --- autotests/glossarystoretest.cpp | 32 ++++++++++ src/qml/GlossaryDialog.qml | 89 ++++++++++++++++++++++------ src/qml/Playlist.qml | 16 ++--- src/qml/PlaylistItem.qml | 42 ++++++++++++- src/translate/glossarystore.cpp | 18 +++++- src/translate/translationservice.cpp | 34 +++++++++-- src/translate/translationservice.h | 6 +- 7 files changed, 204 insertions(+), 33 deletions(-) diff --git a/autotests/glossarystoretest.cpp b/autotests/glossarystoretest.cpp index 6c7f9a1..fd00096 100644 --- a/autotests/glossarystoretest.cpp +++ b/autotests/glossarystoretest.cpp @@ -2,6 +2,8 @@ #include "glossarystore.h" +#include +#include #include #include 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) diff --git a/src/qml/GlossaryDialog.qml b/src/qml/GlossaryDialog.qml index c2b5970..b940d54 100644 --- a/src/qml/GlossaryDialog.qml +++ b/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,8 +126,41 @@ Kirigami.Dialog { onTriggered: root.addTerm() } - contentItem: ColumnLayout { - spacing: Kirigami.Units.largeSpacing + 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 @@ -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 { } } } + } } diff --git a/src/qml/Playlist.qml b/src/qml/Playlist.qml index d4a9227..6dc24a6 100644 --- a/src/qml/Playlist.qml +++ b/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 - height: 50 + width: Kirigami.Units.smallSpacing + height: Kirigami.Units.gridUnit * 2.5 color: resizeHandlerTall.hovered ? Kirigami.Theme.highlightColor : Kirigami.Theme.alternateBackgroundColor radius: Kirigami.Units.cornerRadius border.width: 1 diff --git a/src/qml/PlaylistItem.qml b/src/qml/PlaylistItem.qml index 9ce0577..0c545e3 100644 --- a/src/qml/PlaylistItem.qml +++ b/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.small + Layout.preferredHeight: Kirigami.Units.iconSizes.smallMedium + 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) + } } } diff --git a/src/translate/glossarystore.cpp b/src/translate/glossarystore.cpp index 2cdcd45..ad156f1 100644 --- a/src/translate/glossarystore.cpp +++ b/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; } diff --git a/src/translate/translationservice.cpp b/src/translate/translationservice.cpp index 4e20ab7..a13a496 100644 --- a/src/translate/translationservice.cpp +++ b/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 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::save(m_glossary); + + GlossaryStore::Glossary candidate = 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() diff --git a/src/translate/translationservice.h b/src/translate/translationservice.h index 7eada2a..db47bd3 100644 --- a/src/translate/translationservice.h +++ b/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;