Changeset View
Standalone View
src/core/slavebase.cpp
Show First 20 Lines • Show All 138 Lines • ▼ Show 20 Line(s) | 137 | if (!m_confirmationAsked) { | |||
---|---|---|---|---|---|
139 | if (status == SlaveBase::Cancel) { | 139 | if (status == SlaveBase::Cancel) { | ||
140 | return OperationCanceled; | 140 | return OperationCanceled; | ||
141 | } | 141 | } | ||
142 | m_confirmationAsked = true; | 142 | m_confirmationAsked = true; | ||
143 | } | 143 | } | ||
144 | return OperationAllowed; | 144 | return OperationAllowed; | ||
145 | } | 145 | } | ||
146 | 146 | | |||
147 | bool hasTempAuth() | 147 | bool hasTempAuth(bool revoke = false) | ||
dfaure: That looks like horrible API ;)
A getter that optionally makes changes, based on a boolean... | |||||
Now I feel stupid for doing this. Apart from that I am not even confident about this change because it involves changes in KAuth (Obviously I am not sure about them either). How about we drop this and the related revisions for now and just delete the slave whenever there's authorization? chinmoyr: Now I feel stupid for doing this. Apart from that I am not even confident about this change… | |||||
If by delete you mean kill, I think you're right, it's probably simpler and safer, and this should happen rarely enough for this not to be a performance issue. dfaure: If by delete you mean kill, I think you're right, it's probably simpler and safer, and this… | |||||
148 | { | 148 | { | ||
149 | #ifdef Q_OS_UNIX | 149 | #ifdef Q_OS_UNIX | ||
150 | foreach (const QString &actId, m_TempAuth) { | 150 | foreach (const QString &actId, m_TempAuth) { | ||
151 | KAuth::Action action(actId); | 151 | KAuth::Action action(actId); | ||
152 | if (revoke) { | ||||
153 | action.revokeAuthorization(); | ||||
154 | } | ||||
152 | if (action.status() != KAuth::Action::AuthorizedStatus) { | 155 | if (action.status() != KAuth::Action::AuthorizedStatus) { | ||
153 | m_TempAuth.remove(actId); | 156 | m_TempAuth.remove(actId); | ||
154 | } | 157 | } | ||
155 | } | 158 | } | ||
156 | return !m_TempAuth.isEmpty(); | 159 | return !m_TempAuth.isEmpty(); | ||
157 | #else | 160 | #else | ||
158 | return false; | 161 | return false; | ||
159 | #endif | 162 | #endif | ||
▲ Show 20 Lines • Show All 383 Lines • ▼ Show 20 Line(s) | |||||
543 | { | 546 | { | ||
544 | send(MSG_NEED_SUBURL_DATA); | 547 | send(MSG_NEED_SUBURL_DATA); | ||
545 | } | 548 | } | ||
546 | 549 | | |||
547 | void SlaveBase::slaveStatus(const QString &host, bool connected) | 550 | void SlaveBase::slaveStatus(const QString &host, bool connected) | ||
548 | { | 551 | { | ||
549 | qint64 pid = getpid(); | 552 | qint64 pid = getpid(); | ||
550 | qint8 b = connected ? 1 : 0; | 553 | qint8 b = connected ? 1 : 0; | ||
551 | KIO_DATA << pid << mProtocol << host << b << d->hasTempAuth(); | 554 | KIO_DATA << pid << mProtocol << host << b << d->hasTempAuth(true); | ||
It turns out that indeed this method is only called when the slave is going to Idle, but, hmm, in this code nothing says so, it looks like a method that just sends current status, and that could be called at any moment... I would suggest to at least add a comment here, something like ... above the method call that you'll have to add to clear the auths, due to the previous comment ;) dfaure: It turns out that indeed this method is only called when the slave is going to Idle, but, hmm… | |||||
552 | if (d->onHold) { | 555 | if (d->onHold) { | ||
553 | stream << d->onHoldUrl; | 556 | stream << d->onHoldUrl; | ||
554 | } | 557 | } | ||
555 | send(MSG_SLAVE_STATUS, data); | 558 | send(MSG_SLAVE_STATUS, data); | ||
556 | } | 559 | } | ||
557 | 560 | | |||
558 | void SlaveBase::canResume() | 561 | void SlaveBase::canResume() | ||
559 | { | 562 | { | ||
▲ Show 20 Lines • Show All 966 Lines • Show Last 20 Lines |
That looks like horrible API ;)
A getter that optionally makes changes, based on a boolean... urgh ;)
"Duplicating" a for loop isn't really duplication, go for a different method.