Refactor details widget #1338

frostasm · 2017-12-29T22:10:31Z

Refactor details widget

Description

Motivation and context

The attempt to simplify the codebase :)

How has this been tested?

Manually

Screenshots (if appropriate):

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]
❌ My change requires a change to the documentation and I have updated it accordingly.
❌ I have added tests to cover my changes.

TheZ3ro · 2017-12-29T23:04:14Z

src/gui/DetailsWidget.cpp

 {
-    if (!selectedEntry) {


Why removing this single check and adding checks in every update function?

Also, in theory m_currentEntry should never be equal to nullptr but in such case I prefer hiding the detailsView instead of having it blank

I was planning to make some changes to the logic of the widget displaying (in my opinion it is better that widget was always visible, and the fields cleared when there are no selected items), but it's probably better to discuss this in another PR. I'll revert some changes in the next hour :)

TheZ3ro · 2017-12-29T23:05:07Z

Great work! 👍 for the ElidedLabel. Just a note, see my comment above

phoerious · 2017-12-29T23:06:41Z

This PR is huge. I won't be able to review it before beginning of next year.

adolfogc · 2017-12-30T00:29:05Z

src/gui/widgets/ElidedLabel.h

+    Q_PROPERTY(QString displayText READ displayText WRITE setDisplayText NOTIFY displayTextChanged)
+    Q_PROPERTY(QString url READ url WRITE setUrl NOTIFY urlChanged)
+public:
+    explicit ElidedLabel(QWidget *parent=Q_NULLPTR, Qt::WindowFlags f=Qt::WindowFlags());


Q_NULLPTR -> nullptr

phoerious · 2018-01-06T19:32:43Z

src/gui/DetailsWidget.cpp

    } else {
-        m_ui->passwordLabel->setText("****");
+        m_ui->entryPasswordLabel->setDisplayText("****");
+        m_ui->entryPasswordLabel->setToolTip(QString());


What are the reasons for using an empty string but not a null string?
Qt docs -> distinction between null and empty strings

You can also use {} instead, which will be a null string.

You can also use {} instead, which will be a null string.

done

phoerious · 2018-01-06T19:42:16Z

src/gui/widgets/ElidedLabel.cpp

+    emit elideModeChanged(m_elideMode);
+}
+
+void ElidedLabel::setDisplayText(const QString& elidedText)


Shouldn't this be called setRawText()?

phoerious · 2018-01-06T19:49:22Z

src/gui/DetailsWidget.cpp

 }

 DetailsWidget::~DetailsWidget()
 {
 }

-void DetailsWidget::getSelectedEntry(Entry* selectedEntry)
+void DetailsWidget::setEntry(Entry* selectedEntry)


You should also update the entry if the currently selected one was changed.

phoerious · 2018-01-06T19:52:13Z

src/gui/DetailsWidget.cpp

+    updateEntryAttachmentsTab();
+    updateEntryAutotypeTab();
+
+    setVisible(!config()->get("GUI/HideDetailsView").toBool());


This shouldn't really be a setting with a checkbox IMHO. The widget has a close button right now, but it's extremely frustrating, because the panel comes back as soon as I select another entry. This setting should be set to false whenever I click the close button and instead of a checkbox in the application settings, there should be a little toolbar button at the bottom edge of the screen to restore a previously closed panel (effectively setting this back to true).

There are currently 3 ways to hide the detailsView:

Permanently in the settings (some user asked for this a while back since the option was available in KeePassX 0.4)

Dragging down the details panel, you can restore it by dragging it up (the case you are describing, also available in KeePassX 0.4)

Clicking the close button (hiding it temporary)

The first two are completely unnecessary if the third one would do what I expect it to do.

The thing missing it's an option in the menubar like KeePassX 0.4 below

As I said: I find that totally unnecessary. It only adds additional complexity although it could be as simple as hiding and restoring it in place with the state being remembered. I wouldn't use KeePassX 0.4 as a guideline for how things have to be.

Dragging it down is also not discoverable. And if it's hidden that way, it's gone completely with no indication left that there was once a panel.

@phoerious @TheZ3ro guys can you describe what changes I need to make?

I would prefer it as I described above. Remove the settings checkbox and instead use the setting to automatically remember the panel state. Add a tool button at the bottom to toggle panel visibility.

It might be better to add a show/hide button to the toolbar? KeePassXC doesn't have as many panels as IntelliJ IDEA

Maybe we can fix it in a follow-up PR when we decide a better method to hide it

I can offer to add this to the 'View' menu, scheduled as follow-up for PR #1305:

TheZ3ro · 2018-01-22T23:38:19Z

@frostasm please align this PR to the DetailsWidget changes made in #1414
after that I think we can merge this

adolfogc · 2018-01-22T23:46:22Z

src/gui/DetailsWidget.cpp

+    if (hasTotp) {
+        m_step = m_currentEntry->totpStep();
+        updateTotp();
+        m_totpTimer->start(m_step * 10);


m_timer->start(m_step * 1000); (As fixed in #1414).

m_timer->start(m_step * 1000);
@adolfogc @TheZ3ro @phoerious
As for me, there's a bug. If we select an element at the twentieth second of the current round (the whole round takes for example 30 seconds), then first ten seconds will be correct and next twentieth not.

Then you should use that as an offset. But letting the timer time out every few milliseconds is just a waste of processing power.

I will try to fix it tomorrow

I'd also be okay with running it unconditionally every second, but m_step * 10 makes no sense.

Done. I set the timer for one second

adolfogc · 2018-01-22T23:47:28Z

src/gui/DetailsWidget.h


 private:
+    void setTabEnabled(QTabWidget *tabWidget, QWidget* widget, bool enabled);


QTabWidget *tabWidget -> QTabWidget* tabWidget.

TheZ3ro · 2018-01-30T13:19:45Z

This needs to be merged in 2.3.0, @frostasm are you still working on this?

frostasm · 2018-01-30T14:03:12Z

@TheZ3ro unfotrunately got no time to finish changes, will try to do it by the end of this week

phoerious · 2018-02-01T20:42:30Z

What's the status on this? Will you be able to finish towards the end of the week?

frostasm · 2018-02-01T21:50:37Z

@phoerious if we consider this patch just as a refactoring of the code base, then I think he's already completed

frostasm · 2018-02-02T08:58:49Z

@phoerious I would prefer to leave this patch in its current state (do not change the behavior of a widget to be displayed)

phoerious · 2018-02-04T13:34:29Z

I'm okay with that. Could you rebase to release/2.3.0 please?

frostasm · 2018-02-04T13:43:10Z

yes

phoerious · 2018-02-04T13:47:09Z

GitHub still shows it as being out of date. Are you sure you rebased to release/2.3.0?

frostasm · 2018-02-04T13:50:06Z

sorry, I forgot to pull the changes from upstream. I need few minutes.

phoerious · 2018-02-04T13:56:34Z

Thanks!

- 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 29, 2017

View reviewed changes

adolfogc reviewed Dec 30, 2017

View reviewed changes

frostasm force-pushed the refactor-details-widget branch from 3ef5004 to 2958762 Compare December 30, 2017 09:59

phoerious requested changes Jan 6, 2018

View reviewed changes

phoerious added this to the v2.3.0 milestone Jan 6, 2018

phoerious added the user interface label Jan 6, 2018

frostasm force-pushed the refactor-details-widget branch 2 times, most recently from bbcb82b to cd79289 Compare January 14, 2018 12:21

TheZ3ro approved these changes Jan 16, 2018

View reviewed changes

TheZ3ro mentioned this pull request Jan 22, 2018

Segmentation fault when deleting entry with TOTP Seed #1360

Closed

frostasm force-pushed the refactor-details-widget branch from cd79289 to 26a9dac Compare January 22, 2018 20:48

adolfogc reviewed Jan 22, 2018

View reviewed changes

frostasm force-pushed the refactor-details-widget branch from 26a9dac to 139f779 Compare January 31, 2018 21:00

phoerious changed the base branch from develop to release/2.3.0 February 1, 2018 20:41

frostasm force-pushed the refactor-details-widget branch from 139f779 to c7880c8 Compare February 1, 2018 21:40

frostasm force-pushed the refactor-details-widget branch from c7880c8 to ce2ef52 Compare February 2, 2018 08:33

phoerious approved these changes Feb 4, 2018

View reviewed changes

frostasm force-pushed the refactor-details-widget branch from ce2ef52 to 6b051c4 Compare February 4, 2018 13:45

frostasm added 2 commits February 4, 2018 15:51

Refactor DetailsWidget

6007e0d

Use ElidedLabel in DetailsWidget

b264614

frostasm force-pushed the refactor-details-widget branch from 6b051c4 to b264614 Compare February 4, 2018 13:51

phoerious merged commit 90eea14 into keepassxreboot:release/2.3.0 Feb 4, 2018


		private:
		void setTabEnabled(QTabWidget tabWidget, QWidget widget, bool enabled);

Refactor details widget #1338

Refactor details widget #1338

Conversation

frostasm commented Dec 29, 2017 • edited Loading

Description

Motivation and context

How has this been tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheZ3ro commented Dec 29, 2017 • edited Loading

phoerious commented Dec 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phoerious Jan 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fonic Jan 14, 2018 • edited Loading

Choose a reason for hiding this comment

TheZ3ro commented Jan 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheZ3ro commented Jan 30, 2018

frostasm commented Jan 30, 2018

phoerious commented Feb 1, 2018 • edited Loading

frostasm commented Feb 1, 2018

frostasm commented Feb 2, 2018

phoerious commented Feb 4, 2018

frostasm commented Feb 4, 2018 • edited Loading

phoerious commented Feb 4, 2018

frostasm commented Feb 4, 2018

phoerious commented Feb 4, 2018

frostasm commented Dec 29, 2017 •

edited

Loading

TheZ3ro commented Dec 29, 2017 •

edited

Loading

phoerious Jan 6, 2018 •

edited

Loading

fonic Jan 14, 2018 •

edited

Loading

phoerious commented Feb 1, 2018 •

edited

Loading

frostasm commented Feb 4, 2018 •

edited

Loading