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

[Thomas Lübking]

> On Sept. 21, 2014, 6:11 nachm., Thomas Lübking wrote:
> > kdeui/util/qosxkeychain.h, line 99
> > <https://git.reviewboard.kde.org/r/120202/diff/2/?file=314175#file314175line99>
> >
> >     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);"
> 
> René J.V. Bertin wrote:
>     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 :-/ )?

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


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/120202/#review67155
-----------------------------------------------------------


On Sept. 21, 2014, 4:40 nachm., René J.V. Bertin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/120202/
> -----------------------------------------------------------
> 
> (Updated Sept. 21, 2014, 4:40 nachm.)
> 
> 
> 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 https://git.reviewboard.kde.org/r/119838/ 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: https://git.reviewboard.kde.org/r/120202/diff/
> 
> 
> 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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-mac/attachments/20140923/90873658/attachment.html>