Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add New/Preview Entry Attachments dialog and functionality #11637

Merged
merged 10 commits into from
Jan 12, 2025
Merged

Add New/Preview Entry Attachments dialog and functionality #11637

merged 10 commits into from
Jan 12, 2025

Conversation

w15eacre
Copy link
Contributor

@w15eacre w15eacre commented Jan 7, 2025 •
edited by droidmonkey
Loading

  • This change adds a new opportunity to add attachments that don’t require a real file in the file system.
  • Add a new dialog window to add attachments and integrate it into the EntryAttachmentsWidget.
  • Add an error message if the file name is invalid and disable the save button.
  • Add the preview button.
  • Add preview with support for: images and text.

Screenshots

save_open_buttons_preview
attachments
new
read_only
unkown
preview

Testing strategy

  • Manual testing

Type of change

  • ✅ New feature (change that adds functionality)

Closes #3383
Closes #11506

@w15eacre w15eacre force-pushed the feature/add_attachments_new_button branch from eae01f6 to df95ad6 Compare January 7, 2025 20:37
@droidmonkey
Copy link
Member

droidmonkey commented Jan 7, 2025 •
edited
Loading

Can we pair this with an attachment preview option? Same dialog just in reverse. Can also add an image control on there in case contents are not text.

@w15eacre w15eacre force-pushed the feature/add_attachments_new_button branch from 47d1bb0 to abf2159 Compare January 8, 2025 18:26
@w15eacre
Copy link
Contributor Author

w15eacre commented Jan 8, 2025

@droidmonkey Added text and image preview button.

@w15eacre w15eacre changed the title Add New Entry Attachments dialog and functionality Add New/Preview Entry Attachments dialog and functionality Jan 8, 2025
@droidmonkey
Copy link
Member

Love it!

@w15eacre w15eacre force-pushed the feature/add_attachments_new_button branch from abf2159 to e00aec9 Compare January 8, 2025 23:20
@xboxones1
Copy link
Contributor

xboxones1 commented Jan 9, 2025 •
edited
Loading

Maybe we should remove the "Preview" button and open supported formats by default in the preview window? And for unsupported formats, add a warning that the file will be decrypted and opened in other software. For an example you can see how it is implemented in classic keepass2.

There is also a bug with the preview window style, you can even see it on the screenshots above. If you open it from the entry, everything is fine, if you open it from the "preview panel", it doesn't follow the style.

12

@w15eacre
Copy link
Contributor Author

w15eacre commented Jan 9, 2025 •
edited
Loading

Maybe we should remove the "Preview" button and open supported formats by default in the preview window? And for unsupported formats, add a warning that the file will be decrypted and opened in other software. For an example you can see how it is implemented in classic keepass2.

There is also a bug with the preview window style, you can even see it on the screenshots above. If you open it from the entry, everything is fine, if you open it from the "preview panel", it doesn't follow the style.

12

I will fix this issue with the style. It's strange, because the same window is shown

@w15eacre
Copy link
Contributor Author

w15eacre commented Jan 9, 2025

If we replace the "Open" button, then we should be able to edit the file, but I don't understand how to edit non-text files (PDF, images, ...). I want to support preview PDF, because some services save recovery codes in this format.

@w15eacre
Copy link
Contributor Author

w15eacre commented Jan 9, 2025

@xboxones1 I fixed styles.

I'll check the keepass2 to understand its behavior.

@droidmonkey What do you think about this?

@droidmonkey droidmonkey added user interface ux pr: new feature Pull request that adds a new feature labels Jan 9, 2025
@droidmonkey droidmonkey added this to the v2.7.10 milestone Jan 9, 2025
@droidmonkey droidmonkey self-requested a review January 9, 2025 18:03
Fix read-only preview
3f91ffe 
Refactor EntryAttachmentsWidget and PreviewEntryAttachmentsDialog to remove unnecessary parent references

Fixes keepassxreboot#11506
@w15eacre w15eacre force-pushed the feature/add_attachments_new_button branch from 1c1483a to 3f91ffe Compare January 9, 2025 18:10
@droidmonkey
Copy link
Member

It works great, although for some reason wouldn't let me preview keeagent.settings which is XML (I think, maybe its Base64).

image

I think we can make a couple more enhancements. See above for button adds. We can likely get rid of "Rename" by allowing for direct rename within the list widget.

We should create a little grouping in the button list as well:

image

@w15eacre
Copy link
Contributor Author

w15eacre commented Jan 9, 2025

Added support for xml, json, yaml, soap, protobuf.

Next time I'll start refactoring the UI

@w15eacre w15eacre marked this pull request as draft January 9, 2025 22:31
@w15eacre
Copy link
Contributor Author

@droidmonkey I discovered strange behavior:
If I open a file and change it, I discard the changes and close the editor application. I see that the data in the database does not match the open file. If you restart the application, the problem goes away. The whole problem is in re-opening the file, you need to delete it if the name is the same

@w15eacre w15eacre force-pushed the feature/add_attachments_new_button branch from 66e0e43 to c941cda Compare January 11, 2025 00:55
@droidmonkey
Copy link
Member

Well, that will need to be fixed 🫣

@droidmonkey
Copy link
Member

@w15eacre I am moving a couple things around, MimeTypes should be in core/Tools.h/cpp for example. Also trying to avoid use of std::arrays since we are singled up on Qt

@droidmonkey
Copy link
Member

droidmonkey commented Jan 11, 2025 •
edited
Loading

Done, also made several improvements to the code.

Only one annoying issue remains, if you save an attachment and it does not prompt to overwrite the file, the selection will deselect. I can't see any reason for this in the code, it appears to be a qt bug.

@droidmonkey
Improve code for preview widget
23c567e 
* Reduce multi-selection to single selection when previewing to avoid opening and saving more than the previewed attachment
* Don't share EntryAttachments with the preview widget, just pass name/data pair
* Reduce number of single-use function calls
@droidmonkey droidmonkey force-pushed the feature/add_attachments_new_button branch from d4edff5 to 23c567e Compare January 11, 2025 16:10
@w15eacre
Copy link
Contributor Author

I can't reproduce this problem.
I've finished refactoring the UI.

@w15eacre w15eacre marked this pull request as ready for review January 11, 2025 18:54
@w15eacre w15eacre force-pushed the feature/add_attachments_new_button branch from 207a15f to 6feea6a Compare January 11, 2025 20:36
@droidmonkey
Final cleanup round
3116ded 
* Fix sizing of attachment columns
* Parent new/preview attachment dialog to widget so it closes properly on database auto-lock
* Fix targeting of preview widget styling to not impact unintended children
* Add padding to attachment table items
@droidmonkey
Copy link
Member

I love everything about this, really enhances the attachment experience!

@droidmonkey droidmonkey merged commit dce34de into keepassxreboot:develop Jan 12, 2025
11 checks passed
@w15eacre w15eacre mentioned this pull request Jan 12, 2025
Open
@droidmonkey droidmonkey added the pr: backported Pull request backported to previous release label Jan 19, 2025
droidmonkey pushed a commit that referenced this pull request Jan 19, 2025
@w15eacre @droidmonkey
Add New/Preview Entry Attachments dialog and functionality (#11637)
99c8936 
Closes #11506
Closes #3383

* This change adds a new opportunity to add attachments that don’t require a real file in the file system.
* Add a new dialog window to add and preview attachments and integrate it into the EntryAttachmentsWidget.
* Attachment preview support for images and plain text files.

Additional enhancements:
* Fix sizing of attachment columns
* Add padding to attachment table items
* Fix targeting of preview widget styling to not impact unintended children
@droidmonkey droidmonkey mentioned this pull request Jan 25, 2025
Closed
1 task
pull bot pushed a commit to Graysonbarton/keepassxc that referenced this pull request Jan 26, 2025
@w15eacre @pull
Add New/Preview Entry Attachments dialog and functionality (keepassxr…
97608b7 
…eboot#11637)

Closes keepassxreboot#11506
Closes keepassxreboot#3383

* This change adds a new opportunity to add attachments that don’t require a real file in the file system.
* Add a new dialog window to add and preview attachments and integrate it into the EntryAttachmentsWidget.
* Attachment preview support for images and plain text files.

Additional enhancements:
* Fix sizing of attachment columns
* Add padding to attachment table items
* Fix targeting of preview widget styling to not impact unintended children
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@droidmonkey droidmonkey droidmonkey approved these changes

Assignees
No one assigned
Labels
pr: backported Pull request backported to previous release pr: new feature Pull request that adds a new feature user interface ux
Projects
None yet
Milestone
v2.7.10
3 participants
@w15eacre @droidmonkey @xboxones1