D29017: WIP: Introduce three new methods that return all "entries" in a folder

[Ahmad Samir]

ahmadsamir abandoned this revision.

REPOSITORY
  R311 KWallet

REVISION DETAIL
  https://phabricator.kde.org/D29017

To: ahmadsamir, #frameworks, dfaure
Cc: blaze, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29017: WIP: Introduce three new methods that return all "entries" in a folder

[Ahmad Samir]

ahmadsamir reopened this revision.
ahmadsamir marked an inline comment as done.
ahmadsamir added a comment.


  Move to gitlab https://invent.kde.org/frameworks/kwallet/-/merge_requests/2

REPOSITORY
  R311 KWallet

REVISION DETAIL
  https://phabricator.kde.org/D29017

To: ahmadsamir, #frameworks, dfaure
Cc: blaze, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29017: WIP: Introduce three new methods that return all "entries" in a folder

[Ahmad Samir]

This revision was not accepted when it landed; it landed in state "Needs 
Revision".
This revision was automatically updated to reflect the committed changes.
Closed by commit R311:1a19afb6a06d: WIP: Introduce three new methods that 
return all entries in a folder (authored by ahmadsamir).

REPOSITORY
  R311 KWallet

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D29017?vs=80670=83224

REVISION DETAIL
  https://phabricator.kde.org/D29017

AFFECTED FILES
  CMakeLists.txt
  src/api/KWallet/CMakeLists.txt
  src/api/KWallet/kwallet.cpp
  src/api/KWallet/kwallet.h
  src/api/KWallet/org.kde.KWallet.xml
  src/runtime/kwalletd/backend/kwalletbackend.cc
  src/runtime/kwalletd/backend/kwalletbackend.h
  src/runtime/kwalletd/kwalletd.cpp
  src/runtime/kwalletd/kwalletd.h

To: ahmadsamir, #frameworks, dfaure
Cc: blaze, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29017: WIP: Introduce three new methods that return all "entries" in a folder

[Ahmad Samir]

ahmadsamir added a comment.


  In D29017#664279 , @blaze wrote:
  
  > Check this commit message 
https://phabricator.kde.org/R32:f56cdda54b7f88b119f2c7cfb2f534b4fe74478f
  
  
  Sorry for the delay. We applied a workable hack in 
https://phabricator.kde.org/D28880 to get rid of the '[^/]' in the string that 
wildcardToRegularExpression returns, and it seemed to work when I tested falkon.
  
  I guess I'll have migrate this to invent.kde and try addressing the review 
comments.

REPOSITORY
  R311 KWallet

REVISION DETAIL
  https://phabricator.kde.org/D29017

To: ahmadsamir, #frameworks, dfaure
Cc: blaze, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29017: WIP: Introduce three new methods that return all "entries" in a folder

[Mikołaj Płomieński]

blaze added a comment.


  Check this commit message 
https://phabricator.kde.org/R32:f56cdda54b7f88b119f2c7cfb2f534b4fe74478f

REPOSITORY
  R311 KWallet

REVISION DETAIL
  https://phabricator.kde.org/D29017

To: ahmadsamir, #frameworks, dfaure
Cc: blaze, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29017: WIP: Introduce three new methods that return all "entries" in a folder

[David Faure]

dfaure requested changes to this revision.
dfaure added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> kwallet.cpp:219
> +KSecretsService::ReadItemPropertyJob *readLabelJob = 
> item->label();
> +if (readLabelJob->exec()) {
> +const QString label = 
> readLabelJob->propertyValue().toString();

Urgh. This actually calls for async API instead.

But now I'm really confused. HAVE_KSECRETSSERVICE is never set anywhere (except 
to 0), how does one even compile this code?

> kwallet.h:382
> + * is the entry key.
> + *  @return Returns 0 on success, non-zero on error.
> + *

Are there documented error codes somewhere? Otherwise a bool would do be more 
readable, no?

But then if we don't have error codes, wouldn't

  QMap entriesList()

be better?

> kwalletbackend.cc:558
> +const EntryMap  = _entries[_folder];
> +rc.append(map.values());
> +

`rc = map.values()`, append() makes little sense if it's empty.

> kwalletbackend.h:140
> +// Get a list of all the entries in the current folder.
> +// You delete the list.
> +// @since 5.70

This makes no sense. The list is a value. Does this mean "The caller takes 
ownership of the entries"?

REPOSITORY
  R311 KWallet

REVISION DETAIL
  https://phabricator.kde.org/D29017

To: ahmadsamir, #frameworks, dfaure
Cc: blaze, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29017: WIP: Introduce three new methods that return all "entries" in a folder

[Ahmad Samir]

ahmadsamir added a comment.


  Todo: port kwalletmanager locally and see if it still works (which isn't 
saying much as I don't use kwallet that much). I think we can delay this to KF 
5.71, to get more testing.

REPOSITORY
  R311 KWallet

REVISION DETAIL
  https://phabricator.kde.org/D29017

To: ahmadsamir, #frameworks, dfaure
Cc: blaze, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns

D29017: WIP: Introduce three new methods that return all "entries" in a folder

[Ahmad Samir]

ahmadsamir created this revision.
ahmadsamir added reviewers: Frameworks, dfaure.
Herald added a project: Frameworks.
ahmadsamir requested review of this revision.

REVISION SUMMARY
  As was discussed in https://phabricator.kde.org/D28880 with dfaure, it
  seems that all users (only two, KWalletManager and Falkon) of the current
  read{Entry,Map,Passowrd}list() methods basically use "*" as a wildcard
  to get a list of all the "entries" in a folder. Therefore it makes sense
  to introduce new methods that do just that, this gets rid of regular
  expression usage for matching a certain pattern. This fits with how .e.g.
  KWalletManager is using readEntryList(), to get a list of all the entries
  to fill a list view with them, then the "matching" is done with a QSFPM-like
  class.

TEST PLAN
  - make && ctest
  - Ported Falkon locally, and it seems to work

REPOSITORY
  R311 KWallet

BRANCH
  l-kwallet-dump-wildcard (branched from master)

REVISION DETAIL
  https://phabricator.kde.org/D29017

AFFECTED FILES
  CMakeLists.txt
  src/api/KWallet/CMakeLists.txt
  src/api/KWallet/kwallet.cpp
  src/api/KWallet/kwallet.h
  src/api/KWallet/org.kde.KWallet.xml
  src/runtime/kwalletd/backend/kwalletbackend.cc
  src/runtime/kwalletd/backend/kwalletbackend.h
  src/runtime/kwalletd/kwalletd.cpp
  src/runtime/kwalletd/kwalletd.h

To: ahmadsamir, #frameworks, dfaure
Cc: blaze, kde-frameworks-devel, LeGast00n, cblack, michaelh, ngraham, bruns