Cumulative updates to secret service integration

[Aetf]

Type of change

• ✅ Bug fix (non-breaking change which fixes an issue)
• ✅ Refactor (significant modification to existing code)

Description and Context

This PR contains multiple fixes and refactor. I just think they're not worth separate PRs. But @droidmonkey, if you prefer, I can also do that.

• UI improvements (mostly refactor)
  • Use proper models for database and session in the settings page
  • Fix button text (unlock/lock) not changed according to the database locking status
  • Fix button icons not present on icon themes other than Breeze
  • Fix the disconnect button may get clipped when a new session opens
• Remove half created entries if the creation fails.
• Cleanup all connections when the database is replaced due to locking, fix KeepassXC not providing Freedesktop.Secrets after lock/unlock of database. #4004
• Fix when exposing groups, the root group is considered in recycle bin
• Fix service not registered if starting with the plugin enabled

Update:

(Checking PID for existing services is saved for targeting 2.6.0 due to translation freeze).

Testing strategy

Test manually.

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]

[Aetf]

• Address comments
• Remove the checking PID for existing services from this PR. I'll have a separate PR for that targeting develop.

[Aetf]

Added two more small bug fixes.

[Aetf]

Rebased on latest release/2.5.2

[droidmonkey]

I do not like this because it is the exact opposite of how signal/slots should be used. The Settings Model should be created, then the signals connected from the parent. The settings model should have no knowledge of the db tab widget.

I keep it as-is for now, but this would be a good upgrade for 2.6.0 to break some hard dependencies.

[Aetf]

I'll keep that in mind. In general, I'm also thinking about removing dependencies on db tab widget.

(Or even better, don't depend on any GUI classes at all. But that's quite difficult for now, given the db widget is the only one keeping the filename and lock state).

[droidmonkey]

The filename is located in the Database class. You can determine if its locked by checking to see if it has a key associated with it. We should make a convenience function that returns that state...

[Aetf]

Thanks for reviewing!

[Aetf]

Ah yes. But a locked db is basically a new object. The db widget is still the one emiting db replaced signal that I need to catch to reconnect signals on the db. And btw since we are on the topic, the db tab widget is the only source of truth about currently opened dbs, is that correct?

…

On Fri, Dec 27, 2019, 21:04 Jonathan White ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/fdosecrets/widgets/SettingsModels.cpp <#4009 (comment)> : > + } + + void SettingsDatabaseModel::populateModel() + { + beginResetModel(); + + m_dbs.clear(); + + if (m_dbTabs) { + // Add existing database tabs + for (int idx = 0; idx != m_dbTabs->count(); ++idx) { + auto dbWidget = m_dbTabs->databaseWidgetFromIndex(idx); + databaseAdded(dbWidget, false); + } + // connect signals + connect(m_dbTabs, &DatabaseTabWidget::databaseOpened, this, [this](DatabaseWidget* db) { The filename is located in the Database class. You can determine if its locked by checking to see if it has a key associated with it. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#4009?email_source=notifications&email_token=AALTBD4SKY3PNB4XCU7KZE3Q22X25A5CNFSM4JZWGXRKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCQKKGHY#discussion_r361770455>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AALTBDY3FBETU67XJHMUVCLQ22X25ANCNFSM4JZWGXRA> .

[droidmonkey]

Yes that is correct