On Friday 12 July 2013 19:38:50 Thomas Lübking wrote: > >> + disconnect(job, 0, 0, 0); > >> + job->stop(); > >> +++ // TODO how do the get off the hash? > > > > QLineEdit is asociated with job. So other option could be to > > move job pointer to new class inhered by QLineEdit. > > That doesn't scale because everytime you want to change the > assiciation, you'll have to re-inherit something. > Just ensure to remove the element from the hash/map before > deleting it (i did not see this happen anywhere) to ensure > you've no dangeling pointers there. >
Above disconnect() is line:
AddressbookJob *job = m_completionRequests.take(toEdit);
And take method will return element and after that will remove it
from hash/map. So element is removed from hash/map.
> >> +++ // TODO QMap is gonna perform better here
> >> + QHash <QLineEdit *, AddressbookJob *>
> >> m_completionRequests; +
> >
> > I thought that QHash is better for pointers (integers).
>
> The data type is nearby irrelevant.
> QHash is more expensive at insertion and has a fixed cost
> part, which makes it more expensive for very short
> maps/hashes - as we can expect here - anyway.
>
Ok, changed code to use QMap instead.
> >> +void SettingsDialog::slotAccept()
> >> +{
> >> + disconnect(sender(), SIGNAL(saved()), this,
> >> SLOT(slotAccept())); + saveSignalCount--;
> >> +++ // TODO QT5, test isConnected
> >
> > Why is needed to test if is connected? QDialog::accept() is
> > called after all (above) widgets emit signal saved.
>
> To avoid relying on the saveSignalCount counter.
>
So rather test in slotAccept() if all 10 signals are still
connected? I think it is better to have counter instead checking
everytime all 10 signals if are connected...
> > Why to cache password? Also in kwallet you can change
> > password.
>
> To avoid having to wait and eventually even fail (when the PW
> backend is -temporarily- not accessible or even the server
> has a authentification timeout below the PW backend reply
> time) -> cache password, try cached if available, if it
> fails, ask the PW plugin, if it differs, update the cache and
> use the new one for authentication, otherwise ask the user
> for the password (and remove/update the cached PW as well)
>
So where should be password cached? In PluginLoader code? In
PasswordInterface (plugin) class? Or in plugin itself? Or
somewhere else in Trojita code?
> > Why to use QMetaObject::invokeMethod instead
> > QTimer::singleShot?
>
> a) to avoid having to include QTimer
> b) because you rely on Qt's workaround for a very popular
> clientcode abuse (QMetaObject::invokeMethod with
> QueuedConnection does the same and is internally used by
> QTimer, as Kevin pointed out, because really MAAAANY users
> abuse the zero timer singleshot)
>
Ok, I can change it.
But to be more consistent, I look into trojita code and there are
34x calls with QTimer and 1x call with QMetaObject...
> > I need to make sure that plugins implement doStart and
> > doStop methods. And it is not possible to force that
> > without pure virtual methods.
>
> Yes, i was just pointing out they usually don't have to.
>
> > m_wallet is not dialog but single object for accessing
> > kwallet. it can be reused (if wallet was not closed) for
> > next kwallet access, so I dot no removing it. Dialog is
> > shown by some kde process and I do not know how to tell it
> > application name which accessing wallet.
>
> Yes, and the kwallet dialog is gonna be removed (by default)
> as it implies security that is absolutely not present. So
> it's not really important to have a very special "Application
> Name" here - you can just pass "Trojita", as long as you keep
> it, there should be no problem.
>
Problem is that I do not know how to pass "Trojita" string to
kwallet open wallet confirm dialog :-(
Kevin, can you help me? How to tell kwallet that application
"Trojita" wants to open wallet and not "unknown" application?
I did not find nothing in api on:
http://api.kde.org/4.10-api/kdelibs-apidocs/kdeui/html/classKWallet_1_1Wallet.html
>
> Cheers,
> Thomas
--
Pali Rohár
pali.ro...@gmail.com
signature.asc
Description: This is a digitally signed message part.
