Bug 376739 - KCM shows the Apply Settings dialog even if nothing changed
Summary: KCM shows the Apply Settings dialog even if nothing changed
Status: RESOLVED FIXED
Alias: None
Product: plasma-nm
Classification: Plasma
Component: kcm (show other bugs)
Version: 5.9.2
Platform: Other Linux
: NOR normal
Target Milestone: ---
Assignee: Jan Grulich
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-02-20 22:26 UTC by Elvis Angelaccio
Modified: 2017-02-22 11:08 UTC (History)
1 user (show)

See Also:
Latest Commit:
Version Fixed In: 5.9.3


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Elvis Angelaccio 2017-02-20 22:26:12 UTC
To reproduce:

- Open the new Connection KCM
- Go back (click the All Settings button in System Settings)

The "Apply Settings" confirmation dialog appears, even though you have not changed anything in the KCM.
Comment 1 Elvis Angelaccio 2017-02-20 22:27:47 UTC
(In reply to Elvis Angelaccio from comment #0)
> To reproduce:
> 
> - Open the new Connection KCM
> - Go back (click the All Settings button in System Settings)

or just close System Settings...
Comment 2 Jan Grulich 2017-02-21 06:02:30 UTC
I cannot reproduce that, are you sure you didn't change anything?
Comment 3 Elvis Angelaccio 2017-02-21 09:31:54 UTC
Yes. I just noticed that 'kcmshell5 kcm_networkmanagement' is not affected. I'll try to investigate what's going on...
Comment 4 Elvis Angelaccio 2017-02-21 10:28:40 UTC
Ok I this is what is going on here.

Debug output when running kcmshell5 kcm_networkmanagement:

Going to emit validityChanged( false ) from: void ConnectionEditorBase::initialize()
Going to emit validityChanged(false) from: void ConnectionEditorBase::initialize()
Going to emit KCModule::changed(false) from void KCMNetworkmanagement::loadConnectionSettings(const NetworkManager::ConnectionSettings::Ptr &)
Going to emit validityChanged( false ) from: void ConnectionEditorBase::initialize()
Going to emit validityChanged(false) from: void ConnectionEditorBase::initialize()
Going to emit KCModule::changed(false) from void KCMNetworkmanagement::loadConnectionSettings(const NetworkManager::ConnectionSettings::Ptr &)
Going to emit validityChanged(true) from: void ConnectionEditorBase::validChanged(bool)
Going to emit validityChanged(true) from: void ConnectionEditorBase::validChanged(bool)
Going to emit validityChanged(true) from: void ConnectionEditorBase::validChanged(bool)
Going to emit KCModule::changed( true ), &ConnectionEditorTabWidget::validityChanged was emitted.
Going to emit validityChanged( true ) from: void ConnectionEditorBase::initialize()

Debug output when running systemsettings5:

Going to emit validityChanged( false ) from: void ConnectionEditorBase::initialize()
Going to emit validityChanged(false) from: void ConnectionEditorBase::initialize()
Going to emit KCModule::changed(false) from void KCMNetworkmanagement::loadConnectionSettings(const NetworkManager::ConnectionSettings::Ptr &)
Going to emit validityChanged( false ) from: void ConnectionEditorBase::initialize()
Going to emit validityChanged(false) from: void ConnectionEditorBase::initialize()
Going to emit KCModule::changed(false) from void KCMNetworkmanagement::loadConnectionSettings(const NetworkManager::ConnectionSettings::Ptr &)
Going to emit validityChanged(true) from: void ConnectionEditorBase::validChanged(bool)
Going to emit validityChanged(true) from: void ConnectionEditorBase::validChanged(bool)
Going to emit validityChanged( true ) from: void ConnectionEditorBase::initialize()
Going to emit validityChanged(true) from: void ConnectionEditorBase::validChanged(bool)
Going to emit KCModule::changed( true ), &ConnectionEditorTabWidget::validityChanged was emitted.

Now, in both cases there is a KCModule::changed(true) emitted (which imho looks wrong). Question is, why kcmshell ignores it?
Comment 5 Jan Grulich 2017-02-21 10:40:40 UTC
With settingChanged() signal it's a bit tricky, because some connection types (e.g. wireless) asynchronously loads passwords and once those are received then we fill the UI with them which emits settingChanged() signal. For that reason I use isInitialized() to see whether secrets have been already fetched and I don't propose any change to KCM (enable apply button).
Comment 6 Elvis Angelaccio 2017-02-21 11:43:28 UTC
Ok I figured out why kcmshell is not affected, it just resets the Apply button to false later...
I'll try to come up with a patch, KCModule::changed(true) should not be emitted in the first place, imho.
Comment 7 Elvis Angelaccio 2017-02-22 11:08:15 UTC
Git commit 2c70ac2a6eddc9f87eac7e4e3a8c68f558d7dd0b by Elvis Angelaccio.
Committed on 21/02/2017 at 13:59.
Pushed by elvisangelaccio into branch 'Plasma/5.9'.

Don't assume initialized if there are pending dbus replies

Summary:
ConnectionEditorBase::initialize() does not count the number of pending dbus
replies, so it is currently assuming that if control reaches the end of the
function, we are not waiting for secrets and we can claim the kcm is
initialized. That is not necessarily true, as bug #376739 shows.

If we count the number of replies actually received, we can be sure that there
are no more pending replies and we can then set m_initialized to true at the
end of initialize().
FIXED-IN: 5.9.3

Test Plan:
Open Connection KCM from systemsettings5, no longer had the Apply button
enabled. Playing with the KCM still enables the Apply button if something is
changed.

Reviewers: jgrulich

Subscribers: plasma-devel

Tags: #plasma

Differential Revision: https://phabricator.kde.org/D4705

M  +6    -1    libs/editor/connectioneditorbase.cpp
M  +1    -0    libs/editor/connectioneditorbase.h

https://commits.kde.org/plasma-nm/2c70ac2a6eddc9f87eac7e4e3a8c68f558d7dd0b