Skip to content

Fix pre-release issues with attachment viewer #12244

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions share/translations/keepassxc_en.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4088,6 +4088,10 @@ Would you like to overwrite the existing attachment?</source>
<source>Save…</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>New Attachment</source>
<translation type="unfinished"></translation>
</message>
</context>
<context>
<name>EntryAttributesModel</name>
Expand Down Expand Up @@ -4657,6 +4661,10 @@ You can enable the DuckDuckGo website icon service in the security section of th
<source>Zoom:</source>
<translation type="unfinished"></translation>
</message>
<message>
<source>Fit</source>
<translation type="unfinished"></translation>
</message>
</context>
<context>
<name>ImportWizard</name>
Expand Down Expand Up @@ -9261,10 +9269,6 @@ This option is deprecated, use --set-key-file instead.</source>
<comment>TOTP</comment>
<translation type="unfinished"></translation>
</message>
<message>
<source>Fit</source>
<translation type="unfinished"></translation>
</message>
</context>
<context>
<name>QtIOCompressor</name>
Expand Down
3 changes: 1 addition & 2 deletions src/gui/entry/EntryAttachmentsWidget.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@

namespace
{
constexpr const char* DefaultName = "New Attachment";
constexpr const char* Suffix = ".txt";

QString generateUniqueName(const QString& name, const QStringList& existingNames)
Expand Down Expand Up @@ -210,7 +209,7 @@ void EntryAttachmentsWidget::newAttachments()
}

// Create a temporary file to allow the user to edit the attachment
auto newFileName = generateUniqueName(DefaultName, m_entryAttachments->keys());
auto newFileName = generateUniqueName(tr("New Attachment"), m_entryAttachments->keys());
m_entryAttachments->set(newFileName, QByteArray());

auto currentIndex = m_attachmentsModel->index(m_attachmentsModel->rowByKey(newFileName), 0);
Expand Down
9 changes: 4 additions & 5 deletions src/gui/entry/attachments/ImageAttachmentsWidget.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,6 @@ namespace
constexpr std::array ZoomList = {0.25, 0.5, 0.75, 1.0, 2.0};
constexpr double WheelZoomStep = 1.1;

const QString FitText = QObject::tr("Fit");

QString formatZoomText(double zoomFactor)
{
return QString("%1%").arg(QString::number(zoomFactor * 100, 'f', 0));
Expand Down Expand Up @@ -92,9 +90,10 @@ void ImageAttachmentsWidget::initZoomComboBox()
{
m_ui->zoomComboBox->clear();

auto textWidth = m_ui->zoomComboBox->fontMetrics().horizontalAdvance(FitText);
auto fitText = tr("Fit");
auto textWidth = m_ui->zoomComboBox->fontMetrics().horizontalAdvance(fitText);

m_ui->zoomComboBox->addItem(FitText, 0.0);
m_ui->zoomComboBox->addItem(fitText, 0.0);

for (const auto& zoom : ZoomList) {
auto zoomText = formatZoomText(zoom);
Expand Down Expand Up @@ -146,7 +145,7 @@ void ImageAttachmentsWidget::onZoomChanged(const QString& zoomText)
{
auto zoomFactor = 1.0;

if (zoomText == FitText) {
if (zoomText == tr("Fit")) {
m_ui->imagesView->enableAutoFitInView();

zoomFactor = std::min(m_ui->imagesView->calculateFitInViewFactor(), zoomFactor);
Expand Down
7 changes: 6 additions & 1 deletion src/gui/entry/attachments/TextAttachmentsEditWidget.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@
#include "ui_TextAttachmentsEditWidget.h"

#include <QPushButton>
#include <QScrollBar>
#include <QTextEdit>
#include <qwidget.h>

TextAttachmentsEditWidget::TextAttachmentsEditWidget(QWidget* parent)
: QWidget(parent)
Expand All @@ -30,6 +30,11 @@ TextAttachmentsEditWidget::TextAttachmentsEditWidget(QWidget* parent)

connect(m_ui->attachmentsTextEdit, &QTextEdit::textChanged, this, &TextAttachmentsEditWidget::textChanged);
connect(m_ui->previewPushButton, &QPushButton::clicked, this, &TextAttachmentsEditWidget::previewButtonClicked);
connect(m_ui->attachmentsTextEdit->verticalScrollBar(), &QScrollBar::valueChanged, this, [this](int value) {
// Return a percentage of document scroll
auto percent = static_cast<double>(value) / m_ui->attachmentsTextEdit->verticalScrollBar()->maximum();
emit scrollChanged(percent);
});
}

TextAttachmentsEditWidget::~TextAttachmentsEditWidget() = default;
Expand Down
1 change: 1 addition & 0 deletions src/gui/entry/attachments/TextAttachmentsEditWidget.h
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ class TextAttachmentsEditWidget : public QWidget

signals:
void textChanged();
void scrollChanged(double percent);
void previewButtonClicked(bool isChecked);

private:
Expand Down
6 changes: 5 additions & 1 deletion src/gui/entry/attachments/TextAttachmentsEditWidget.ui
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,11 @@
</layout>
</item>
<item>
<widget class="QTextEdit" name="attachmentsTextEdit"/>
<widget class="QTextEdit" name="attachmentsTextEdit">
<property name="tabStopDistance">
<double>10.000000000000000</double>
</property>
</widget>
</item>
</layout>
</widget>
Expand Down
37 changes: 31 additions & 6 deletions src/gui/entry/attachments/TextAttachmentsPreviewWidget.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,10 @@
#include <QComboBox>
#include <QDebug>
#include <QMetaEnum>
#include <QScrollBar>
#include <QSortFilterProxyModel>
#include <QStandardItemModel>
#include <QTimer>

namespace
{
Expand All @@ -48,6 +50,7 @@ namespace
TextAttachmentsPreviewWidget::TextAttachmentsPreviewWidget(QWidget* parent)
: QWidget(parent)
, m_ui(new Ui::TextAttachmentsPreviewWidget())
, m_userManuallySelectedType(false)
{
m_ui->setupUi(this);

Expand Down Expand Up @@ -88,10 +91,13 @@ void TextAttachmentsPreviewWidget::initTypeCombobox()
filterProxyMode->sort(0, Qt::SortOrder::DescendingOrder);
m_ui->typeComboBox->setModel(filterProxyMode);

connect(m_ui->typeComboBox,
QOverload<int>::of(&QComboBox::currentIndexChanged),
this,
&TextAttachmentsPreviewWidget::onTypeChanged);
connect(m_ui->typeComboBox, QOverload<int>::of(&QComboBox::currentIndexChanged), this, [this](int index) {
m_userManuallySelectedType = true;
onTypeChanged(index);
});

// Configure text browser to open external links
m_ui->previewTextBrowser->setOpenExternalLinks(true);

m_ui->typeComboBox->setCurrentIndex(m_ui->typeComboBox->findData(PlainText));

Expand All @@ -100,11 +106,17 @@ void TextAttachmentsPreviewWidget::initTypeCombobox()

void TextAttachmentsPreviewWidget::updateUi()
{
if (!m_attachment.name.isEmpty()) {
// Only auto-select format based on file extension if user hasn't manually chosen one
if (!m_userManuallySelectedType && !m_attachment.name.isEmpty()) {
const auto mimeType = Tools::getMimeType(QFileInfo(m_attachment.name));

auto index = m_ui->typeComboBox->findData(ConvertToPreviewTextType(mimeType));
m_ui->typeComboBox->setCurrentIndex(index);
if (index >= 0) {
// Temporarily block signals to avoid triggering manual selection flag
m_ui->typeComboBox->blockSignals(true);
m_ui->typeComboBox->setCurrentIndex(index);
m_ui->typeComboBox->blockSignals(false);
}
}

onTypeChanged(m_ui->typeComboBox->currentIndex());
Expand All @@ -116,6 +128,7 @@ void TextAttachmentsPreviewWidget::onTypeChanged(int index)
qWarning() << "TextAttachmentsPreviewWidget: Unknown text format";
}

const auto scrollPos = m_ui->previewTextBrowser->verticalScrollBar()->value();
const auto fileType = m_ui->typeComboBox->itemData(index).toInt();
if (fileType == TextAttachmentsPreviewWidget::PreviewTextType::PlainText) {
m_ui->previewTextBrowser->setPlainText(m_attachment.data);
Expand All @@ -130,4 +143,16 @@ void TextAttachmentsPreviewWidget::onTypeChanged(int index)
m_ui->previewTextBrowser->setMarkdown(m_attachment.data);
}
#endif

// Delay setting the scrollbar position to ensure the text is rendered first
QTimer::singleShot(
100, this, [this, scrollPos] { m_ui->previewTextBrowser->verticalScrollBar()->setValue(scrollPos); });
}

void TextAttachmentsPreviewWidget::matchScroll(double percent)
{
// Match the scroll position of the text browser to the given percentage
int maxScroll = m_ui->previewTextBrowser->verticalScrollBar()->maximum();
int newScrollPos = static_cast<int>(maxScroll * percent);
m_ui->previewTextBrowser->verticalScrollBar()->setValue(newScrollPos);
}
4 changes: 4 additions & 0 deletions src/gui/entry/attachments/TextAttachmentsPreviewWidget.h
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,9 @@ class TextAttachmentsPreviewWidget : public QWidget

Q_ENUM(PreviewTextType)

public slots:
void matchScroll(double percent);

private slots:
void onTypeChanged(int index);

Expand All @@ -58,4 +61,5 @@ private slots:
QScopedPointer<Ui::TextAttachmentsPreviewWidget> m_ui;

attachments::Attachment m_attachment;
bool m_userManuallySelectedType;
};
6 changes: 5 additions & 1 deletion src/gui/entry/attachments/TextAttachmentsPreviewWidget.ui
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,11 @@
</layout>
</item>
<item>
<widget class="QTextBrowser" name="previewTextBrowser"/>
<widget class="QTextBrowser" name="previewTextBrowser">
<property name="tabStopDistance">
<double>10.000000000000000</double>
</property>
</widget>
</item>
</layout>
</widget>
Expand Down
51 changes: 36 additions & 15 deletions src/gui/entry/attachments/TextAttachmentsWidget.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -19,19 +19,16 @@
#include "TextAttachmentsEditWidget.h"
#include "TextAttachmentsPreviewWidget.h"

#include "ui_TextAttachmentsWidget.h"

#include <QSplitter>
#include <QTextEdit>
#include <QTimer>
#include <QVBoxLayout>

TextAttachmentsWidget::TextAttachmentsWidget(QWidget* parent)
: QWidget(parent)
, m_ui(new Ui::TextAttachmentsWidget())
, m_previewUpdateTimer(new QTimer(this))
, m_mode(attachments::OpenMode::ReadOnly)
{
m_ui->setupUi(this);
initWidget();
}

Expand Down Expand Up @@ -65,7 +62,15 @@ void TextAttachmentsWidget::updateWidget()
}

m_editWidget->openAttachment(m_attachment, m_mode);
m_previewWidget->openAttachment(m_attachment, attachments::OpenMode::ReadOnly);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing this line causes a delay (almost 1 second) when a preview is rendered, even with really small plain text files. Is this necessary? Could we use something like this here, or does it break the scrollbars you mentioned in the issue thread?

if (m_previewWidget->width() > 0) {
    m_previewWidget->openAttachment(m_attachment, attachments::OpenMode::ReadOnly);
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But just implementing the above will do the openAttachment() again when the timer hits.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line was removed because it caused the preview widget to be loaded 3 times when the edit window is first shown and the preview isn't even visible. How can removing it cause more delay? Not sure on your report here.

Copy link
Member

@varjolintu varjolintu Jul 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe the videos will help. develop branch:
https://github.com/user-attachments/assets/24993d75-2fc2-4827-b53e-d4d53d85ee99

This PR:
https://github.com/user-attachments/assets/272688bf-12f9-4028-9957-728859d85932

As you can see, this PR has a delay every time Preview dialog is opened.

Copy link
Member

@droidmonkey droidmonkey Jul 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahhhhhh, just preview, not edit. The delay is from the 500ms timer for when the attachment text is changed.

}

void TextAttachmentsWidget::updatePreviewWidget()
{
m_previewVisible = m_previewWidget->width() > 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This m_previewWidget->width() > 0 is repeated many times in the file. Maybe create a function for it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's kind of crazy qt doesn't offer an isCollapsed function on the QSplitter. isVisible always returns true as well. Good idea on a defined function.

if (m_previewVisible) {
m_attachment = m_editWidget->getAttachment();
m_previewWidget->openAttachment(m_attachment, attachments::OpenMode::ReadOnly);
}
}

void TextAttachmentsWidget::initWidget()
Expand All @@ -78,28 +83,44 @@ void TextAttachmentsWidget::initWidget()
m_previewUpdateTimer->setInterval(500);

// Only update the preview after a set timeout and if it is visible
connect(m_previewUpdateTimer, &QTimer::timeout, this, [this] {
if (m_previewWidget->width() > 0) {
m_attachment = m_editWidget->getAttachment();
m_previewWidget->openAttachment(m_attachment, attachments::OpenMode::ReadOnly);
}
});
connect(m_previewUpdateTimer, &QTimer::timeout, this, &TextAttachmentsWidget::updatePreviewWidget);
connect(m_editWidget,
&TextAttachmentsEditWidget::scrollChanged,
m_previewWidget,
&TextAttachmentsPreviewWidget::matchScroll);

connect(
m_editWidget, &TextAttachmentsEditWidget::textChanged, m_previewUpdateTimer, QOverload<>::of(&QTimer::start));

connect(m_editWidget, &TextAttachmentsEditWidget::previewButtonClicked, [this] {
const auto sizes = m_splitter->sizes();
const auto previewSize = sizes.value(1, 0) > 0 ? 0 : 1;
m_splitter->setSizes({1, previewSize});
// Split the display in half if showing the preview widget
const auto previewSize = m_previewWidget->width() > 0 ? 0 : m_splitter->width() / 2;
const auto editSize = m_splitter->width() - previewSize;
m_splitter->setSizes({editSize, previewSize});
updatePreviewWidget();
});

// Check if the preview panel is manually collapsed or shown
connect(m_splitter, &QSplitter::splitterMoved, this, [this](int, int) {
// Trigger a preview update if it has become visible
auto visible = m_previewWidget->width() > 0;
if (visible && !m_previewVisible) {
updatePreviewWidget();
}
m_previewVisible = visible;
});

m_splitter->addWidget(m_editWidget);
m_splitter->addWidget(m_previewWidget);
// Prevent collapsing of the edit widget
m_splitter->setCollapsible(0, false);

m_ui->verticalLayout->addWidget(m_splitter);
// Setup this widget with the splitter
auto layout = new QVBoxLayout(this);
layout->setContentsMargins(0, 0, 0, 0);
layout->addWidget(m_splitter);
setLayout(layout);
setObjectName("TextAttachmentsWidget");

updateWidget();
}
7 changes: 5 additions & 2 deletions src/gui/entry/attachments/TextAttachmentsWidget.h
Original file line number Diff line number Diff line change
Expand Up @@ -43,15 +43,18 @@ class TextAttachmentsWidget : public QWidget
void openAttachment(attachments::Attachment attachment, attachments::OpenMode mode);
attachments::Attachment getAttachment() const;

private slots:
void updatePreviewWidget();

private:
void updateWidget();
void initWidget();
void updateWidget();

QScopedPointer<Ui::TextAttachmentsWidget> m_ui;
QPointer<QSplitter> m_splitter;
QPointer<TextAttachmentsEditWidget> m_editWidget;
QPointer<TextAttachmentsPreviewWidget> m_previewWidget;
QPointer<QTimer> m_previewUpdateTimer;
bool m_previewVisible = false;

attachments::Attachment m_attachment;
attachments::OpenMode m_mode;
Expand Down
36 changes: 0 additions & 36 deletions src/gui/entry/attachments/TextAttachmentsWidget.ui

This file was deleted.

Loading