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.

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

Improve entry attachments view #1298

Merged

Conversation

frostasm
Copy link
Contributor

@frostasm frostasm commented Dec 16, 2017

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

image

Details view

image

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]

@@ -130,7 +130,7 @@
<item>
<widget class="QTabWidget" name="tabWidget">
<property name="currentIndex">
<number>0</number>
<number>4</number>
Copy link
Contributor

Choose a reason for hiding this comment

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

Revert this to 0. QtCreator mixed this up

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@@ -122,9 +122,11 @@ void TestEntryModel::testAttachmentsModel()
entryAttachments->set("first", QByteArray("123"));

entryAttachments->set("2nd", QByteArray("456"));
entryAttachments->set("2nd", QByteArray("789"));
entryAttachments->set("2nd", QByteArray("7895"));
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

С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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

@@ -338,3 +359,28 @@ void DetailsWidget::updateTabIndex(int index)
m_selectedTabEntry = index;
}
}

void DetailsWidget::openAttachment(const QModelIndex& index)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree with you. I'll fix it tonight.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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).

Copy link
Contributor

Choose a reason for hiding this comment

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

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 frostasm force-pushed the improve-entry-attachments-view branch 2 times, most recently from 149b117 to 8f8aea4 Compare December 19, 2017 08:23
@frostasm
Copy link
Contributor Author

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

@frostasm frostasm force-pushed the improve-entry-attachments-view branch from 70749c4 to 35e3670 Compare December 20, 2017 09:10
@frostasm frostasm force-pushed the improve-entry-attachments-view branch from 35e3670 to a80e078 Compare December 22, 2017 13:42
@frostasm
Copy link
Contributor Author

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

@fonic
Copy link
Contributor

fonic commented Dec 23, 2017

@frostasm this is great, good work!

@TheZ3ro
Copy link
Contributor

TheZ3ro commented Dec 23, 2017

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

@TheZ3ro TheZ3ro requested a review from a team December 23, 2017 12:53
@TheZ3ro TheZ3ro added this to the v2.3.0 milestone Dec 23, 2017
@frostasm
Copy link
Contributor Author

@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?

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
Copy link
Contributor

TheZ3ro commented Dec 23, 2017

@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
Copy link
Contributor Author

@TheZ3ro okay 👍

@frostasm
Copy link
Contributor Author

@droidmonkey can you review the changes?

@droidmonkey
Copy link
Member

Probably not for a while

Copy link
Member

@phoerious phoerious left a comment

Choose a reason for hiding this comment

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

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

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) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@phoerious phoerious Dec 24, 2017

Choose a reason for hiding this comment

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

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).

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");
Copy link
Member

Choose a reason for hiding this comment

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

All three tab labels need to be translatable.

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

@phoerious phoerious Dec 24, 2017

Choose a reason for hiding this comment

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

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.

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

if (m_ui->tabWidget->count() > 2) {
Copy link
Member

Choose a reason for hiding this comment

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

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

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);
Copy link
Member

Choose a reason for hiding this comment

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

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


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

Ui::EntryAttachmentsWidget* m_ui;
Copy link
Member

Choose a reason for hiding this comment

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

Use QScopedPointer and remove delete from destructor.


Ui::EntryAttachmentsWidget* m_ui;
EntryAttachments* m_entryAttachments;
EntryAttachmentsModel* const m_attachmentsModel;
Copy link
Member

Choose a reason for hiding this comment

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

These two should be QPointers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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...

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

return errors.isEmpty();
}

bool EntryAttachmentsWidget::openAttachment(const QModelIndex& index, QString* errorMessage)
Copy link
Member

Choose a reason for hiding this comment

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

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

<width>400</width>
<height>366</height>
<width>448</width>
<height>370</height>
Copy link
Member

Choose a reason for hiding this comment

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

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

<x>0</x>
<y>0</y>
<width>589</width>
<height>300</height>
Copy link
Member

Choose a reason for hiding this comment

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

Remove widget sizes.

@frostasm frostasm force-pushed the improve-entry-attachments-view branch 2 times, most recently from 3065eb8 to adef56e Compare December 24, 2017 16:55
@frostasm frostasm force-pushed the improve-entry-attachments-view branch from adef56e to 2045342 Compare December 24, 2017 18:10
@frostasm
Copy link
Contributor Author

@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
Copy link
Member

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

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

@phoerious I think I finished :D

Copy link
Member

@phoerious phoerious left a comment

Choose a reason for hiding this comment

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

Just one follow-up request, rest looks fab.

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

@phoerious phoerious Dec 25, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done ;)

Copy link
Member

Choose a reason for hiding this comment

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

Much nicer and shorter. Thanks.

@frostasm frostasm force-pushed the improve-entry-attachments-view branch from accb902 to 4721c9f Compare December 25, 2017 12:23
@frostasm frostasm force-pushed the improve-entry-attachments-view branch from 4721c9f to 80636ab Compare December 25, 2017 12:36
@phoerious phoerious merged commit d7a83f1 into keepassxreboot:develop Dec 25, 2017
@frostasm
Copy link
Contributor Author

@phoerious @TheZ3ro thanks :)

@frostasm frostasm deleted the improve-entry-attachments-view branch December 25, 2017 13:13
@phoerious
Copy link
Member

Well, thank you. 😄

phoerious added a commit that referenced this pull request Feb 27, 2018
- 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]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants