Fix pre-release issues with attachment viewer #12244
Conversation
Copilot
AI
changed the title
|
Fix the other problems mentioned in the comments of the source issue. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #12244 +/- ##
===========================================
- Coverage 64.28% 64.25% -0.03%
===========================================
Files 376 376
Lines 39309 39339 +30
===========================================
+ Hits 25268 25275 +7
- Misses 14041 14064 +23 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Fixed the remaining attachment editor issues mentioned in the source issue comments. The changes address:
All existing tests pass and the fixes are minimal surgical changes. Fixed in commit f6c6321. |
There was a problem hiding this comment.
Pull Request Overview
This PR ensures proper localization for the attachment editor by moving translation calls into the correct context and marking all hardcoded strings for translation.
- Use
tr("Fit")withinImageAttachmentsWidgetmethods instead of an anonymous namespace - Replace hardcoded
"New Attachment"withtr("New Attachment")and remove the unused constant - Introduce and reset a manual‐selection flag in
TextAttachmentsPreviewWidget, and enable external link opening in its preview browser
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/gui/entry/attachments/TextAttachmentsPreviewWidget.h | Added m_userManuallySelectedType member |
| src/gui/entry/attachments/TextAttachmentsPreviewWidget.cpp | Initialized and reset manual‐selection flag; enabled external links |
| src/gui/entry/attachments/ImageAttachmentsWidget.cpp | Replaced anonymous‐namespace FitText with tr("Fit") |
| src/gui/entry/EntryAttachmentsWidget.cpp | Swapped hardcoded default name for tr("New Attachment") |
| share/translations/keepassxc_en.ts | Added entries for "New Attachment" and "Fit" translations |
Comments suppressed due to low confidence (3)
src/gui/entry/attachments/TextAttachmentsPreviewWidget.h:61
- Add a brief comment explaining the purpose of m_userManuallySelectedType to improve code readability.
bool m_userManuallySelectedType;
src/gui/entry/attachments/TextAttachmentsPreviewWidget.cpp:67
- Consider adding a unit test to verify that the manual type selection flag is correctly reset when opening a new attachment.
// Reset manual selection flag when opening a new attachment
src/gui/entry/attachments/TextAttachmentsPreviewWidget.cpp:100
- Add a GUI test to confirm that the previewTextBrowser indeed opens external links as expected.
m_ui->previewTextBrowser->setOpenExternalLinks(true);
droidmonkey
changed the title
phoerious
added
the
pr: ai-assisted
|
Fixed a bunch of scrolling issues with the preview: 2025-07-04_08-42-52.mp4 |
|
@varjolintu ready for your review/testing |
The main text in the PR could've just been "Fixed a few missing translations with.." or something equally compact :) |
|
Our intern coder gets a little verbose sometimes |
|
|
||
| void TextAttachmentsWidget::updatePreviewWidget() | ||
| { | ||
| m_previewVisible = m_previewWidget->width() > 0; |
There was a problem hiding this comment.
This m_previewWidget->width() > 0 is repeated many times in the file. Maybe create a function for it?
There was a problem hiding this comment.
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.
| @@ -65,7 +65,15 @@ void TextAttachmentsWidget::updateWidget() | |||
| } | |||
|
|
|||
| m_editWidget->openAttachment(m_attachment, m_mode); | |||
| m_previewWidget->openAttachment(m_attachment, attachments::OpenMode::ReadOnly); | |||
There was a problem hiding this comment.
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);
}
There was a problem hiding this comment.
But just implementing the above will do the openAttachment() again when the timer hits.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ahhhhhh, just preview, not edit. The delay is from the 500ms timer for when the attachment text is changed.
…ditor Co-authored-by: droidmonkey <[email protected]>
…ent editor Co-authored-by: droidmonkey <[email protected]>
* Update preview panel if manually moved from collapsed position * Preview is now only updated once on first show instead of three times
* Match edit view scroll position (by percentage) when changed. This ensures the preview remains in relative sync with the edited document, for example when a large amount of HTML reduces down to a short preview document. * Reset the preview scroll back to it's previous position after re-rendering a change. * Fix default preview size to be half the width of the edit widget.
droidmonkey
force-pushed
the
copilot/fix-12243
branch
2 times, most recently
from
ab3ed28 to
511bad8
Compare
droidmonkey
force-pushed
the
copilot/fix-12243
branch
from
511bad8 to
1e6766d
Compare
This PR fixes two translation issues in the attachment editor that prevented proper localization:
Issues Fixed
1. "FIT" translation not working in image attachment zoom controls
The "Fit" text in the image attachment widget zoom dropdown was not being translated because
QObject::tr("Fit")was called in an anonymous namespace, causing the translation context to be lost.Before: Translation system couldn't find the proper context for "Fit"
After:
tr("Fit")is called directly in theImageAttachmentsWidgetclass methods where the translation context is properly available2. "New Attachment" string not marked for translation
The default name for new attachments was hardcoded as a string literal and never marked for translation.
Before:
After:
Technical Details
tr("Fit")calls from anonymous namespace to proper class methods inImageAttachmentsWidgettr("New Attachment")call inEntryAttachmentsWidget::newAttachments()DefaultNameconstant to keep code cleanThe changes are minimal and surgical, following existing code patterns in the project. Both strings now appear correctly in the translation files and will be available for localization.
Fixes #12243.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.