fix showing any notes in an entry without notes (Fixes #6461) by xvallspl · Pull Request #6481 · keepassxreboot/keepassxc

xvallspl · 2021-05-04T06:52:30Z

Fixes Issue #6461

Entries without notes were not clearing the notes field in the entry preview widget. I'm not clearing them either, but making the field invisible (it will get overwritten by next entry with notes).

Screenshots

(gif)

Testing strategy

Manual testing

Type of change

✅ Bug fix (non-breaking change that fixes an issue)

michaelk83 · 2021-05-04T11:36:30Z

src/gui/EntryPreviewWidget.cpp

+    if (m_currentEntry->notes().isEmpty()) {
+        m_ui->entryNotesTextEdit->setVisible(false);
        m_ui->toggleEntryNotesButton->setVisible(false);
+    } else {
+        m_ui->entryNotesTextEdit->setVisible(true);
+        if (config()->get(Config::Security_HideNotes).toBool()) {
+            setEntryNotesVisible(false);
+            m_ui->toggleEntryNotesButton->setVisible(!m_ui->entryNotesTextEdit->toPlainText().isEmpty());
+            m_ui->toggleEntryNotesButton->setChecked(false);
+        } else {
+            setEntryNotesVisible(true);
+            m_ui->toggleEntryNotesButton->setVisible(false);
+        }
    }


I'm not familiar with the rest of KPXC's code, nor with Qt yet, but just taking these lines on their own, this looks messy. A bunch of branching code setting multiple inter-dependent state in a complex manner. This screams "bugs!" at me.

First, why not actually clear the notes member in the model by default, when it's initialized? Or always load the notes from the entry, so that here it would get overwritten by an empty string? That's a more sensible solution to me than this UI hack.

Related to that, having two different ways to check if the notes are empty (lines 243 and 250) feels wrong. I understand these check slightly different things, but if the above point is fixed, then the 2nd form (line 250) should work for both (and can be checked once).

From line 250, m_ui->entryNotesTextEdit, it appears this can be editable in some cases? If so, then you shouldn't be hiding the edit box just because it's initially empty (line 244). Or, if it's never editable here, then why is this an edit box and not a label?

(For that matter, if it's editable, why are the "edit" and "view" views not separated to begin with? Though I realize that separating them is beyond the scope of this PR.)

Ignoring the above, if you go ahead with this UI approach, I would at least rearrange the code to reduce the branching complexity. How about this:

Check the two conditions in advance, and put their results in boolean variables, isNotesEmpty and isHideNotes (or whatever the naming convention is).

Set each boolean state exactly once, passing the correct combination of conditions to the boolean parameter. For example, m_ui->toggleEntryNotesButton->setVisible(isHideNotes && !isNotesEmpty);. This would clearly spell out what the compound condition is for each state.

Finally, this could really use some unit tests, if possible (if there already are, then at least for the new bug scenario). And ideally they should be written before changing the actual code.

I realize @droidmonkey has already approved this PR, and it's not my place to overrule, but these are just some points to consider.
(and PS, I also realize it's not a critical piece of code at all, but it just bothered me when I saw this.)

There are a lot of quirks with this dialog to make it work and function the way it does. It's not clean, welcome to gui programming.

The old branching is why there was a bug to begin with, this new branching is much much cleaner to follow.

Then how about point 5, at least? I think that would be cleaner still.

Reasonable yes

michaelk83 · 2021-05-07T16:27:08Z

src/gui/EntryPreviewWidget.cpp

+
+    m_ui->entryNotesTextEdit->setVisible(hasNotes);
+    setEntryNotesVisible(hasNotes && !hideNotes);
+    m_ui->toggleEntryNotesButton->setVisible(hasNotes && hideNotes && !m_ui->entryNotesTextEdit->toPlainText().isEmpty());


Can && !m_ui->entryNotesTextEdit->toPlainText().isEmpty() be part of hasNotes (line 243), or would that break line 246?

If you always initialize entryNotesTextEdit with the notes from the current entry, then it'll get reset to an empty string, and then only one of entryNotesTextEdit->toPlainText().isEmpty() or m_currentEntry->notes().isEmpty() should be enough (and you won't have to hide the text edit either).

Can && !m_ui->entryNotesTextEdit->toPlainText().isEmpty() be part of hasNotes (line 243), or would that break line 246?

It would break

If you always initialize entryNotesTextEdit with the notes from the current entry, then it'll get reset to an empty string, and then only one of entryNotesTextEdit->toPlainText().isEmpty() or m_currentEntry->notes().isEmpty() should be enough (and you won't have to hide the text edit either).

That would be the ideal case, but I was trying to change the code as little as possible to fix this bug ASAP 🙃 . That would require changing the setEntryNotesVisible and the setNotesVisible (which is also called by group functionality) functions. There is some work that could be done to simplify things but I'm not sure it is in the scope of this PR.

src/gui/EntryPreviewWidget.cpp

michaelk83 · 2021-05-07T17:21:19Z

The updated code looks much nicer. Thanks!

Fixes #6461

Added - Show search bar when toolbar is hidden or in overflow [#6279] - Show countdown for clipboard clearing in status bar [#6333] - Command line option to lock all open databases [#6511] - Allow CSV import of bare TOTP secrets [#6211] - Retain file creation time when saving database [#6576] - Set permissions of saved attachments to be private to the current user [#6363] - OPVault: Use Text instead of Name for attribute names [#6334] Changed - Reports: Allow resizing of reports columns [#6435] - Reports: Toggle showing expired entries [#6534] - Save Always on Top setting [#6236] - Password generator can exclude additional lookalike characters (6/G, 8/B) [#6196] Fixed - Allow setting MSI properties in unattended install [#6196] - Update MainWindow minimum size to enable smaller verticle space [#6196] - Use application font size when setting default or monospace fonts [#6332] - Fix notes not clearing in entry preview panel in some cases [#6481] - macOS: Correct window activation when restoring from tray [#6575] - macOS: Better handling of minimize after unlock when using browser integration [#6338] - Linux: Start after the system tray is available on LXQt [#6216] - Linux: Allow selection of modal dialogs on X11 in Auto-Type [#6204] - KeeShare: prevent crash when file extension is missing [#6174]

Release 2.6.5 Added - Show search bar when toolbar is hidden or in overflow [keepassxreboot#6279] - Show countdown for clipboard clearing in status bar [keepassxreboot#6333] - Command line option to lock all open databases [keepassxreboot#6511] - Allow CSV import of bare TOTP secrets [keepassxreboot#6211] - Retain file creation time when saving database [keepassxreboot#6576] - Set permissions of saved attachments to be private to the current user [keepassxreboot#6363] - OPVault: Use Text instead of Name for attribute names [keepassxreboot#6334] Changed - Reports: Allow resizing of reports columns [keepassxreboot#6435] - Reports: Toggle showing expired entries [keepassxreboot#6534] - Save Always on Top setting [keepassxreboot#6236] - Password generator can exclude additional lookalike characters (6/G, 8/B) [keepassxreboot#6196] Fixed - Allow setting MSI properties in unattended install [keepassxreboot#6196] - Update MainWindow minimum size to enable smaller verticle space [keepassxreboot#6196] - Use application font size when setting default or monospace fonts [keepassxreboot#6332] - Fix notes not clearing in entry preview panel in some cases [keepassxreboot#6481] - macOS: Correct window activation when restoring from tray [keepassxreboot#6575] - macOS: Better handling of minimize after unlock when using browser integration [keepassxreboot#6338] - Linux: Start after the system tray is available on LXQt [keepassxreboot#6216] - Linux: Allow selection of modal dialogs on X11 in Auto-Type [keepassxreboot#6204] - KeeShare: prevent crash when file extension is missing [keepassxreboot#6174] Release 2.6.6 Fixed - Fix focusing search when pressing hotkey [keepassxreboot#6603] - Trim whitespace from TOTP key input prior to processing [keepassxreboot#6604] - Fix building on macOS [keepassxreboot#6598] - Resolve compiler warnings for unused return values [keepassxreboot#6607]

droidmonkey approved these changes May 4, 2021

View reviewed changes

droidmonkey added user interface bug labels May 4, 2021

droidmonkey added this to the v2.6.5 milestone May 4, 2021

michaelk83 reviewed May 4, 2021

View reviewed changes

michaelk83 reviewed May 7, 2021

View reviewed changes

src/gui/EntryPreviewWidget.cpp Outdated Show resolved Hide resolved

xvallspl force-pushed the fix/showing-previous-entry-notes-when-no-notes branch from 9e1c64a to f74793d Compare May 7, 2021 17:11

Fix showing preview notes in an entry without notes

b4239b3

Fixes #6461

droidmonkey force-pushed the fix/showing-previous-entry-notes-when-no-notes branch from f74793d to b4239b3 Compare May 8, 2021 16:26

droidmonkey approved these changes May 8, 2021

View reviewed changes

droidmonkey merged commit 64279bb into develop May 8, 2021

droidmonkey deleted the fix/showing-previous-entry-notes-when-no-notes branch May 8, 2021 21:35

droidmonkey mentioned this pull request May 15, 2021

Backports from develop to 2.6.5 release #6525

Merged

phoerious added pr: bugfix Pull request fixes a bug and removed bug labels Nov 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix showing any notes in an entry without notes (Fixes #6461)#6481

fix showing any notes in an entry without notes (Fixes #6461)#6481
droidmonkey merged 1 commit intodevelopfrom
fix/showing-previous-entry-notes-when-no-notes

xvallspl commented May 4, 2021 •

edited

Loading

Uh oh!

michaelk83 May 4, 2021 •

edited

Loading

Uh oh!

droidmonkey May 4, 2021 •

edited

Loading

Uh oh!

michaelk83 May 4, 2021

Uh oh!

droidmonkey May 4, 2021

Uh oh!

michaelk83 May 7, 2021

Uh oh!

xvallspl May 7, 2021

Uh oh!

Uh oh!

michaelk83 commented May 7, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

xvallspl commented May 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Screenshots

Testing strategy

Type of change

Uh oh!

michaelk83 May 4, 2021 • 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!

droidmonkey May 4, 2021 • 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!

michaelk83 May 4, 2021

Choose a reason for hiding this comment

Uh oh!

droidmonkey May 4, 2021

Choose a reason for hiding this comment

Uh oh!

michaelk83 May 7, 2021

Choose a reason for hiding this comment

Uh oh!

xvallspl May 7, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

michaelk83 commented May 7, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

xvallspl commented May 4, 2021 •

edited

Loading

michaelk83 May 4, 2021 •

edited

Loading

droidmonkey May 4, 2021 •

edited

Loading