'Re: Request for review'

[prev in list] [next in list] [prev in thread] [next in thread] 

List:       kde-devel
Subject:    Re: Request for review
From:       Conny_Marco_Menebröcker <admin () menebroecker-web ! de>
Date:       2014-02-17 6:50:44
Message-ID: 5301B144.6070907 () menebroecker-web ! de
[Download RAW message or body]

[Attachment #2 (multipart/alternative)]

Hi Kevin,

thank you very much for your comments.
I revised the code regarding your comments and checked in some minutes ago.

Best regards,

Conny

Am 16.02.2014 14:45, schrieb Kevin Krammer:
> Hi,
>
> On Saturday, 2014-02-15, 19:44:46, Conny Marco Menebröcker wrote:
>> Hello all,
>>
>>
>> I have written my first plasmoid. I called it PlasmaTaskViewer, because
>> it views the todos of a calendar file.
>> In my case it shows the entries of my iPhone reminder app.
>>
>> I would be happy if someone reviews my code. I am especially interested
>> in comments about the use of kcalcore and the data engine. But every
>> other comment is welcome, too.
>>
>> https://sourceforge.net/projects/plasmataskviewer/
> Looks pretty good.
>
> One thing that is improvable is the handling of received data. Instead of
> putting that into a stringlist I would suggest doing something like
>
> QHash<long, QByteArra> m_ical_data;
>
> The slot is then just
>
> void TaskViewerContainer::dataSlot(long id, const QByteArray &data)
> {
>      m_ical_data[id].append(data);
> }
>
> For the member holding the KJob pointer I would suggest
> QPointer<Kjob> m_job;
>
> It has a pointer operator so code would look "nicer"
> if (m_job) m_job->kill();
>
> Btw, I don't think you need to kill() the job when it is done :)
>
> One other thing you could try (don't know if this is equivalent) is to have
> just one MemoryCalendar instance and close() it at the end of the loop.
>
> Since you don't need the completed information as a string for display, it
> might be better to just have it as a list of bool.
>
> Alternatively you could consider "pairing" the related values:
>
> QVariantList list;
> QVariantHash entry;
> entry["summary"] = oneTodo->summary();
> entry["completed"] = oneTodo->isCompleted();
> list << entry;
>
> in the applet that would the be something like
>
> QVariantHash entry = todo_list[i].toHash();
> addScrollerItem(entry["summary"], entry["completed"]);
>
> Cheers,
> Kevin
>
>
>>> Visit http://mail.kde.org/mailman/listinfo/kde-devel#unsub to unsubscribe <<

[Attachment #5 (text/html)]

<html>
  <head>
    <meta content="text/html; charset=ISO-8859-1"
      http-equiv="Content-Type">
  </head>
  <body bgcolor="#FFFFFF" text="#000000">
    Hi Kevin,<br>
    <br>
    thank you very much for your comments.<br>
    I revised the code regarding your comments and checked in some
    minutes ago.<br>
    <br>
    Best regards,<br>
    <br>
    Conny<br>
    <br>
    <br>
    <div class="moz-cite-prefix">Am 16.02.2014 14:45, schrieb Kevin
      Krammer:<br>
    </div>
    <blockquote cite="mid:11707070.YeyHehlPt7@persephone" type="cite">
      <pre wrap="">Hi,

On Saturday, 2014-02-15, 19:44:46, Conny Marco Menebr&ouml;cker wrote:
</pre>
      <blockquote type="cite">
        <pre wrap="">Hello all,

I have written my first plasmoid. I called it PlasmaTaskViewer, because
it views the todos of a calendar file.
In my case it shows the entries of my iPhone reminder app.

I would be happy if someone reviews my code. I am especially interested
in comments about the use of kcalcore and the data engine. But every
other comment is welcome, too.

<a class="moz-txt-link-freetext" \
href="https://sourceforge.net/projects/plasmataskviewer/">https://sourceforge.net/projects/plasmataskviewer/</a>
 </pre>
      </blockquote>
      <pre wrap="">
Looks pretty good.

One thing that is improvable is the handling of received data. Instead of 
putting that into a stringlist I would suggest doing something like

QHash&lt;long, QByteArra&gt; m_ical_data;

The slot is then just

void TaskViewerContainer::dataSlot(long id, const QByteArray &amp;data)
{
    m_ical_data[id].append(data);
}

For the member holding the KJob pointer I would suggest
QPointer&lt;Kjob&gt; m_job;

It has a pointer operator so code would look "nicer"
if (m_job) m_job-&gt;kill();

Btw, I don't think you need to kill() the job when it is done :)

One other thing you could try (don't know if this is equivalent) is to have 
just one MemoryCalendar instance and close() it at the end of the loop.

Since you don't need the completed information as a string for display, it 
might be better to just have it as a list of bool.

Alternatively you could consider "pairing" the related values:

QVariantList list;
QVariantHash entry;
entry["summary"] = oneTodo-&gt;summary();
entry["completed"] = oneTodo-&gt;isCompleted();
list &lt;&lt; entry;

in the applet that would the be something like

QVariantHash entry = todo_list[i].toHash();
addScrollerItem(entry["summary"], entry["completed"]);

Cheers,
Kevin
</pre>
      <br>
      <fieldset class="mimeAttachmentHeader"></fieldset>
      <br>
      <pre wrap="">
</pre>
      <blockquote type="cite">
        <blockquote type="cite">
          <pre wrap="">Visit <a class="moz-txt-link-freetext" \
href="http://mail.kde.org/mailman/listinfo/kde-devel#unsub">http://mail.kde.org/mailman/listinfo/kde-devel#unsub</a> \
to unsubscribe &lt;&lt; </pre>
        </blockquote>
      </blockquote>
    </blockquote>
    <br>
  </body>
</html>

>> Visit http://mail.kde.org/mailman/listinfo/kde-devel#unsub to unsubscribe <<

[prev in list] [next in list] [prev in thread] [next in thread] 