Improve entry attachments view by frostasm · Pull Request #1298 · keepassxreboot/keepassxc

frostasm · 2017-12-16T20:58:17Z

Improve entry attachments view.

Description

Create separate widget to edit/view entry attachments - EntryAttachmentsWidget
Add drag and drop support for attachments view in EntryAttachmentsWidget
Use EntryAttachmentsWidget to display and edit attachments in EditEntryWidget
Add attachments tab to details view - use EntryAttachmentsWidget
Allow to open attachments from details view

Motivation and context

In my opinion, the tableview more user friendly than a listview.

How has this been tested?

Manually.

Screenshots (if appropriate):

EditEntryWidget

Details view

Types of changes

✅ New feature (non-breaking change which adds functionality)

Checklist:

✅ I have read the CONTRIBUTING document. [REQUIRED]
✅ My code follows the code style of this project. [REQUIRED]
✅ All new and existing tests passed. [REQUIRED]
✅ I have compiled and verified my code with -DWITH_ASAN=ON. [REQUIRED]

TheZ3ro · 2017-12-18T14:04:10Z

src/gui/DetailsWidget.ui

    <widget class="QTabWidget" name="tabWidget">
     <property name="currentIndex">
-      <number>0</number>
+      <number>4</number>


Revert this to 0. QtCreator mixed this up

Maybe it is better to activate the first tab in the constructor (to not depend on the form state)?

I think the currentIndex is needed in the xml anyway, just set it to 0 so we don't add extra code for it

TheZ3ro · 2017-12-18T14:08:48Z

tests/TestEntryModel.cpp


    entryAttachments->set("2nd", QByteArray("456"));
-    entryAttachments->set("2nd", QByteArray("789"));
+    entryAttachments->set("2nd", QByteArray("7895"));


Why this change?

Сhanged to have different size ("456" - 3 bytes, "7895" - 4 bytes, I check the size in the next lines). This is not critical, I can remove the changes.

Just to be consistent with the 123-456-789 can you replace the final 5 with a 0 ?
Merely style reasons

TheZ3ro · 2017-12-18T14:11:40Z

src/gui/DetailsWidget.cpp

    }
 }
+
+void DetailsWidget::openAttachment(const QModelIndex& index)


Can't we use the openAttachment from the EditEntryWidget class? Or alternatively declaring it static in a different class and point both of them to the static one? This is just repeated code and I would like to leave the detailsWidget light

Agree with you. I'll fix it tonight.

@TheZ3ro perhaps it is better to create a separate widget to display/edit the attachments. With the functionality of adding, deleting and opening files (which can be turned off in the details view).

It's fine opening files from the details view, it's the extra code that I don't really like.
Making a separate widget and import it will be very nice and convenient

frostasm · 2017-12-19T21:40:05Z

@TheZ3ro I added a widget to edit/view the attachments

frostasm · 2017-12-22T17:09:58Z

@TheZ3ro I rebase changes on top of the develop branch ;)

fonic · 2017-12-23T09:16:30Z

@frostasm this is great, good work!

TheZ3ro · 2017-12-23T12:53:24Z

This is working fine and ready to merge, waiting someone else review, ping @keepassxreboot/core-developers

Nice PR follow-up idea: add keyboard actions for enter and cancel keys for open and remove attachments

frostasm · 2017-12-23T14:00:42Z

@fonic thank you :)

Nice PR follow-up idea: add keyboard actions for enter and cancel keys for open and remove attachments

@TheZ3ro good idea. What keyboard shortcuts would be helpful for each role?

add - insert key
remove - delete key
open - QAbstractItemView::activated() signal
save - ?

Possible improvements:

ask permission to replace the existing attachments, when adding new attachments
add the ability to sort attachments (by name, by size)
add the ability to rename existing attachments

I have free time on the weekends and can implement the described proposal.

TheZ3ro · 2017-12-23T14:29:29Z

@frostasm can you report those proposal in a new issue? So we can track them for a new PR.
This PR is fine as-is

frostasm · 2017-12-23T14:41:40Z

@TheZ3ro okay 👍

frostasm · 2017-12-23T15:33:32Z

@droidmonkey can you review the changes?

droidmonkey · 2017-12-23T18:26:58Z

Probably not for a while

phoerious

All in all a fantastic improvement. I've got a few corrections for you as a Christmas present, though. ;-)

phoerious · 2017-12-24T11:48:59Z

src/gui/DetailsWidget.cpp

-    if (m_ui->tabWidget->count() < 4) {
-        m_ui->tabWidget->insertTab(static_cast<int>(AttributesTab), m_attributesWidget, "Attributes");
-        m_ui->tabWidget->insertTab(static_cast<int>(AutotypeTab), m_autotypeWidget, "Autotype");
+    if (m_ui->tabWidget->count() < 5) {


Can we somehow get rid of this hard-coded number? Maybe create a vector first and use its length or so. Or maybe even clear the tab widget and re-add stuff every time. This number looks awfully easy to overlook.

I completely agree with you about the magic numbers. I think it is necessary to refactor DetailsWidget in another pull request. I can do that after this changes will be merged. If you don't mind.

I think it would be enough to simply clear out the widget and add stuff as needed (I guess it's unlikely that this would impact performance massively).

phoerious · 2017-12-24T11:51:04Z

src/gui/DetailsWidget.cpp

+    if (m_ui->tabWidget->count() < 5) {
+        m_ui->tabWidget->insertTab(static_cast<int>(AttributesTab), m_attributesTabWidget, "Attributes");
+        m_ui->tabWidget->insertTab(static_cast<int>(AttachmentsTab), m_attachmentsTabWidget, "Attachments");
+        m_ui->tabWidget->insertTab(static_cast<int>(AutotypeTab), m_autotypeTabWidget, "Autotype");


All three tab labels need to be translatable.

phoerious · 2017-12-24T11:55:25Z

src/gui/DetailsWidget.cpp

@@ -209,9 +226,8 @@ void DetailsWidget::getSelectedGroup(Group* selectedGroup)
    m_ui->stackedWidget->setCurrentIndex(GroupPreview);


A general note about getSelectedEntry and getSelected group: I know this is not your code, but these methods shouldn't be called get<WHATEVER> if they have side-effects.

phoerious · 2017-12-24T11:57:06Z

src/gui/DetailsWidget.cpp

@@ -209,9 +226,8 @@ void DetailsWidget::getSelectedGroup(Group* selectedGroup)
    m_ui->stackedWidget->setCurrentIndex(GroupPreview);

    if (m_ui->tabWidget->count() > 2) {


See comment above about magic numbers. At this point I would actually prefer clearing out the tab widget and re-adding needed items unconditionally.

phoerious · 2017-12-24T12:04:16Z

src/gui/entry/EntryAttachmentsWidget.cpp

+    m_attachmentsModel->setEntryAttachments(m_entryAttachments);
+    m_ui->attachmentsView->setModel(m_attachmentsModel);
+    m_ui->attachmentsView->horizontalHeader()->setStretchLastSection(true);
+    m_ui->attachmentsView->horizontalHeader()->resizeSection(EntryAttachmentsModel::NameColumn, 400);


I would prefer if you disabled row numbers here. They make the table look like a spreadsheet.

phoerious · 2017-12-24T13:33:09Z

src/gui/entry/EntryAttachmentsWidget.h

+
+    bool eventFilter(QObject* watched, QEvent* event) override;
+
+    Ui::EntryAttachmentsWidget* m_ui;


Use QScopedPointer and remove delete from destructor.

phoerious · 2017-12-24T13:43:07Z

src/gui/entry/EntryAttachmentsWidget.h

+
+    Ui::EntryAttachmentsWidget* m_ui;
+    EntryAttachments* m_entryAttachments;
+    EntryAttachmentsModel* const m_attachmentsModel;


These two should be QPointers.

I think that it is not necessary, they are only used internally (they are not re-assigned in the process of work)

No, but it also doesn't hurt anybody and may catch errors further down the road. Who knows where these objects are used in 5 months...

BTW I really only mean QPointers, not QSharedPointers or QScopedPointers. QPointers simply guard the pointer and reset to 0 if the QObject is deallocated. They don't count references or own anything and they can be converted to and from raw pointers.

phoerious · 2017-12-24T13:45:51Z

src/gui/entry/EntryAttachmentsWidget.cpp

+    return errors.isEmpty();
+}
+
+bool EntryAttachmentsWidget::openAttachment(const QModelIndex& index, QString* errorMessage)


non-const reference for errorMessage instead of pointer to QString.

phoerious · 2017-12-24T13:46:51Z

src/gui/entry/EditEntryWidgetAdvanced.ui

-    <width>400</width>
-    <height>366</height>
+    <width>448</width>
+    <height>370</height>


You should completely remove widget sizes here (click the little reset button in QtDesigner's property view).

phoerious · 2017-12-24T13:47:05Z

src/gui/entry/EntryAttachmentsWidget.ui

+    <x>0</x>
+    <y>0</y>
+    <width>589</width>
+    <height>300</height>


Remove widget sizes.

frostasm · 2017-12-24T22:09:58Z

@phoerious thanks for the detailed review 👍. About magic numbers and bad function names. If you don't mind, I would refactor this widget in my next PR. Because current pull request relate only to the introduction of the new functionality and not to the refactoring of the existing one.

phoerious · 2017-12-25T00:47:02Z

Ok, fair enough. Just don't forget about it.

frostasm · 2017-12-25T09:57:39Z

@phoerious I think I finished :D

phoerious

Just one follow-up request, rest looks fab.

phoerious · 2017-12-25T11:28:18Z

src/gui/entry/EntryAttachmentsModel.cpp

+    if (role == Qt::DisplayRole) {
+        const QString key = keyByIndex(index);
+        switch (column) {
+        case Columns::NameColumn:


I would replace the inner switch/case statements with if/else if as well. Perhaps even flatten this structure to a single level, since DisplayRole && NameColumn have the same action as EditRole && NameColumn, so it can be reduced to a single condition. DisplayRole && SizeColumn and EditRole && SizeColumn are also very similar, so they could be reduced to a return guard and another return if the previous condition wasn't met.

Much nicer and shorter. Thanks.

frostasm · 2017-12-25T13:13:02Z

@phoerious @TheZ3ro thanks :)

phoerious · 2017-12-25T13:13:38Z

Well, thank you. 😄

- Add support for KDBX 4.0, Argon2 and ChaCha20 [#148, #1179, #1230, #1494] - Add SSH Agent feature [#1098, #1450, #1463] - Add preview panel with details of the selected entry [#879, #1338] - Add more and configurable columns to entry table and allow copying of values by double click [#1305] - Add KeePassXC-Browser API as a replacement for KeePassHTTP [#608] - Deprecate KeePassHTTP [#1392] - Add support for Steam one-time passwords [#1206] - Add support for multiple Auto-Type sequences for a single entry [#1390] - Adjust YubiKey HMAC-SHA1 challenge-response key generation for KDBX 4.0 [#1060] - Replace qHttp with cURL for website icon downloads [#1460] - Remove lock file [#1231] - Add option to create backup file before saving [#1385] - Ask to save a generated password before closing the entry password generator [#1499] - Resolve placeholders recursively [#1078] - Add Auto-Type button to the toolbar [#1056] - Improve window focus handling for Auto-Type dialogs [#1204, #1490] - Auto-Type dialog and password generator can now be exited with ESC [#1252, #1412] - Add optional dark tray icon [#1154] - Add new "Unsafe saving" option to work around saving problems with file sync services [#1385] - Add IBus support to AppImage and additional image formats to Windows builds [#1534, #1537] - Add diceware password generator to CLI [#1406] - Add --key-file option to CLI [#816, #824] - Add DBus interface for opening and closing KeePassXC databases [#283] - Add KDBX compression options to database settings [#1419] - Discourage use of old fixed-length key files in favor of arbitrary files [#1326, #1327] - Correct reference resolution in entry fields [#1486] - Fix window state and recent databases not being remembered on exit [#1453] - Correct history item generation when configuring TOTP for an entry [#1446] - Correct multiple TOTP bugs [#1414] - Automatic saving after every change is now a default [#279] - Allow creation of new entries during search [#1398] - Correct menu issues on macOS [#1335] - Allow compilation on OpenBSD [#1328] - Improve entry attachments view [#1139, #1298] - Fix auto lock for Gnome and Xfce [#910, #1249] - Don't remember key files in file dialogs when the setting is disabled [#1188] - Improve database merging and conflict resolution [#807, #1165] - Fix macOS pasteboard issues [#1202] - Improve startup times on some platforms [#1205] - Hide the notes field by default [#1124] - Toggle main window by clicking tray icon with the middle mouse button [#992] - Fix custom icons not copied over when databases are merged [#1008] - Allow use of DEL key to delete entries [#914] - Correct intermittent crash due to stale history items [#1527] - Sanitize newline characters in title, username and URL fields [#1502] - Reopen previously opened databases in correct order [#774] - Use system's zxcvbn library if available [#701] - Implement various i18n improvements [#690, #875, #1436]

TheZ3ro reviewed Dec 18, 2017

View reviewed changes

frostasm force-pushed the improve-entry-attachments-view branch 2 times, most recently from 149b117 to 8f8aea4 Compare December 19, 2017 08:23

frostasm force-pushed the improve-entry-attachments-view branch from 70749c4 to 35e3670 Compare December 20, 2017 09:10

fonic mentioned this pull request Dec 21, 2017

Improve and extend entry view table #1305

Merged

TheZ3ro approved these changes Dec 21, 2017

View reviewed changes

frostasm force-pushed the improve-entry-attachments-view branch from 35e3670 to a80e078 Compare December 22, 2017 13:42

TheZ3ro requested a review from a team December 23, 2017 12:53

TheZ3ro added the user interface label Dec 23, 2017

TheZ3ro added this to the v2.3.0 milestone Dec 23, 2017

phoerious requested changes Dec 24, 2017

View reviewed changes

frostasm force-pushed the improve-entry-attachments-view branch 2 times, most recently from 3065eb8 to adef56e Compare December 24, 2017 16:55

frostasm added 3 commits December 24, 2017 20:10

Use a table view to display the list of attachments in EditEntryWidget

bf5b963

Add drag and drop support for attachments view in EditEntryWidget

90a64db

Add attachments tab to details view

0c200d3

frostasm force-pushed the improve-entry-attachments-view branch from adef56e to 2045342 Compare December 24, 2017 18:10

frostasm force-pushed the improve-entry-attachments-view branch 2 times, most recently from 097c67b to accb902 Compare December 25, 2017 09:31

phoerious requested changes Dec 25, 2017

View reviewed changes

frostasm force-pushed the improve-entry-attachments-view branch from accb902 to 4721c9f Compare December 25, 2017 12:23

frostasm added 3 commits December 25, 2017 14:36

Fix failing entry model test for attachment name and size

ed0bda9

Move attachments view to the separate widget EntryAttachmentsWidget

c490c21

Use EntryAttachmentsWidget in details view

80636ab

frostasm force-pushed the improve-entry-attachments-view branch from 4721c9f to 80636ab Compare December 25, 2017 12:36

phoerious approved these changes Dec 25, 2017

View reviewed changes

phoerious merged commit d7a83f1 into keepassxreboot:develop Dec 25, 2017

frostasm deleted the improve-entry-attachments-view branch December 25, 2017 13:13

		@@ -209,9 +226,8 @@ void DetailsWidget::getSelectedGroup(Group* selectedGroup)
		m_ui->stackedWidget->setCurrentIndex(GroupPreview);

		@@ -209,9 +226,8 @@ void DetailsWidget::getSelectedGroup(Group* selectedGroup)
		m_ui->stackedWidget->setCurrentIndex(GroupPreview);

		if (m_ui->tabWidget->count() > 2) {


		bool eventFilter(QObject* watched, QEvent* event) override;

		Ui::EntryAttachmentsWidget* m_ui;

Uh oh!

Conversation

frostasm commented Dec 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and context

How has this been tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frostasm commented Dec 19, 2017

Uh oh!

frostasm commented Dec 22, 2017

Uh oh!

fonic commented Dec 23, 2017

Uh oh!

TheZ3ro commented Dec 23, 2017

Uh oh!

frostasm commented Dec 23, 2017

Uh oh!

TheZ3ro commented Dec 23, 2017

Uh oh!

frostasm commented Dec 23, 2017

Uh oh!

frostasm commented Dec 23, 2017

Uh oh!

droidmonkey commented Dec 23, 2017

Uh oh!

phoerious left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phoerious Dec 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phoerious Dec 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

frostasm commented Dec 16, 2017 •

edited

Loading

phoerious left a comment •

edited

Loading

phoerious Dec 24, 2017 •

edited

Loading

phoerious Dec 24, 2017 •

edited

Loading

phoerious Dec 25, 2017 •

edited

Loading