DevHeads.net

Review Request 120202: [OS X] improvements to the kwallet/OSX keychain integration

Review request for KDE Software on Mac OS X and kdelibs.

Repository: kdelibs

Description
I'm still working on (the KDE4-based version of) my OS X keychain backend for kwallet. I'm at a point where I think I can present a work-in-progress in an RR because at least one feature has been improved enough to be of interest for everyone, and also because I could use feedback on how to proceed.
I'm currently focussing on 2 settings that are configured in the kwallet KCM (SystemSettings), and for which I'm working on an implementation not requiring kwalletd and/or DBus.

- idle time closing of wallets. This feature was not supported in the commited version presented in <a href="https://git.reviewboard.kde.org/r/119838/" title="https://git.reviewboard.kde.org/r/119838/">https://git.reviewboard.kde.org/r/119838/</a> The present patch adds an idleTimer and a shared lastAccessTime member. The idleTimer is reset each time a client performs one of a series of actions that I count as wallet accesses, and before resetting I update the idle timeout value from KConfig. When the timer fires, the elapsed time is compared to the shared last access time, and if it is >= the timeout, the wallet is closed. This applies only to "KDE keychains", so keychains used by OS X applications should not be affected.

- "close when last application exits". This requires maintaining a "user list" which keeps track of what application has what wallet open. I've implemented an "internal" version of such a registry, mapping wallet name to application names and the list of wallets they have open (a list of wallet reference, pid per application name). The registry is functional, but I have not yet decided (read: figured out) how to make a distributed representation of it.

So the work-in-progress concerns the distributed user registry. The idea would be to maintain the registry in shared memory, meaning it'd be reset (= disappear) when the last application exits, contrary to a file which can go stale. This would be simple if QSharedMemory objects could be resized, but apparently they cannot, so I'll have to look at other solutions possibly involving OS X frameworks (NSData and it's non-objectiveC version CFDataRef or CFMutableDataRef might be candidates). Suggestions welcome.

Other work in progress concerns a less wheel-reinventing approach that builds on kwalletd and DBus. I don't see why the code used in `kwallet.cpp` wouldn't work, but I must still misunderstand its finer details. The present patch contains outcommented code that does indeed cause kwalletd to be launched and slots and signals to become visible e.g. in `qdbusviewer`. But they don't work, which in turn makes the whole kwallet layer dysfunctional. Here too feedback is welcome on how what I'm missing and/or how to get this to work.
Once kwalletd works, wallet idle timeout closing and closing when the last client exits should work out-of-the-box, or at least I suppose.

Diffs
kdeui/util/kwallet.h d7f703f
kdeui/util/kwallet_mac.cpp 8344ebb
kdeui/util/qosxkeychain.h d0934e6
kdeui/util/qosxkeychain.cpp 7cb9a22

Diff: <a href="https://git.reviewboard.kde.org/r/120202/diff/" title="https://git.reviewboard.kde.org/r/120202/diff/">https://git.reviewboard.kde.org/r/120202/diff/</a>

Testing
OS X 10.6.8, kdelibs 4.14.1 git/master, KDE/MacPorts 4.12.5 .
Once finalised, all changes should port easily to KF5's kwallet_mac.cpp .

Thanks,

René J.V. Bertin

Comments

Re: Review Request 120202: [OS X] improvements to the kwallet/OS

By =?utf-8?Q?Ren=C... at 09/21/2014 - 12:40

(Updated Sept. 21, 2014, 6:40 p.m.)

Review request for KDE Software on Mac OS X and kdelibs.

Changes
Moved `slotIdleTimedOut()` into `WalletPrivate`.

Repository: kdelibs

Description
I'm still working on (the KDE4-based version of) my OS X keychain backend for kwallet. I'm at a point where I think I can present a work-in-progress in an RR because at least one feature has been improved enough to be of interest for everyone, and also because I could use feedback on how to proceed.
I'm currently focussing on 2 settings that are configured in the kwallet KCM (SystemSettings), and for which I'm working on an implementation not requiring kwalletd and/or DBus.

- idle time closing of wallets. This feature was not supported in the commited version presented in <a href="https://git.reviewboard.kde.org/r/119838/" title="https://git.reviewboard.kde.org/r/119838/">https://git.reviewboard.kde.org/r/119838/</a> The present patch adds an idleTimer and a shared lastAccessTime member. The idleTimer is reset each time a client performs one of a series of actions that I count as wallet accesses, and before resetting I update the idle timeout value from KConfig. When the timer fires, the elapsed time is compared to the shared last access time, and if it is >= the timeout, the wallet is closed. This applies only to "KDE keychains", so keychains used by OS X applications should not be affected.

- "close when last application exits". This requires maintaining a "user list" which keeps track of what application has what wallet open. I've implemented an "internal" version of such a registry, mapping wallet name to application names and the list of wallets they have open (a list of wallet reference, pid per application name). The registry is functional, but I have not yet decided (read: figured out) how to make a distributed representation of it.

So the work-in-progress concerns the distributed user registry. The idea would be to maintain the registry in shared memory, meaning it'd be reset (= disappear) when the last application exits, contrary to a file which can go stale. This would be simple if QSharedMemory objects could be resized, but apparently they cannot, so I'll have to look at other solutions possibly involving OS X frameworks (NSData and it's non-objectiveC version CFDataRef or CFMutableDataRef might be candidates). Suggestions welcome.

Other work in progress concerns a less wheel-reinventing approach that builds on kwalletd and DBus. I don't see why the code used in `kwallet.cpp` wouldn't work, but I must still misunderstand its finer details. The present patch contains outcommented code that does indeed cause kwalletd to be launched and slots and signals to become visible e.g. in `qdbusviewer`. But they don't work, which in turn makes the whole kwallet layer dysfunctional. Here too feedback is welcome on how what I'm missing and/or how to get this to work.
Once kwalletd works, wallet idle timeout closing and closing when the last client exits should work out-of-the-box, or at least I suppose.

Diffs (updated)
kdeui/util/kwallet.h d7f703f
kdeui/util/kwallet_mac.cpp 8344ebb
kdeui/util/qosxkeychain.h d0934e6
kdeui/util/qosxkeychain.cpp 7cb9a22

Diff: <a href="https://git.reviewboard.kde.org/r/120202/diff/" title="https://git.reviewboard.kde.org/r/120202/diff/">https://git.reviewboard.kde.org/r/120202/diff/</a>

Testing
OS X 10.6.8, kdelibs 4.14.1 git/master, KDE/MacPorts 4.12.5 .
Once finalised, all changes should port easily to KF5's kwallet_mac.cpp .

Thanks,

René J.V. Bertin

Re: Review Request 120202: [OS X] improvements to the kwallet/OS

By =?utf-8?Q?Thoma... at 09/21/2014 - 14:11

kdeui/util/qosxkeychain.h
<https://git.reviewboard.kde.org/r/120202/#comment46859>

If OSXKaychain is an exported class (i don't know), this is an ABI incompatible change.

It's also massively invasive and adds quite some overhead.

Why did you not just remove the #ifdef from the slot declaration in Wallet (former patch) and #ifdef the implementation body instead?
(There are several such internal slots present, you don't have to "fix" the Wallet architecture with this patch ;-)

If there's absolutely no other solution and you do not want to add the slot unconditionally, you can still reimplement protected ::timerEvent() and do the timer "the hard way", ie. "myTimer = startTimer(timeout); ... if (te->timerId() == myTimer()) { blahFoo; } else BaseClass::timerEvent(te);"

- Thomas Lübking

On Sept. 21, 2014, 4:40 nachm., René J.V. Bertin wrote:

Re: Review Request 120202: [OS X] improvements to the kwallet/OS

By =?utf-8?Q?Thoma... at 09/24/2014 - 10:22

Isn't at all.
It's just easier to revert since in Qt5 you can just use a lambda slot (what means you can write the functionality inline into the connect - no need for a slot function anywhere)
You can use the QTimer Object for this, yes (it'd be a bit wonky for some public interface, but as a semi-private class, there's no problem with this)

- Thomas

On Sept. 21, 2014, 4:40 nachm., René J.V. Bertin wrote:

Re: Review Request 120202: [OS X] improvements to the kwallet/OS

By =?utf-8?Q?Ren=C... at 09/23/2014 - 19:14

Well, I "decided against it" until I realised it wasn't really a big deal :)

Just how is "adding a QObject inheriting member to OSXKeyChain" any different in terms of additional overhead than just letting the class itself inherit QObject?

What I might do is define an additional class in QOSXKeychain.h, inheriting QTimer (which inherits QObject), give it a reference to the wallet instance, and then replace the QTimer* member in WalletPrivate with a pointer to that new class. That way the QObject overhead only occurs when I'm actually going to use the timer. Sounds like a plan to me (but that may just be the time of night ^^)

- René J.V.

On Sept. 21, 2014, 6:40 p.m., René J.V. Bertin wrote:

Re: Review Request 120202: [OS X] improvements to the kwallet/OS

By =?utf-8?Q?Thoma... at 09/23/2014 - 18:01

Not from my POV - it's far less invasive than altering the private baseclass.
You "decided against it" (although, timerEvent() would have to be implemented everywhere just as well)

Suggestion: isolate it by adding a QObject inheriting member to OSXKeyChain (to call a slot there) for this will no longer be a problem with Qt5 (YEAH for lambda slots! =)

- Thomas

On Sept. 21, 2014, 4:40 nachm., René J.V. Bertin wrote:

Re: Review Request 120202: [OS X] improvements to the kwallet/OS

By =?utf-8?Q?Ren=C... at 09/23/2014 - 16:43

Is it a really a big issue to introduce an empty and unused slot in the other Wallet implementation? (After all, "my" Wallet implementation has a number of slots too that are there only to satisfy the needs of the other ;) )

- René J.V.

On Sept. 21, 2014, 6:40 p.m., René J.V. Bertin wrote:

Re: Review Request 120202: [OS X] improvements to the kwallet/OS

By =?utf-8?Q?Thoma... at 09/23/2014 - 16:21

Ah, I now see: The various backends do not inherit a common base but are just split by architecture (on the same header)

As long as it's not referenced, a function does not have to be implemented.
BUT: moc will referece all slots, so NO. YOU MUST ADD IMPLEMENTATIONS EVERYWHERE.

(What you must not do as well in this regard, is to introduce pure virtual functions, ie "virtual void foo() = 0;" - the class becomes abstract by this and cannot be instatiated)

So the remaining option would be "int myTimer = QObject::startTimer(timeout);"

- Thomas

On Sept. 21, 2014, 4:40 nachm., René J.V. Bertin wrote:

Re: Review Request 120202: [OS X] improvements to the kwallet/OS

By =?utf-8?Q?Ren=C... at 09/21/2014 - 15:44

I would never have done things this way if QOSXKeychain was an exported class... but I'm sensible to the overhead argument. I also realise I could probably have declared it `protected QObject`.
I considered your suggestion, but decided against it because it would require patching kwallet.cpp too. Or at least I think it would, I presume one has to provide an implementation for every member function that's declared in the header? Unless one can make the declaration virtual and only provide an implementation in kwallet_mac.cpp (the sort of detail I just cannot seem to memorise :-/ )?

- René J.V.

On Sept. 21, 2014, 6:40 p.m., René J.V. Bertin wrote:

Re: Review Request 120202: [OS X] improvements to the kwallet/OS

By =?utf-8?Q?Ren=C... at 09/25/2014 - 09:58

(Updated Sept. 25, 2014, 3:58 p.m.)

Review request for KDE Software on Mac OS X and kdelibs.

Changes
I have addressed the idleTimer's slot issue by making the WalletPrivate class inherit QObject in addition to QOSXKeychain.h . For that I've had to move it to a new headerfile, `kwallet_mac.h`, a header I could have created earlier given the complexity `kwallet_mac.cpp` has attained. In the end this I thought this was the cleanest solution.

Now that this is out of the way (I hope), I'd appreciate some feedback on the 2 open questions:

1- what is missing from my DBus implementation that could explain why I see the slots and signals in qdbusviewer but calls sent to the slots never arrive in my code? Or rather, how do I get it to work?

2- how to complete the DBus-free wallet-user registry? The only thing missing is a method to share the structure in distributed memory without a central server. I'd need something like QSharedMemory with resizing capabilities.
Should I stop looking and share the reference to another QSharedMemory instance rather than share the registry's representation directly? A kind of shared handle (pointer to pointer, in old Apple speak from pre-MMU days).
The requirements are simple: each application having a Wallet open should be able to read the current registry contents ("user list"), and add or remove oneself to/from it.
All those operations can be performed on a copy freshly checked out of shared (and locked) memory but I fear it'd be rather delicate and race-condition prone. Each client will need to attach to the shared reference as well as the shared resource (to which that reference refers), and I think they'd all need to release the shared resource when the shared reference changes.

Any thoughts?

There was some demand from the kde-mac community to try and come up with an approach not requiring a central server (kwalletd), so I'd probably want to get approach 2 working even if we get approach 1 to function.

Repository: kdelibs

Description
I'm still working on (the KDE4-based version of) my OS X keychain backend for kwallet. I'm at a point where I think I can present a work-in-progress in an RR because at least one feature has been improved enough to be of interest for everyone, and also because I could use feedback on how to proceed.
I'm currently focussing on 2 settings that are configured in the kwallet KCM (SystemSettings), and for which I'm working on an implementation not requiring kwalletd and/or DBus.

- idle time closing of wallets. This feature was not supported in the commited version presented in <a href="https://git.reviewboard.kde.org/r/119838/" title="https://git.reviewboard.kde.org/r/119838/">https://git.reviewboard.kde.org/r/119838/</a> The present patch adds an idleTimer and a shared lastAccessTime member. The idleTimer is reset each time a client performs one of a series of actions that I count as wallet accesses, and before resetting I update the idle timeout value from KConfig. When the timer fires, the elapsed time is compared to the shared last access time, and if it is >= the timeout, the wallet is closed. This applies only to "KDE keychains", so keychains used by OS X applications should not be affected.

- "close when last application exits". This requires maintaining a "user list" which keeps track of what application has what wallet open. I've implemented an "internal" version of such a registry, mapping wallet name to application names and the list of wallets they have open (a list of wallet reference, pid per application name). The registry is functional, but I have not yet decided (read: figured out) how to make a distributed representation of it.

So the work-in-progress concerns the distributed user registry. The idea would be to maintain the registry in shared memory, meaning it'd be reset (= disappear) when the last application exits, contrary to a file which can go stale. This would be simple if QSharedMemory objects could be resized, but apparently they cannot, so I'll have to look at other solutions possibly involving OS X frameworks (NSData and it's non-objectiveC version CFDataRef or CFMutableDataRef might be candidates). Suggestions welcome.

Other work in progress concerns a less wheel-reinventing approach that builds on kwalletd and DBus. I don't see why the code used in `kwallet.cpp` wouldn't work, but I must still misunderstand its finer details. The present patch contains outcommented code that does indeed cause kwalletd to be launched and slots and signals to become visible e.g. in `qdbusviewer`. But they don't work, which in turn makes the whole kwallet layer dysfunctional. Here too feedback is welcome on how what I'm missing and/or how to get this to work.
Once kwalletd works, wallet idle timeout closing and closing when the last client exits should work out-of-the-box, or at least I suppose.

Diffs (updated)
kdeui/util/kwallet.h d7f703f
kdeui/util/kwallet_mac.cpp 8344ebb
kdeui/util/qosxkeychain.h d0934e6
kdeui/util/qosxkeychain.cpp 7cb9a22

Diff: <a href="https://git.reviewboard.kde.org/r/120202/diff/" title="https://git.reviewboard.kde.org/r/120202/diff/">https://git.reviewboard.kde.org/r/120202/diff/</a>

Testing
OS X 10.6.8, kdelibs 4.14.1 git/master, KDE/MacPorts 4.12.5 .
Once finalised, all changes should port easily to KF5's kwallet_mac.cpp .

Thanks,

René J.V. Bertin

Re: Review Request 120202: [OS X] improvements to the kwallet/OS

By =?utf-8?Q?Ren=C... at 10/15/2014 - 15:51

(Updated Oct. 15, 2014, 9:51 p.m.)

Review request for KDE Software on Mac OS X and kdelibs.

Changes
Better failure handling in `KWallet::QSharedValue`

Repository: kdelibs

Description
I'm still working on (the KDE4-based version of) my OS X keychain backend for kwallet. I'm at a point where I think I can present a work-in-progress in an RR because at least one feature has been improved enough to be of interest for everyone, and also because I could use feedback on how to proceed.
I'm currently focussing on 2 settings that are configured in the kwallet KCM (SystemSettings), and for which I'm working on an implementation not requiring kwalletd and/or DBus.

- idle time closing of wallets. This feature was not supported in the commited version presented in <a href="https://git.reviewboard.kde.org/r/119838/" title="https://git.reviewboard.kde.org/r/119838/">https://git.reviewboard.kde.org/r/119838/</a> The present patch adds an idleTimer and a shared lastAccessTime member. The idleTimer is reset each time a client performs one of a series of actions that I count as wallet accesses, and before resetting I update the idle timeout value from KConfig. When the timer fires, the elapsed time is compared to the shared last access time, and if it is >= the timeout, the wallet is closed. This applies only to "KDE keychains", so keychains used by OS X applications should not be affected.

- "close when last application exits". This requires maintaining a "user list" which keeps track of what application has what wallet open. I've implemented an "internal" version of such a registry, mapping wallet name to application names and the list of wallets they have open (a list of wallet reference, pid per application name). The registry is functional, but I have not yet decided (read: figured out) how to make a distributed representation of it.

So the work-in-progress concerns the distributed user registry. The idea would be to maintain the registry in shared memory, meaning it'd be reset (= disappear) when the last application exits, contrary to a file which can go stale. This would be simple if QSharedMemory objects could be resized, but apparently they cannot, so I'll have to look at other solutions possibly involving OS X frameworks (NSData and it's non-objectiveC version CFDataRef or CFMutableDataRef might be candidates). Suggestions welcome.

Other work in progress concerns a less wheel-reinventing approach that builds on kwalletd and DBus. I don't see why the code used in `kwallet.cpp` wouldn't work, but I must still misunderstand its finer details. The present patch contains outcommented code that does indeed cause kwalletd to be launched and slots and signals to become visible e.g. in `qdbusviewer`. But they don't work, which in turn makes the whole kwallet layer dysfunctional. Here too feedback is welcome on how what I'm missing and/or how to get this to work.
Once kwalletd works, wallet idle timeout closing and closing when the last client exits should work out-of-the-box, or at least I suppose.

Diffs (updated)
kdeui/util/kwallet.h d7f703f
kdeui/util/kwallet_mac.h PRE-CREATION
kdeui/util/kwallet_mac.cpp 8344ebb
kdeui/util/qosxkeychain.h d0934e6
kdeui/util/qosxkeychain.cpp 7cb9a22

Diff: <a href="https://git.reviewboard.kde.org/r/120202/diff/" title="https://git.reviewboard.kde.org/r/120202/diff/">https://git.reviewboard.kde.org/r/120202/diff/</a>

Testing
OS X 10.6.8, kdelibs 4.14.1 git/master, KDE/MacPorts 4.12.5 .
Once finalised, all changes should port easily to KF5's kwallet_mac.cpp .

Thanks,

René J.V. Bertin

Re: Review Request 120202: [OS X] improvements to the kwallet/OS

By =?utf-8?Q?Ren=C... at 10/16/2014 - 07:26

(Updated Oct. 16, 2014, 1:26 p.m.)

Review request for KDE Software on Mac OS X and kdelibs.

Changes
Corrects a regression introduced in yesterday's update (I really should get more sleep)

Repository: kdelibs

Description
I'm still working on (the KDE4-based version of) my OS X keychain backend for kwallet. I'm at a point where I think I can present a work-in-progress in an RR because at least one feature has been improved enough to be of interest for everyone, and also because I could use feedback on how to proceed.
I'm currently focussing on 2 settings that are configured in the kwallet KCM (SystemSettings), and for which I'm working on an implementation not requiring kwalletd and/or DBus.

- idle time closing of wallets. This feature was not supported in the commited version presented in <a href="https://git.reviewboard.kde.org/r/119838/" title="https://git.reviewboard.kde.org/r/119838/">https://git.reviewboard.kde.org/r/119838/</a> The present patch adds an idleTimer and a shared lastAccessTime member. The idleTimer is reset each time a client performs one of a series of actions that I count as wallet accesses, and before resetting I update the idle timeout value from KConfig. When the timer fires, the elapsed time is compared to the shared last access time, and if it is >= the timeout, the wallet is closed. This applies only to "KDE keychains", so keychains used by OS X applications should not be affected.

- "close when last application exits". This requires maintaining a "user list" which keeps track of what application has what wallet open. I've implemented an "internal" version of such a registry, mapping wallet name to application names and the list of wallets they have open (a list of wallet reference, pid per application name). The registry is functional, but I have not yet decided (read: figured out) how to make a distributed representation of it.

So the work-in-progress concerns the distributed user registry. The idea would be to maintain the registry in shared memory, meaning it'd be reset (= disappear) when the last application exits, contrary to a file which can go stale. This would be simple if QSharedMemory objects could be resized, but apparently they cannot, so I'll have to look at other solutions possibly involving OS X frameworks (NSData and it's non-objectiveC version CFDataRef or CFMutableDataRef might be candidates). Suggestions welcome.

Other work in progress concerns a less wheel-reinventing approach that builds on kwalletd and DBus. I don't see why the code used in `kwallet.cpp` wouldn't work, but I must still misunderstand its finer details. The present patch contains outcommented code that does indeed cause kwalletd to be launched and slots and signals to become visible e.g. in `qdbusviewer`. But they don't work, which in turn makes the whole kwallet layer dysfunctional. Here too feedback is welcome on how what I'm missing and/or how to get this to work.
Once kwalletd works, wallet idle timeout closing and closing when the last client exits should work out-of-the-box, or at least I suppose.

Diffs (updated)
kdeui/util/kwallet.h d7f703f
kdeui/util/kwallet_mac.h PRE-CREATION
kdeui/util/kwallet_mac.cpp 8344ebb
kdeui/util/qosxkeychain.h d0934e6
kdeui/util/qosxkeychain.cpp 7cb9a22

Diff: <a href="https://git.reviewboard.kde.org/r/120202/diff/" title="https://git.reviewboard.kde.org/r/120202/diff/">https://git.reviewboard.kde.org/r/120202/diff/</a>

Testing
OS X 10.6.8, kdelibs 4.14.1 git/master, KDE/MacPorts 4.12.5 .
Once finalised, all changes should port easily to KF5's kwallet_mac.cpp .

Thanks,

René J.V. Bertin

Re: Review Request 120202: [OS X] improvements to the kwallet/OS

By =?utf-8?Q?Ren=C... at 10/18/2014 - 14:45

(Updated Oct. 18, 2014, 8:45 p.m.)

Review request for KDE Software on Mac OS X and kdelibs.

Changes
some polishing to the idleTimer (someone in kdepim tries to set a negative interval after waking the system in the morning; setting the idleTimer's objectName should help determine if it is the "incriminated" timer or not).

Repository: kdelibs

Description
I'm still working on (the KDE4-based version of) my OS X keychain backend for kwallet. I'm at a point where I think I can present a work-in-progress in an RR because at least one feature has been improved enough to be of interest for everyone, and also because I could use feedback on how to proceed.
I'm currently focussing on 2 settings that are configured in the kwallet KCM (SystemSettings), and for which I'm working on an implementation not requiring kwalletd and/or DBus.

- idle time closing of wallets. This feature was not supported in the commited version presented in <a href="https://git.reviewboard.kde.org/r/119838/" title="https://git.reviewboard.kde.org/r/119838/">https://git.reviewboard.kde.org/r/119838/</a> The present patch adds an idleTimer and a shared lastAccessTime member. The idleTimer is reset each time a client performs one of a series of actions that I count as wallet accesses, and before resetting I update the idle timeout value from KConfig. When the timer fires, the elapsed time is compared to the shared last access time, and if it is >= the timeout, the wallet is closed. This applies only to "KDE keychains", so keychains used by OS X applications should not be affected.

- "close when last application exits". This requires maintaining a "user list" which keeps track of what application has what wallet open. I've implemented an "internal" version of such a registry, mapping wallet name to application names and the list of wallets they have open (a list of wallet reference, pid per application name). The registry is functional, but I have not yet decided (read: figured out) how to make a distributed representation of it.

So the work-in-progress concerns the distributed user registry. The idea would be to maintain the registry in shared memory, meaning it'd be reset (= disappear) when the last application exits, contrary to a file which can go stale. This would be simple if QSharedMemory objects could be resized, but apparently they cannot, so I'll have to look at other solutions possibly involving OS X frameworks (NSData and it's non-objectiveC version CFDataRef or CFMutableDataRef might be candidates). Suggestions welcome.

Other work in progress concerns a less wheel-reinventing approach that builds on kwalletd and DBus. I don't see why the code used in `kwallet.cpp` wouldn't work, but I must still misunderstand its finer details. The present patch contains outcommented code that does indeed cause kwalletd to be launched and slots and signals to become visible e.g. in `qdbusviewer`. But they don't work, which in turn makes the whole kwallet layer dysfunctional. Here too feedback is welcome on how what I'm missing and/or how to get this to work.
Once kwalletd works, wallet idle timeout closing and closing when the last client exits should work out-of-the-box, or at least I suppose.

Diffs (updated)
kdeui/util/kwallet.h d7f703f
kdeui/util/kwallet_mac.h PRE-CREATION
kdeui/util/kwallet_mac.cpp 8344ebb
kdeui/util/qosxkeychain.h d0934e6
kdeui/util/qosxkeychain.cpp 7cb9a22

Diff: <a href="https://git.reviewboard.kde.org/r/120202/diff/" title="https://git.reviewboard.kde.org/r/120202/diff/">https://git.reviewboard.kde.org/r/120202/diff/</a>

Testing (updated)
OS X 10.6.8, kdelibs 4.14.2 git/master, KDE/MacPorts 4.12.5 & 4.13.3.
Once finalised, all changes should port easily to KF5's kwallet_mac.cpp .

Thanks,

René J.V. Bertin

Re: Review Request 120202: [OS X] improvements to the kwallet/OS

By =?utf-8?Q?Ren=C... at 11/19/2014 - 13:00

(Updated Nov. 19, 2014, 6 p.m.)

Review request for KDE Software on Mac OS X and kdelibs.

Changes
- qDebug statements converted to kDebug/kWarning or moved inside conditional blocks
- initial implementation of disconnectApplication using a shared reference counter (in the lastAccessTime object)

Repository: kdelibs

Description
I'm still working on (the KDE4-based version of) my OS X keychain backend for kwallet. I'm at a point where I think I can present a work-in-progress in an RR because at least one feature has been improved enough to be of interest for everyone, and also because I could use feedback on how to proceed.
I'm currently focussing on 2 settings that are configured in the kwallet KCM (SystemSettings), and for which I'm working on an implementation not requiring kwalletd and/or DBus.

- idle time closing of wallets. This feature was not supported in the commited version presented in <a href="https://git.reviewboard.kde.org/r/119838/" title="https://git.reviewboard.kde.org/r/119838/">https://git.reviewboard.kde.org/r/119838/</a> The present patch adds an idleTimer and a shared lastAccessTime member. The idleTimer is reset each time a client performs one of a series of actions that I count as wallet accesses, and before resetting I update the idle timeout value from KConfig. When the timer fires, the elapsed time is compared to the shared last access time, and if it is >= the timeout, the wallet is closed. This applies only to "KDE keychains", so keychains used by OS X applications should not be affected.

- "close when last application exits". This requires maintaining a "user list" which keeps track of what application has what wallet open. I've implemented an "internal" version of such a registry, mapping wallet name to application names and the list of wallets they have open (a list of wallet reference, pid per application name). The registry is functional, but I have not yet decided (read: figured out) how to make a distributed representation of it.

So the work-in-progress concerns the distributed user registry. The idea would be to maintain the registry in shared memory, meaning it'd be reset (= disappear) when the last application exits, contrary to a file which can go stale. This would be simple if QSharedMemory objects could be resized, but apparently they cannot, so I'll have to look at other solutions possibly involving OS X frameworks (NSData and it's non-objectiveC version CFDataRef or CFMutableDataRef might be candidates). Suggestions welcome.

Other work in progress concerns a less wheel-reinventing approach that builds on kwalletd and DBus. I don't see why the code used in `kwallet.cpp` wouldn't work, but I must still misunderstand its finer details. The present patch contains outcommented code that does indeed cause kwalletd to be launched and slots and signals to become visible e.g. in `qdbusviewer`. But they don't work, which in turn makes the whole kwallet layer dysfunctional. Here too feedback is welcome on how what I'm missing and/or how to get this to work.
Once kwalletd works, wallet idle timeout closing and closing when the last client exits should work out-of-the-box, or at least I suppose.

Diffs (updated)
kdeui/util/kwallet_mac.h PRE-CREATION
kdeui/util/kwallet_mac.cpp 8344ebb
kdeui/util/qosxkeychain.h d0934e6
kdeui/util/qosxkeychain.cpp 7cb9a22

Diff: <a href="https://git.reviewboard.kde.org/r/120202/diff/" title="https://git.reviewboard.kde.org/r/120202/diff/">https://git.reviewboard.kde.org/r/120202/diff/</a>

Testing
OS X 10.6.8, kdelibs 4.14.2 git/master, KDE/MacPorts 4.12.5 & 4.13.3.
Once finalised, all changes should port easily to KF5's kwallet_mac.cpp .

Thanks,

René J.V. Bertin

Re: Review Request 120202: [OS X] improvements to the kwallet/OS

By =?utf-8?Q?Ren=C... at 11/19/2014 - 13:05

(Updated Nov. 19, 2014, 6:05 p.m.)

Review request for KDE Software on Mac OS X and kdelibs.

Repository: kdelibs

Description
I'm still working on (the KDE4-based version of) my OS X keychain backend for kwallet. I'm at a point where I think I can present a work-in-progress in an RR because at least one feature has been improved enough to be of interest for everyone, and also because I could use feedback on how to proceed.
I'm currently focussing on 2 settings that are configured in the kwallet KCM (SystemSettings), and for which I'm working on an implementation not requiring kwalletd and/or DBus.

- idle time closing of wallets. This feature was not supported in the commited version presented in <a href="https://git.reviewboard.kde.org/r/119838/" title="https://git.reviewboard.kde.org/r/119838/">https://git.reviewboard.kde.org/r/119838/</a> The present patch adds an idleTimer and a shared lastAccessTime member. The idleTimer is reset each time a client performs one of a series of actions that I count as wallet accesses, and before resetting I update the idle timeout value from KConfig. When the timer fires, the elapsed time is compared to the shared last access time, and if it is >= the timeout, the wallet is closed. This applies only to "KDE keychains", so keychains used by OS X applications should not be affected.

- "close when last application exits". This requires maintaining a "user list" which keeps track of what application has what wallet open. I've implemented an "internal" version of such a registry, mapping wallet name to application names and the list of wallets they have open (a list of wallet reference, pid per application name). The registry is functional, but I have not yet decided (read: figured out) how to make a distributed representation of it.

So the work-in-progress concerns the distributed user registry. The idea would be to maintain the registry in shared memory, meaning it'd be reset (= disappear) when the last application exits, contrary to a file which can go stale. This would be simple if QSharedMemory objects could be resized, but apparently they cannot, so I'll have to look at other solutions possibly involving OS X frameworks (NSData and it's non-objectiveC version CFDataRef or CFMutableDataRef might be candidates). Suggestions welcome.

Other work in progress concerns a less wheel-reinventing approach that builds on kwalletd and DBus. I don't see why the code used in `kwallet.cpp` wouldn't work, but I must still misunderstand its finer details. The present patch contains outcommented code that does indeed cause kwalletd to be launched and slots and signals to become visible e.g. in `qdbusviewer`. But they don't work, which in turn makes the whole kwallet layer dysfunctional. Here too feedback is welcome on how what I'm missing and/or how to get this to work.
Once kwalletd works, wallet idle timeout closing and closing when the last client exits should work out-of-the-box, or at least I suppose.

Diffs
kdeui/util/kwallet_mac.h PRE-CREATION
kdeui/util/kwallet_mac.cpp 8344ebb
kdeui/util/qosxkeychain.h d0934e6
kdeui/util/qosxkeychain.cpp 7cb9a22

Diff: <a href="https://git.reviewboard.kde.org/r/120202/diff/" title="https://git.reviewboard.kde.org/r/120202/diff/">https://git.reviewboard.kde.org/r/120202/diff/</a>

Testing
OS X 10.6.8, kdelibs 4.14.2 git/master, KDE/MacPorts 4.12.5 & 4.13.3.
Once finalised, all changes should port easily to KF5's kwallet_mac.cpp .

Thanks,

René J.V. Bertin

Re: Review Request 120202: [OS X] improvements to the kwallet/OS

By =?utf-8?Q?Ren=C... at 01/03/2015 - 15:41

(Updated Jan. 3, 2015, 8:41 p.m.)

Review request for KDE Software on Mac OS X and kdelibs.

Changes
This version caters to OS X 10.9 and newer which no longer added newly created keychains to the keychain search list. It also introduces a slight change to the internal implementation of wallet folders. OS X will ask access permission to keychain items when an application previously granted permission has been updated (or when a not yet allowed application attempts to access an item), and presents a dialog identifying the item with the field that carries the KWallet folder name. This often lead to series of requests seemingly for the same item (= all entries in the wallet folder). The field in question ("Where" in Keychain Access) now also contains the account name so the access requests are more informative.

Repository: kdelibs

Description
I'm still working on (the KDE4-based version of) my OS X keychain backend for kwallet. I'm at a point where I think I can present a work-in-progress in an RR because at least one feature has been improved enough to be of interest for everyone, and also because I could use feedback on how to proceed.
I'm currently focussing on 2 settings that are configured in the kwallet KCM (SystemSettings), and for which I'm working on an implementation not requiring kwalletd and/or DBus.

- idle time closing of wallets. This feature was not supported in the commited version presented in <a href="https://git.reviewboard.kde.org/r/119838/" title="https://git.reviewboard.kde.org/r/119838/">https://git.reviewboard.kde.org/r/119838/</a> The present patch adds an idleTimer and a shared lastAccessTime member. The idleTimer is reset each time a client performs one of a series of actions that I count as wallet accesses, and before resetting I update the idle timeout value from KConfig. When the timer fires, the elapsed time is compared to the shared last access time, and if it is >= the timeout, the wallet is closed. This applies only to "KDE keychains", so keychains used by OS X applications should not be affected.

- "close when last application exits". This requires maintaining a "user list" which keeps track of what application has what wallet open. I've implemented an "internal" version of such a registry, mapping wallet name to application names and the list of wallets they have open (a list of wallet reference, pid per application name). The registry is functional, but I have not yet decided (read: figured out) how to make a distributed representation of it.

So the work-in-progress concerns the distributed user registry. The idea would be to maintain the registry in shared memory, meaning it'd be reset (= disappear) when the last application exits, contrary to a file which can go stale. This would be simple if QSharedMemory objects could be resized, but apparently they cannot, so I'll have to look at other solutions possibly involving OS X frameworks (NSData and it's non-objectiveC version CFDataRef or CFMutableDataRef might be candidates). Suggestions welcome.

Other work in progress concerns a less wheel-reinventing approach that builds on kwalletd and DBus. I don't see why the code used in `kwallet.cpp` wouldn't work, but I must still misunderstand its finer details. The present patch contains outcommented code that does indeed cause kwalletd to be launched and slots and signals to become visible e.g. in `qdbusviewer`. But they don't work, which in turn makes the whole kwallet layer dysfunctional. Here too feedback is welcome on how what I'm missing and/or how to get this to work.
Once kwalletd works, wallet idle timeout closing and closing when the last client exits should work out-of-the-box, or at least I suppose.

Diffs (updated)
kdeui/util/kwallet_mac.h PRE-CREATION
kdeui/util/kwallet_mac.cpp 8344ebb
kdeui/util/qosxkeychain.h d0934e6
kdeui/util/qosxkeychain.cpp 7cb9a22

Diff: <a href="https://git.reviewboard.kde.org/r/120202/diff/" title="https://git.reviewboard.kde.org/r/120202/diff/">https://git.reviewboard.kde.org/r/120202/diff/</a>

Testing
OS X 10.6.8, kdelibs 4.14.2 git/master, KDE/MacPorts 4.12.5 & 4.13.3.
Once finalised, all changes should port easily to KF5's kwallet_mac.cpp .

Thanks,

René J.V. Bertin

Re: Review Request 120202: [OS X] improvements to the kwallet/OS

By =?utf-8?Q?Ren=C... at 01/03/2015 - 15:42

(Updated Jan. 3, 2015, 8:42 p.m.)

Review request for KDE Software on Mac OS X and kdelibs.

Repository: kdelibs

Description
I'm still working on (the KDE4-based version of) my OS X keychain backend for kwallet. I'm at a point where I think I can present a work-in-progress in an RR because at least one feature has been improved enough to be of interest for everyone, and also because I could use feedback on how to proceed.
I'm currently focussing on 2 settings that are configured in the kwallet KCM (SystemSettings), and for which I'm working on an implementation not requiring kwalletd and/or DBus.

- idle time closing of wallets. This feature was not supported in the commited version presented in <a href="https://git.reviewboard.kde.org/r/119838/" title="https://git.reviewboard.kde.org/r/119838/">https://git.reviewboard.kde.org/r/119838/</a> The present patch adds an idleTimer and a shared lastAccessTime member. The idleTimer is reset each time a client performs one of a series of actions that I count as wallet accesses, and before resetting I update the idle timeout value from KConfig. When the timer fires, the elapsed time is compared to the shared last access time, and if it is >= the timeout, the wallet is closed. This applies only to "KDE keychains", so keychains used by OS X applications should not be affected.

- "close when last application exits". This requires maintaining a "user list" which keeps track of what application has what wallet open. I've implemented an "internal" version of such a registry, mapping wallet name to application names and the list of wallets they have open (a list of wallet reference, pid per application name). The registry is functional, but I have not yet decided (read: figured out) how to make a distributed representation of it.

So the work-in-progress concerns the distributed user registry. The idea would be to maintain the registry in shared memory, meaning it'd be reset (= disappear) when the last application exits, contrary to a file which can go stale. This would be simple if QSharedMemory objects could be resized, but apparently they cannot, so I'll have to look at other solutions possibly involving OS X frameworks (NSData and it's non-objectiveC version CFDataRef or CFMutableDataRef might be candidates). Suggestions welcome.

Other work in progress concerns a less wheel-reinventing approach that builds on kwalletd and DBus. I don't see why the code used in `kwallet.cpp` wouldn't work, but I must still misunderstand its finer details. The present patch contains outcommented code that does indeed cause kwalletd to be launched and slots and signals to become visible e.g. in `qdbusviewer`. But they don't work, which in turn makes the whole kwallet layer dysfunctional. Here too feedback is welcome on how what I'm missing and/or how to get this to work.
Once kwalletd works, wallet idle timeout closing and closing when the last client exits should work out-of-the-box, or at least I suppose.

Diffs
kdeui/util/kwallet_mac.h PRE-CREATION
kdeui/util/kwallet_mac.cpp 8344ebb
kdeui/util/qosxkeychain.h d0934e6
kdeui/util/qosxkeychain.cpp 7cb9a22

Diff: <a href="https://git.reviewboard.kde.org/r/120202/diff/" title="https://git.reviewboard.kde.org/r/120202/diff/">https://git.reviewboard.kde.org/r/120202/diff/</a>

Testing (updated)
OS X 10.6.8 and OS X 10.9.5, kdelibs 4.14.n git/master, KDE/MacPorts 4.12.5 & 4.13.3.
Once finalised, all changes should port easily to KF5's kwallet_mac.cpp .

Thanks,

René J.V. Bertin

Re: Review Request 120202: [OS X] improvements to the kwallet/OS

By =?utf-8?Q?Ren=C... at 08/17/2015 - 11:00

(Updated Aug. 17, 2015, 5 p.m.)

Review request for KDE Software on Mac OS X and kdelibs.

Changes
Improved handling of the QSharedMemory create/attach functionality for the wallet access timer feature. Attempting an attach in order to create the shared segment only when the attach failed can lead to race conditions where a different process manages to create the segment while we're still trying to attach. This is actually documented in the QSharedMemory notes for QNX.

Repository: kdelibs

Description
I'm still working on (the KDE4-based version of) my OS X keychain backend for kwallet. I'm at a point where I think I can present a work-in-progress in an RR because at least one feature has been improved enough to be of interest for everyone, and also because I could use feedback on how to proceed.
I'm currently focussing on 2 settings that are configured in the kwallet KCM (SystemSettings), and for which I'm working on an implementation not requiring kwalletd and/or DBus.

- idle time closing of wallets. This feature was not supported in the commited version presented in <a href="https://git.reviewboard.kde.org/r/119838/" title="https://git.reviewboard.kde.org/r/119838/">https://git.reviewboard.kde.org/r/119838/</a> The present patch adds an idleTimer and a shared lastAccessTime member. The idleTimer is reset each time a client performs one of a series of actions that I count as wallet accesses, and before resetting I update the idle timeout value from KConfig. When the timer fires, the elapsed time is compared to the shared last access time, and if it is >= the timeout, the wallet is closed. This applies only to "KDE keychains", so keychains used by OS X applications should not be affected.

- "close when last application exits". This requires maintaining a "user list" which keeps track of what application has what wallet open. I've implemented an "internal" version of such a registry, mapping wallet name to application names and the list of wallets they have open (a list of wallet reference, pid per application name). The registry is functional, but I have not yet decided (read: figured out) how to make a distributed representation of it.

So the work-in-progress concerns the distributed user registry. The idea would be to maintain the registry in shared memory, meaning it'd be reset (= disappear) when the last application exits, contrary to a file which can go stale. This would be simple if QSharedMemory objects could be resized, but apparently they cannot, so I'll have to look at other solutions possibly involving OS X frameworks (NSData and it's non-objectiveC version CFDataRef or CFMutableDataRef might be candidates). Suggestions welcome.

Other work in progress concerns a less wheel-reinventing approach that builds on kwalletd and DBus. I don't see why the code used in `kwallet.cpp` wouldn't work, but I must still misunderstand its finer details. The present patch contains outcommented code that does indeed cause kwalletd to be launched and slots and signals to become visible e.g. in `qdbusviewer`. But they don't work, which in turn makes the whole kwallet layer dysfunctional. Here too feedback is welcome on how what I'm missing and/or how to get this to work.
Once kwalletd works, wallet idle timeout closing and closing when the last client exits should work out-of-the-box, or at least I suppose.

Diffs (updated)
kdeui/util/kwallet_mac.h PRE-CREATION
kdeui/util/kwallet_mac.cpp 8344ebb
kdeui/util/qosxkeychain.h d0934e6
kdeui/util/qosxkeychain.cpp 7cb9a22

Diff: <a href="https://git.reviewboard.kde.org/r/120202/diff/" title="https://git.reviewboard.kde.org/r/120202/diff/">https://git.reviewboard.kde.org/r/120202/diff/</a>

Testing
OS X 10.6.8 and OS X 10.9.5, kdelibs 4.14.n git/master, KDE/MacPorts 4.12.5 & 4.13.3.
Once finalised, all changes should port easily to KF5's kwallet_mac.cpp .

Thanks,

René J.V. Bertin

Re: Review Request 120202: [OS X] improvements to the kwallet/OS

By =?utf-8?Q?Thoma... at 10/15/2014 - 16:07

Please check <a href="https://techbase.kde.org/Policies/Kdelibs_Coding_Style" title="https://techbase.kde.org/Policies/Kdelibs_Coding_Style">https://techbase.kde.org/Policies/Kdelibs_Coding_Style</a>

- Thomas Lübking

On Okt. 15, 2014, 7:51 nachm., René J.V. Bertin wrote:

Re: Review Request 120202: [OS X] improvements to the kwallet/OS

By =?utf-8?Q?Ren=C... at 10/16/2014 - 17:16

Nope, I thought I'd been avoiding that habit but it's possible a few (only declarations I hope) slipped through . Can't recall seeing anything making it "not done" in the guidelines though, did I miss it?

- René J.V.

On Oct. 16, 2014, 1:26 p.m., René J.V. Bertin wrote:

Re: Review Request 120202: [OS X] improvements to the kwallet/OS

By =?utf-8?Q?Thoma... at 10/16/2014 - 14:42

Ok, I oversaw that this is entirely your code.
Well if this is actually what makes you happy, that's oc fine. And when you ever should abandon the code, there's luckily always astyle to the rescue :-P

Not only. You've *sometimes* stuff like
```cpp
void foo()
{ bar();
fooBar();
}
```

and blanks in braces isn't consistent (or at least I fail to see the pattern) - I frankly thought this was a mixup of xcode and "my first day in vim" ;-)

- Thomas

On Okt. 16, 2014, 11:26 vorm., René J.V. Bertin wrote:

Re: Review Request 120202: [OS X] improvements to the kwallet/OS

By =?utf-8?Q?Ren=C... at 10/16/2014 - 07:30

Sure, and guess what I noticed first ;)

```
Nobody is forced to use this style, but to have consistent formatting of the source code files it is recommended to make use of it.
```

I won't mix and match styles in existing code, trying instead to match the style used "locally" as closely as possible. In files I create from scratch I'd prefer to stick to my own convictions, though - basically the only difference is the absence of a space after a keyword. It's not like that isn't exactly unprecedented ...

- René J.V.

On Oct. 16, 2014, 1:26 p.m., René J.V. Bertin wrote:

Re: Review Request 120202: [OS X] improvements to the kwallet/OS

By Albert Astals Cid at 09/18/2014 - 18:28

kdeui/util/kwallet.h
<https://git.reviewboard.kde.org/r/120202/#comment46647>

This is bad, slots in an ifdef are a bad idea.

Is there any reason this slot has to be in KWallet and not some other object?

- Albert Astals Cid

On set. 14, 2014, 3:36 p.m., René J.V. Bertin wrote:

Re: Review Request 120202: [OS X] improvements to the kwallet/OS

By =?utf-8?Q?Thoma... at 09/24/2014 - 05:44

file a bug. As mentioned, moc does not handle preproc statements, so it will add the slot to the metaobject, but the referenced function does not exist in your binary.
The "correct" solution is to keep the definition and skip only the function body in the implementation.

The only way this could have worked before was that kwalletd.moc wasn't updated before (there's a usually timestamp comparism to check whether moc is still up-to-date on the header. Very funny if your local ntp server goes wild ;-)

- Thomas

On Sept. 21, 2014, 4:40 nachm., René J.V. Bertin wrote:

Re: Review Request 120202: [OS X] improvements to the kwallet/OS

By Ian Wadham at 09/23/2014 - 22:36

Re this issue, I just found a problem building KWalletD from KDE 4 kde-runtime master. About a year and 2 weeks ago, Valentin Rusu, added a slot called connectToScreenSaver() to kwalletd.h and kwalletd.cpp, with an #ifdef Q_WS_X11 conditional wrapping the code in both files. This is now failing to build on my machine, using Clang:

In file included from /kdedev/kde4m/kdesrc/kde/kde-runtime/kwalletd/kwalletd.cpp:1670:
/kdedev/kde4m/kdesrc/build/kde/kde-runtime/kwalletd/kwalletd.moc:267:22: error:
no member named 'connectToScreenSaver' in 'KWalletD'
case 60: _t->connectToScreenSaver(); break;
~~ ^
In file included from /kdedev/kde4m/kdesrc/kde/kde-runtime/kwalletd/kwalletd.cpp:25:
/kdedev/kde4m/kdesrc/kde/kde-runtime/kwalletd/kwalletd.h:247:14: warning: private
field '_useGpg' is not used [-Wunused-private-field]
bool _useGpg;
^
1 warning and 1 error generated.

Any ideas on the "error"? Also the "warning" occurs on KDE 4.14, but not KDE 4.13.

The same "error" arose in my KDE 4.13 build setup after I "touched" kwalletd.h and kwalletd.cpp, but those files have always built OK previously. It is probably at least 2 months since I last compiled KWalletD on my KDE 4.13, so perhaps I now have a pickier compiler (installed by MacPorts) or maybe Automoc and MOC are doing something different.

- Ian

On Sept. 21, 2014, 4:40 p.m., René J.V. Bertin wrote:

Re: Review Request 120202: [OS X] improvements to the kwallet/OS

By =?utf-8?Q?Ren=C... at 09/21/2014 - 12:39

No, the WalletPrivate class didn't need one until now. I think I figured something out (see the updated diff I'll be uploading). I'm not perfectly happy with making `QOSXKeychain` inherit `QObject` and putting the slot declaration in that class (as a virtual), but the alternative would apparently have been to use multiple inheritance in WalletPrivate. I generally prefer to avoid multiple inheritance, and I'm also not sure to what extent moc would have been able to pick up the necessary bits when hidden in class that only exists in a cpp file .

- René J.V.

On Sept. 14, 2014, 5:36 p.m., René J.V. Bertin wrote:

Re: Review Request 120202: [OS X] improvements to the kwallet/OS

By Albert Astals Cid at 09/21/2014 - 06:39

An ifdef in a public class is horrible.

As a user of the KWallet class when i should connect to it? Never? Then don't show me to me it exists.

- Albert

On set. 14, 2014, 3:36 p.m., René J.V. Bertin wrote:

Re: Review Request 120202: [OS X] improvements to the kwallet/OS

By =?utf-8?Q?Ren=C... at 09/21/2014 - 06:35

May I ask why slots in an #ifdef are a bad idea, worse than any other kind of code? I can see why platform-specific class members are not very elegant, but not why that would be different for slots.

The slot has to have access to the Wallet instance, but it should be possible to move it into the KWalletPrivate class since I already added a pointer to the instance to that class. Would that be better?

- René J.V.

On Sept. 14, 2014, 5:36 p.m., René J.V. Bertin wrote:

Re: Review Request 120202: [OS X] improvements to the kwallet/OS

By =?utf-8?Q?Thoma... at 09/21/2014 - 07:38

Indeed (jaws wide open) - so moc doesn't even care about that attribute.

Good to know, I foresee abuse >-)

- Thomas

On Sept. 14, 2014, 3:36 nachm., René J.V. Bertin wrote:

Re: Review Request 120202: [OS X] improvements to the kwallet/OS

By Albert Astals Cid at 09/21/2014 - 12:20

Does the object you're adding the slot have a Q_OBJECT?

- Albert

On set. 14, 2014, 3:36 p.m., René J.V. Bertin wrote:

Re: Review Request 120202: [OS X] improvements to the kwallet/OS

By Albert Astals Cid at 09/21/2014 - 07:30

@Thomas, i think last i checked you can still to connect to private slots, the only thing is that you can't call them directly, but the metaobject doesn't know about "private".

- Albert

On set. 14, 2014, 3:36 p.m., René J.V. Bertin wrote:

Re: Review Request 120202: [OS X] improvements to the kwallet/OS

By =?utf-8?Q?Ren=C... at 09/21/2014 - 11:56

OK, implementation question.

How do I declare a slot in a private class that doesn't have a specific header file?
Putting `private QSLOT` above the function definition makes things compile, but the runtime complains about a missing slot (curiously even expecting it in KWallet::Wallet). Yes, I did update the connect call to pass in the pointer to the parent class ....

- René J.V.

On Sept. 14, 2014, 5:36 p.m., René J.V. Bertin wrote:

Re: Review Request 120202: [OS X] improvements to the kwallet/OS

By Rolf Eike Beer at 09/21/2014 - 12:20

Use Q_PRIVATE_SLOT in the public header?

Eike

Re: Review Request 120202: [OS X] improvements to the kwallet/OS

By =?utf-8?Q?Ren=C... at 09/21/2014 - 07:04

Understood (and agreed). Not why moc doesn't take ifdefs into account, but that may be a design choice, and isn't relevant here.

- René J.V.

On Sept. 14, 2014, 5:36 p.m., René J.V. Bertin wrote:

Re: Review Request 120202: [OS X] improvements to the kwallet/OS

By =?utf-8?Q?Thoma... at 09/21/2014 - 06:53

moc does not handle preprocessor statements.
You'll likely get some error if the condition does not match because moc adds the slot unconditionally, but the function isn't available.

That aside, #ifdef'ing functions in a public header (ie. exported API) is a bad idea in general, because it causes different ABI (not that much a problem of an architecure split) and usually confusion.

-> preattyplease #ifdef the implementation instead.

```cpp
void Foo::bar()
{
#if WANT_THIS
fooBarFoo();
#endif
}
```

@Albert
tbf, there're quite some present slots tagged "internal" in that class and since they're all private slots, they're not available to user code anyway. The general approach is ok, because they don't affect the vtable.

- Thomas

On Sept. 14, 2014, 3:36 nachm., René J.V. Bertin wrote: