On Tuesday 30 July 2013 22:58:42 Thomas Lübking wrote:
> Set #2 of comments:
>
> + QAction *actionComposeMail = m_window->findChild<QAction
> *>(QLatin1String("action_compose_mail")); + QAction
> *actionComposeDraft = m_window->findChild<QAction
> *>(QLatin1String("action_compose_draft")); ...
> + QAction *actionReplyPrivate = m_window->findChild<QAction
> *>(QLatin1String("action_reply_private")); + QAction
> *actionReplyAllButMe = m_window->findChild<QAction
> *>(QLatin1String("action_reply_all_but_me")); + QAction
> *actionReplyAll = m_window->findChild<QAction
> *>(QLatin1String("action_reply_all")); + QAction
> *actionReplyList = m_window->findChild<QAction
> *>(QLatin1String("action_reply_list"));
>
> This begs to silently fail some day -> make them private
> members of GUI::MainWindow and TrojitaPart a friend.
>
KDE XML menu file contains above object names. So if somebody
change them, it will break Kontact plugin (Kontact will not show
any Trojita menus/buttons). So here can be good test if trojita
code will still OK.
What other think? Use findChild with object name? Or add friend
and access to private members directly?
>
> + m_window->show();
> +
> + // Insert it to Kontact
> + setWidget(m_window);
>
> The order seems wrong, ie. flicker or at least overhead prone
> (you show, reparent auto-hides, then needs re-show)
>
This is already fixed in my branch. m_window->show() should not be
called there.
> +Comment=Kontact Trojita Plugin
> lacks acute accent.
>
Fixed.
> +class TrojitaPlugin : public KontactInterface::Plugin
> +{
> + Q_OBJECT
> +
> + public:
> + TrojitaPlugin(KontactInterface::Core *core, const
> QVariantList &); + ~TrojitaPlugin();
> +
> + int weight() const { return 190; }
>
> ounces? pounds? kilogram? ;-)
> maybe add a comment what a "weight" of 190 means here.
>
Function is virtual, comes from KontactInterface::Plugin. I
already moved it to cpp file. I will add comment what 190 means
here. KMail has 200 and lower value means that plugin icon in
contact will be upper. So Trojita have higher pririty then KMail.
> + Q_FOREACH(const KABC::Addressee &addr, list) {
> + Q_FOREACH(const QString &email, addr.emails()) {
> + if (!m_ignores.contains(email) &&
> (email.contains(m_input, Qt::CaseInsensitive) || +
> addr.realName().contains(m_input,
> Qt::CaseInsensitive))) { as you passes m_input into akonadi,
> do you really have to test the match here?
>
No, I will remove it.
> +void AkonadiAddressbook::openContactWindow(const QString
> &email, const QString &displayName) +{
> + // TODO
> +#if 0
>
> is this short-term todo or long term todo? (in latter case the
> feature should not be announced until) +void
I do not know what to do now. Plugin does not working on my
testing machine and I think this is not problem in my code but in
akonadi itself. Look at email with subject "Akonadi plugin". So I
cannot implement something which is not working...
> KDEAddressbookCompletionJob::doStart()
> +{
> + NameEmailList completion;
> +
> + const KABC::Addressee::List &list =
> m_ab->allAddressees();
>
> Just looked it up. this BUILDS the list on every call.
> Could it be necessary to off-thread this job (eg. for "bigger"
> addressboks)?
>
> Maybe findByEmail + findByName are faster?
>
Hm, I will at it. Maybe it will be faster.
Is there any preferred Qt way how to merge two QList
(KABC::Addressee::List) into one QList with removed duplicates?
>
> +void KDEAddressbookProcess::slotKaddressbookRunning()
> +{
> + ...
> + QString uid;
> + KABC::Addressee::List list;
> +
> + if (!m_displayName.isEmpty())
> + list = m_ab->findByName(m_displayName);
> + else if (!m_email.isEmpty())
> + list = m_ab->findByEmail(m_email);
> + else {
> + KABC::Addressee::List listNames =
> m_ab->findByName(m_displayName); + Q_FOREACH(const
> KABC::Addressee &addr, listNames) +
> Q_FOREACH(const QString &addrEmail, addr.emails()) +
> if (m_email == addrEmail)
> + list << addr;
> + }
>
> Does it make sense to build the list while waiting for dbus
> (or starting the process)? Just measure the time usually
> taken by the dbus response, starting the process and building
> this list (for a reasonable amount of accounts)
>
>
Yes, this make sense.
--
Pali Rohár
pali.ro...@gmail.com
signature.asc
Description: This is a digitally signed message part.
