Review Request: Fix bug 276082 and refactor timeline in plasmate

Thu Jun 23 11:23:58 CEST 2011

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/101728/#review4088
-----------------------------------------------------------

thanks for the patch. i believe it needs some work before it can go in, however.

first off: where is the bug fix, precisely? the patch is large and the bug fix is evidently somewhere in there.

after that, there are some issues i have with the refactor related to stability, speed and code readability.

finally, at most trivially, there remain numerous whitespace issues. this file was already pretty bad, we don't need to make it even more inconsistent, and fully bringing it into line with kdelibs style would be nice.

as for devel policy, doing such things in branches is definitely preferred. due to the size of the plasmate project (small) we do allow/expect people to merge their own branches into master when they are Ready(tm).

savesystem/timeline.cpp
<http://git.reviewboard.kde.org/r/101728/#comment3339>

    not really your fault as this was already a bit of a mess, but this should be "kdelibs style", without spaces inside the parens and with braces (even though it's a one-liner):

    if (!createBranchItem()) {
        return;
    }

savesystem/timeline.cpp
<http://git.reviewboard.kde.org/r/101728/#comment3358>

    here is where the refactor isn't worth it imho: because of a desire to break code out into smaller blocks, it now allocates memory for the *entire* git history instead of processing the items one at a time. so memory usage will be related to the size of the git log, and running time is also affected by creating a list, then iterating the list and then iterating it again in qDeleteAll.

    much better would be to simply handle the items one at a time as they previously were.

savesystem/timeline.cpp
<http://git.reviewboard.kde.org/r/101728/#comment3341>

    is breaking out this method worth it? it's used in one place and only one place. worst of all, it doesn't do what the method name implies: it doesn't check if the dir is a git dir, but actually sets the state of m_gitRunner. if anything this should be setWorkingDir; of course, that already exists.

    i'd suggest folding this into setWorkingDir and having setWorkingDir return a bool

savesystem/timeline.cpp
<http://git.reviewboard.kde.org/r/101728/#comment3343>

    should be: QStringList TimeLine::listBranches() const

savesystem/timeline.cpp
<http://git.reviewboard.kde.org/r/101728/#comment3342>

    shold be: QString TimeLine::curentBranch() const

savesystem/timeline.cpp
<http://git.reviewboard.kde.org/r/101728/#comment3344>

    createBranchItem is ambiguous in name: what branch item does it create? it creates an item for the current branch, a piece of internal state.

    this is called only where the current branch is set, and that is precisely where this code should be called from. it makes zero sense to run this code otherwise, so having it as a separate method is not sensible.

    this definitely belongs in loadTimeLine and not as a separate method.

savesystem/timeline.cpp
<http://git.reviewboard.kde.org/r/101728/#comment3357>

    this is a memleak and, in the foreach below a crash, just waiting to happen. there is no reason, other than expecting to run out of stack?, that the gitCommitDAO should be new'd.

    (the crash in the foreach would happen if a (malformed) string was returned such as:

    Merge: foo

    this would cause a crash since the commit variable would be uninitialized. if keeping heap initialization, i'd recommend a check there for commit being valid, and initializing commit to 0 (or NULL) above:

    if (!commit) {
        continue;
    }
    )

savesystem/timeline.cpp
<http://git.reviewboard.kde.org/r/101728/#comment3346>

    foreach (commitLogLine, rawCommitLog) {

savesystem/timeline.cpp
<http://git.reviewboard.kde.org/r/101728/#comment3345>

    this isn't consistent, whitespace wise, with what was there before or with the coding style we tend to use here (kdelibs style).. if should be:

    if (commitLogLine.contains(rx)) {

savesystem/timeline.cpp
<http://git.reviewboard.kde.org/r/101728/#comment3348>

    please add {}s while your fixing things here :)

savesystem/timeline.cpp
<http://git.reviewboard.kde.org/r/101728/#comment3349>

    please add {}s while your fixing things here :)

savesystem/timeline.cpp
<http://git.reviewboard.kde.org/r/101728/#comment3350>

    why ignore all hidden files? are they generated by any part of plasmate? it's completely conceivable that a hidden file is a valid file in the package, afaict.

savesystem/timeline.cpp
<http://git.reviewboard.kde.org/r/101728/#comment3351>

    please add {}s while your fixing things here :)

savesystem/timeline.cpp
<http://git.reviewboard.kde.org/r/101728/#comment3352>

    please add {}s while your fixing things here :)

savesystem/timeline.cpp
<http://git.reviewboard.kde.org/r/101728/#comment3353>

    please add {}s while your fixing things here :)

savesystem/timeline.cpp
<http://git.reviewboard.kde.org/r/101728/#comment3354>

    please add {}s while your fixing things here :)

savesystem/timelineitem.h
<http://git.reviewboard.kde.org/r/101728/#comment3356>

    should be: const TimeLineItem::gitCommitDAO &commit

savesystem/timelineitem.cpp
<http://git.reviewboard.kde.org/r/101728/#comment3355>

    can just be removed if it is dead code.

- Aaron J.

On June 22, 2011, 7:37 p.m., Etienne Rebetez wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/101728/
> -----------------------------------------------------------
> 
> (Updated June 22, 2011, 7:37 p.m.)
> 
> 
> Review request for Plasma.
> 
> 
> Summary
> -------
> 
> Hi,
> 
> When trying out plasmate the last day, i was hit with the bug 276082. Since i didn't want that it happens to someone else, i made a fix for it.
> I the process i found that the loadTimeLine function was far to big. So i did also some refactoring. (this diff is kinda huge but i made the changes in a branch which i can merge to master)
> The timeliene and savepoint creation should now behave as expected.
> 
> Let me know what you think.
> Etienne
> 
> PS: How is the commit policy in plasmate? Could i just push directly to master in the future?
> 
> 
> Diffs
> -----
> 
>   mainwindow.cpp 8326a29 
>   savesystem/timeline.h 719fb73 
>   savesystem/timeline.cpp d258a1a 
>   savesystem/timelineitem.h 1e9699f 
>   savesystem/timelineitem.cpp d9b0a67 
> 
> Diff: http://git.reviewboard.kde.org/r/101728/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Etienne
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/plasma-devel/attachments/20110623/a41e1794/attachment-0001.htm 