KeePassX PR Migration: #78 Inline Messages #162

phoerious · 2017-01-15T00:55:00Z

Description

This pull request is a rebased and updated version of the original KeePassX pull request keepassx/keepassx#78 by @pgalves.

It uses the stripped down KMessageWidget from the Subsurface project to show error messages instead of modal dialogs.

Motivation and Context

Inline messages are less obtrusive than modal dialogs.

How Has This Been Tested?

I tried to create situations in which the modified error messages are shown. Not all situations could be tested, but the most common error messages appear to work as expected.
No existing unit tests fail.

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]

droidmonkey · 2017-01-15T03:37:50Z

wow this looks amazing!

louib · 2017-01-29T01:44:06Z

src/CMakeLists.txt

@@ -86,9 +86,11 @@ set(keepassx_SOURCES
    gui/FileDialog.cpp
    gui/IconModels.cpp
    gui/KeePass1OpenWidget.cpp
+    gui/kmessagewidget.cpp


Shouldn't the filename also be PascalCase, even though this is the original filename?

I think it should, but I left it as is, because that was the original file name. I guess I'll change it.

louib · 2017-01-29T02:04:06Z

src/gui/MainWindow.cpp

+
+void MainWindow::displayTabMessage(const QString& text, MessageWidget::MessageType type)
+{
+    //if (m_ui->stackedWidget->currentIndex() == 0) {


This condition could be removed if no longer used.

louib · 2017-01-29T02:12:21Z

src/gui/DatabaseWidget.cpp

+
+void DatabaseWidget::hideMessage()
+{
+    if (m_messageWidget->isVisible()) {


Why not add the isVisible() condition in MessageWidget::hide and call m_messageWidget->hide() directly? This code is also repeated in EditWidget.cpp

louib · 2017-01-29T02:14:08Z

src/gui/MainWindow.cpp

@@ -104,6 +104,7 @@ MainWindow::MainWindow()
    #endif

    setWindowIcon(filePath()->applicationIcon());
+    m_ui->globalMessageWidget->setHidden(true);


Looks like most of the time the MessageWidget instances are hidden when initialized. The setHidden(true) could be moved in the MessageWidget's constructor (since there is no .ui file for the widget).

louib · 2017-01-29T02:38:30Z

Tested this branch locally (global message, tab message and entry message). Works great! Good job @pgalves!

I think there's a MessageBox::warning left in src/gui/EditWidgetIcons.cpp, line 168, which could use the new MessageWidget as well. There's a couple of MessageBox::warning in the database repair widget, but it would be hard to use the new message widget there since the repair database widget is opened out of the main window.

Don't know how much work that would imply, but it could be cool to hide the message after a certain amount of time.

phoerious · 2017-01-29T12:02:21Z

Thanks for the review @louib! Before we merge this, I will also have a look if all former MessageBoxes are still covered in develop.

(https://github.com/torvalds/subsurface, commit: 82a946152b7f1da177344937acbc9d3cb5b0ccbf). Added MessageWidget class.

DatabaseOpenWidget.

inline MessageWidget in ChangeMasterKeyWidget.

EditWidget and in UnlockDatabaseWidget. Add missing method to show Information Message.

Chnage to one method to set MessageWidget text passing type as parameter. Only messages with questions requiring user input reamin using MessageBox dialog. Use signal/slots to set message in MessageWidget and hide message, signal/slots only used when required.Maybe need to change all calls to signals/slots in the future.

phoerious · 2017-02-10T01:28:44Z

@louib I fixed a few things, but I think I'll leave the rest as is. I don't want to touch the widget's source code much, since those changes would be gone when we update it in the future.

louib · 2017-02-10T01:33:42Z

@phoerious do you want to address the remaining MessageBox in src/gui/EditWidgetIcons.cpp?

phoerious · 2017-02-10T01:39:47Z

I was thinking about it. But a) I would need to get a reference to the MainWindow somehow and b) nobody will ever see that error message except for when either you have no Internet connection or Google ist down. I figured it wasn't really worth the effort right now. Besides, it would shift the whole UI down which can be unpleasant.

louib · 2017-02-10T01:49:06Z

COPYING

@@ -198,3 +198,9 @@ Files: src/zxcvbn/zxcvbn.*
 Copyright: 2015, Tony Evans
           2016, KeePassXC Team
 License: BSD 3-clause
+
+Files: src/gui/kmessagewidget.h
+       src/gui/kmessagewidget.cpp


@phoerious we might want to update those also.

Things your refactoring tools don't find. Good catch!

- Added YubiKey 2FA integration for unlocking databases [#127] - Added TOTP support [#519] - Added CSV import tool [#146, #490] - Added KeePassXC CLI tool [#254] - Added diceware password generator [#373] - Added support for entry references [#370, #378] - Added support for Twofish encryption [#167] - Enabled DEP and ASLR for in-memory protection [#371] - Enabled single instance mode [#510] - Enabled portable mode [#645] - Enabled database lock on screensaver and session lock [#545] - Redesigned welcome screen with common features and recent databases [#292] - Multiple updates to search behavior [#168, #213, #374, #471, #603, #654] - Added auto-type fields {CLEARFIELD}, {SPACE}, {{}, {}} [#267, #427, #480] - Fixed auto-type errors on Linux [#550] - Prompt user prior to executing a cmd:// URL [#235] - Entry attributes can be protected (hidden) [#220] - Added extended ascii to password generator [#538] - Added new database icon to toolbar [#289] - Added context menu entry to empty recycle bin in databases [#520] - Added "apply" button to entry and group edit windows [#624] - Added macOS tray icon and enabled minimize on close [#583] - Fixed issues with unclean shutdowns [#170, #580] - Changed keyboard shortcut to create new database to CTRL+SHIFT+N [#515] - Compare window title to entry URLs [#556] - Implemented inline error messages [#162] - Ignore group expansion and other minor changes when making database "dirty" [#464] - Updated license and copyright information on souce files [#632] - Added contributors list to about dialog [#629]

phoerious added this to the v2.2.0 milestone Jan 15, 2017

phoerious added new feature user interface labels Jan 15, 2017

phoerious force-pushed the migrate/78-inline-messages branch from e56727e to d5e2aa5 Compare January 15, 2017 01:05

phoerious changed the title ~~Migrate KeePassX PR: #78 Inline Messages~~ KeePassX PR Migration: #78 Inline Messages Jan 15, 2017

phoerious force-pushed the migrate/78-inline-messages branch from d5e2aa5 to bbddd3f Compare January 15, 2017 04:10

phoerious force-pushed the migrate/78-inline-messages branch 4 times, most recently from cbd172c to 15e25a9 Compare January 28, 2017 16:56

louib reviewed Jan 29, 2017

View reviewed changes

phoerious force-pushed the migrate/78-inline-messages branch 2 times, most recently from b988968 to 8ffb86c Compare February 10, 2017 01:22

pgalves and others added 10 commits February 10, 2017 02:25

Added kmessagewidget from subsurface project

c6ad476

(https://github.com/torvalds/subsurface, commit: 82a946152b7f1da177344937acbc9d3cb5b0ccbf). Added MessageWidget class.

Replaced MessageBox dialogs with inline MessageWidget in

a710181

DatabaseOpenWidget.

Replace MessageBox dialogs that don't require user interaction with

3c4af1a

inline MessageWidget in ChangeMasterKeyWidget.

Show inline message when unable to load the Key File.

8fa070f

Replace MessageBox Dialog with inline MessageWidget in

c2826bb

EditWidget and in UnlockDatabaseWidget. Add missing method to show Information Message.

Added licence header.

6e7f826

Add license notice for KMessageWidget

84222e8

Update KMessageWidget

b6ea06b

Use KMessageWidget also for new error messages

41d5111

phoerious force-pushed the migrate/78-inline-messages branch from 8ffb86c to 83b9cc4 Compare February 10, 2017 01:26

Rename KMessageWidget files to match our coding style

94f8650

phoerious force-pushed the migrate/78-inline-messages branch from 83b9cc4 to 94f8650 Compare February 10, 2017 01:34

louib reviewed Feb 10, 2017

View reviewed changes

Update KMessageWidget files in COPYING also [skip ci]

bf419d1

louib approved these changes Feb 10, 2017

View reviewed changes

droidmonkey merged commit 586e6f1 into develop Feb 10, 2017

droidmonkey deleted the migrate/78-inline-messages branch February 10, 2017 01:56

phoerious added pr: new feature Pull request that adds a new feature and removed new feature labels Nov 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KeePassX PR Migration: #78 Inline Messages #162

KeePassX PR Migration: #78 Inline Messages #162

phoerious commented Jan 15, 2017 •

edited

Loading

droidmonkey commented Jan 15, 2017

louib Jan 29, 2017

phoerious Jan 29, 2017

louib Jan 29, 2017

louib Jan 29, 2017

louib Jan 29, 2017

louib commented Jan 29, 2017

phoerious commented Jan 29, 2017

phoerious commented Feb 10, 2017

louib commented Feb 10, 2017

phoerious commented Feb 10, 2017

louib Feb 10, 2017

phoerious Feb 10, 2017 •

edited

Loading

KeePassX PR Migration: #78 Inline Messages #162

KeePassX PR Migration: #78 Inline Messages #162

Conversation

phoerious commented Jan 15, 2017 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

droidmonkey commented Jan 15, 2017

louib Jan 29, 2017

Choose a reason for hiding this comment

phoerious Jan 29, 2017

Choose a reason for hiding this comment

louib Jan 29, 2017

Choose a reason for hiding this comment

louib Jan 29, 2017

Choose a reason for hiding this comment

louib Jan 29, 2017

Choose a reason for hiding this comment

louib commented Jan 29, 2017

phoerious commented Jan 29, 2017

phoerious commented Feb 10, 2017

louib commented Feb 10, 2017

phoerious commented Feb 10, 2017

louib Feb 10, 2017

Choose a reason for hiding this comment

phoerious Feb 10, 2017 • edited Loading

Choose a reason for hiding this comment

phoerious commented Jan 15, 2017 •

edited

Loading

phoerious Feb 10, 2017 •

edited

Loading