Grey out Apply button when there are no changes by dwilches · Pull Request #1702 · keepassxreboot/keepassxc

dwilches · 2018-03-11T04:03:19Z

When the user clicks on the Apply button, it will get disabled. When changes are made, it's enabled again.

Description

Whenever the Apply button is pressed, and if the save was successful, then the Apply button is disabled.
Each subwidget used by EditEntryWidget has now a signal called widgetUpdated that is emitted when the widgets' internal content changes. The EditEntryWidget subscribes to that signal to know when to enable the Apply button (by calling entryUpdated()).
There are some views that are not isolated in their own widgets (m_advancedUi, for example) so in those cases I invoked entryUpdated() directly whenever I detected an update:
- some updates occur directly in a Qt widget like when editing the text of a QLineItem, so in that case I connected the widget's signals directly to the entryUpdated() slot.
- some updates occur in EditEntryWidget, so in those cases the invocation to entryUpdated() is made as soon as the change is detected (for example when the user has confirmed an action in a dialog).

Motivation and context

Resolves #1313
Resolves #1181

How has this been tested?

I tested it manually, not sure if I need to write an automated test.

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]

droidmonkey · 2018-03-11T04:55:45Z

The base functionality of #1181 (warn when unsaved changes exist) should be ported into this PR. I like this implementation better for detecting when something is changed.

phoerious · 2018-03-11T09:00:18Z

You can block signals for trivial updates. The button shouldn't be enabled for non-modifying changes.

dwilches · 2018-03-11T22:56:43Z

@droidmonkey What do you mean by "porting functionality"? Should I create a merge commit between my branch and frostasm's branch request-confirmation-to-discard-unsaved-changes and then make my PR include that merge commit?
Or should I copy&paste code from frostasm's branch into a new commmit in my PR?

dwilches · 2018-03-11T23:06:39Z

@phoerious Is it ok to use classes from Qt 5.3? Asking because of QSignalBlocker.
Otherwise I would use QObject::blockSignals which is available also in Qt 5.2

droidmonkey · 2018-03-12T00:19:14Z

You could copy paste the relevant dialog code or implement your own variant. Definitely don't merge the branches.

dwilches · 2018-03-17T22:22:47Z

@droidmonkey @phoerious Changes implemented. This PR is now composed of 3 commits:

Solves Grey out Apply button once clicked by user #1313 but missing signal blocking
Implement signal blocking
Shows confirmation dialog before closing unsaved entries (Unsaved changes in the entry view silently get discarded if escape is pressed #1181)

Let me know what you think!

dwilches · 2018-03-17T22:29:02Z

BTW: Amended the commits to change to my personal email.

phoerious

A few style nitpicks, rest looks good to me.

phoerious · 2018-03-17T22:43:27Z

src/gui/EditWidgetIcons.cpp

+    connect(m_ui->defaultIconsRadio, SIGNAL(toggled(bool)), this, SIGNAL(widgetUpdated()));
+    connect(m_ui->defaultIconsRadio, SIGNAL(toggled(bool)), this, SIGNAL(widgetUpdated()));
+    connect(m_ui->defaultIconsView->selectionModel(),
+            SIGNAL(selectionChanged(const QItemSelection &, const QItemSelection &)),


No space before (please change that everywhere)

You mean the newline? So should it be like:

connect(m_ui->defaultIconsView->selectionModel(), SIGNAL(selectionChanged(const QItemSelection &, const QItemSelection &)), this, SIGNAL(widgetUpdated()));

or like this:

connect(m_ui->defaultIconsView->selectionModel(), SIGNAL(selectionChanged(const QItemSelection &, const QItemSelection &)), this, SIGNAL(widgetUpdated()));

phoerious · 2018-03-17T22:45:20Z

src/gui/entry/EditEntryWidget.cpp

+#ifdef WITH_XC_SSHAGENT
+    // SSH Agent tab
+    if (config()->get("SSHAgent", false).toBool())
+    {


No space before {

phoerious · 2018-03-17T22:45:43Z

src/gui/entry/EditEntryWidget.cpp

    }

+    if (!m_saved)
+    {


phoerious · 2018-03-17T22:46:29Z

src/gui/entry/EditEntryWidget.cpp

    }
 }
+
+void EditEntryWidget::setUnsavedChanges(bool anyUnsaved)


I would call the parameter hasUnsaved

phoerious · 2018-03-17T22:49:48Z

I think ultimately we should change the button state based on the actual entry data, not based on widget edits. But for now it's okay IMHO.

phoerious · 2018-03-17T23:06:27Z

Hmm. The changes seem to hang the GUI tests. Please check what's wrong there. I would assume it's not finishing, because it can't click the button or so.

phoerious · 2018-03-17T23:19:04Z

I restarted the builds. Second rounds seems to go through, but it's quite slow. One build with tests takes 5-6 minutes now instead of 2.

dwilches · 2018-03-18T00:49:27Z

@phoerious Pushed the format and space changes (and added a comment as I'm not sure I got one of your comments right). About the tests, I could run make test with no issue. The compilation is slow though when using -DWITH_ASAN, but could the changes on this PR be affecting something related to test/compilation speed?

dwilches · 2018-03-18T01:41:03Z

Just pushed another commit for a problem I noticed a moment ago. Do you use squash&merge or you want me to squash my commits in my branch before merging the PR?

droidmonkey · 2018-03-18T02:06:07Z

I would do a merge commit for this since there are multiple fixes.

dwilches · 2018-03-18T02:12:37Z

Ok, if it's ok for you, I'd like to squash some of the commits in my branch to clean a bit the story of the repository when the PR gets merged, instead of 5 commits I can leave 2 commits, one per fix.

droidmonkey · 2018-03-18T02:18:32Z

Yah you can squash the intermediate ones

dwilches · 2018-03-18T02:34:55Z

Done 👍

phoerious · 2018-03-18T09:42:32Z

I meant no space before &. Github swallowed my unescaped ampersand.

Compile and run tests with -DWITH_GUI_TESTS=ON. They don't finish on TC.

dwilches · 2018-03-18T20:29:22Z

I found two problems:

Some times the "Entry has been modified" dialog popups up and hangs the test because it keeps waiting for input. I'll solve that one in a moment. It's strange it only fails some times, will look into that.
I found another test that fails every time, but it also fails with a fresh clone in the master branch:

FAIL!  : TestGui::testDeleteEntry() Compared values are not the same
   Actual   (entryView->selectionModel()->selectedRows().size()): 1
   Expected (2)                                                 : 2
   Loc: [/home/dwilches/Repos/tmp/keepassxc/tests/gui/TestGui.cpp(807)]

Not sure if it fails for you too or it's because I'm in Ubuntu+KDE Plasma (the test fails when clicking an entry, and then pressing Ctrl + clicking another entry, then expects 2 entries to be selected, but finds only 1 is selected).

droidmonkey · 2018-03-18T20:54:41Z

master tests passed: https://ci.keepassxc.org/viewLog.html?buildId=9321&buildTypeId=KeepassXC_TeamCityCi

dwilches · 2018-03-18T21:28:57Z

Fixed test that had broken by the new functionality. And fixed the extra spaces before the & too.

About the other test that fails, should be something in my machine then. If I follow by myself the same steps that the test case does, they work as expected.

phoerious · 2018-03-18T21:39:45Z

I stopped the old tests. They were jamming the queue.
A blocking dialog was pretty much what I suspected.

dwilches · 2018-03-29T03:50:32Z

Noticed this PR status says "Changes requested", but there is no pending change, so letting you know in case there is still something needed.

phoerious · 2018-03-29T06:59:50Z

No, you just need to rebase to the current branch head.

Resolves keepassxreboot#1313 What this commit does: * Whenever the Apply button is pressed, and if the save was successful, then the Apply button is disabled. * Each subwidget used by EditEntryWidget has now a signal called `widgetUpdated` that is emitted when the widgets' internal content changes. The EditEntryWidget subscribes to that signal to know when to enable the Apply button (by calling `entryUpdated()`). * There are some views that are not isolated in their own widgets (`m_advancedUi`, for example) so in those cases I invoked `entryUpdated()` directly whenever I detected an update: * some updates occur directly in a Qt widget like when editing the text of a QLineItem, so in that case I connected the widget's signals directly to the `entryUpdated()` slot. * some updates occur in EditEntryWidget, so in those cases the invocation to `entryUpdated()` is made as soon as the change is detected (for example when the user has confirmed an action in a dialog). A known problem: there are some situations when the Apply button will get enabled even if there are no changes, this is because the app changes the value of a field by itself so it's considered an update (for example, clicking on the "Reveal" button changes the text shown in a text field). The solution to this can be a bit complicated: disabling temporarily the `entryUpdated()` whenever the app is going to do an action with such side-effects. So I preferred to let the Apply button get enabled in those cases.

Solves keepassxreboot#1181

dwilches · 2018-03-30T01:38:51Z

@phoerious Done, rebased on top of the current develop branch.

phoerious · 2018-03-31T11:54:36Z

Thanks!

haridsv · 2019-03-30T11:41:17Z

Just checking... this change is not yet in the latest release, right?

droidmonkey · 2019-03-30T12:15:13Z

It is, but I think its broken because as soon as you open an entry it determines there are unsaved changes.

haridsv · 2019-04-01T14:59:07Z

Thanks @droidmonkey! I didn't realize there is a newer version until I saw your response. I got the new version via Brew and now I can see that the Apply button is working as expected. I don't see the erroneous behavior that you are describing. Perhaps it is a corner case bug? Is it happening for you for all entries?

droidmonkey · 2019-04-01T15:55:58Z

The case I am describing is if you try to lock the database with the entry edit view open, it asks if you want to save the entry before closing instead of properly determining if it is "dirty" or not. I thought that applied to the apply button as well, but it does not.

droidmonkey added this to the v2.4.0 milestone Mar 11, 2018

droidmonkey added the user interface label Mar 11, 2018

dwilches force-pushed the hotfix/1313-grey-out-apply-button branch 3 times, most recently from 64f78ef to 7e927e0 Compare March 17, 2018 22:18

dwilches force-pushed the hotfix/1313-grey-out-apply-button branch from 7e927e0 to 8c3c9aa Compare March 17, 2018 22:27

phoerious requested changes Mar 17, 2018

View reviewed changes

dwilches force-pushed the hotfix/1313-grey-out-apply-button branch from eeeb60c to dbfc6eb Compare March 18, 2018 02:30

dwilches force-pushed the hotfix/1313-grey-out-apply-button branch 2 times, most recently from 938c5b6 to 301649f Compare March 18, 2018 21:26

dwilches added 2 commits March 29, 2018 18:35

Request confirmation to discard unsaved changes

240939c

Solves keepassxreboot#1181

dwilches force-pushed the hotfix/1313-grey-out-apply-button branch from 301649f to 240939c Compare March 30, 2018 01:38

phoerious approved these changes Mar 31, 2018

View reviewed changes

phoerious merged commit bd654ea into keepassxreboot:develop Mar 31, 2018

TheZ3ro mentioned this pull request Mar 31, 2018

Code formatting #1191

Merged

droidmonkey mentioned this pull request Apr 3, 2018

Request confirmation to discard unsaved changes ( #1181) #1234

Closed

Uh oh!

Conversation

dwilches commented Mar 11, 2018 • 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?

Types of changes

Checklist:

Uh oh!

droidmonkey commented Mar 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phoerious commented Mar 11, 2018

Uh oh!

dwilches commented Mar 11, 2018

Uh oh!

dwilches commented Mar 11, 2018

Uh oh!

droidmonkey commented Mar 12, 2018

Uh oh!

dwilches commented Mar 17, 2018

Uh oh!

dwilches commented Mar 17, 2018

Uh oh!

phoerious left a comment

Choose a reason for hiding this comment

Uh oh!

phoerious Mar 17, 2018

Choose a reason for hiding this comment

Uh oh!

dwilches Mar 18, 2018

Choose a reason for hiding this comment

Uh oh!

phoerious Mar 17, 2018

Choose a reason for hiding this comment

Uh oh!

phoerious Mar 17, 2018

Choose a reason for hiding this comment

Uh oh!

phoerious Mar 17, 2018

Choose a reason for hiding this comment

Uh oh!

phoerious commented Mar 17, 2018

Uh oh!

phoerious commented Mar 17, 2018

Uh oh!

phoerious commented Mar 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dwilches commented Mar 18, 2018

Uh oh!

dwilches commented Mar 18, 2018

Uh oh!

droidmonkey commented Mar 18, 2018

Uh oh!

dwilches commented Mar 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

droidmonkey commented Mar 18, 2018

Uh oh!

dwilches commented Mar 18, 2018

Uh oh!

phoerious commented Mar 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dwilches commented Mar 18, 2018

Uh oh!

droidmonkey commented Mar 18, 2018

Uh oh!

dwilches commented Mar 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phoerious commented Mar 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dwilches commented Mar 29, 2018

Uh oh!

phoerious commented Mar 29, 2018

Uh oh!

dwilches commented Mar 11, 2018 •

edited

Loading

droidmonkey commented Mar 11, 2018 •

edited

Loading

phoerious commented Mar 17, 2018 •

edited

Loading

dwilches commented Mar 18, 2018 •

edited

Loading

phoerious commented Mar 18, 2018 •

edited

Loading

dwilches commented Mar 18, 2018 •

edited

Loading

phoerious commented Mar 18, 2018 •

edited

Loading

droidmonkey commented Apr 1, 2019 •

edited

Loading