Review Request 112980: Fix Bug 267171 - Version state in details-view with expandable folders not shown

[Frank Reininghaus]

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

Ship it!


Thanks, very nice patch! Works nicely for me :-)

The only minor thing that I noticed when I went through the code is that it might not be obvious what the return value of the function

int createItemStatesList(QMap<QString, QVector<ItemState> >& itemStates,
                             const int firstIndex = 0,
                             const int currentExpansionLevel = 0);

is, and what its parameters mean.

Moreover, the parameter "currentExpansionLevel" seems redundant to me because it's always equal to m_model->expandedParentsCount(firstIndex), right? One could calculate it inside the function by adding

const int currentExpansionLevel = m_model->expandedParentsCount(firstIndex);

inside the function. It means that expandedParentsCount(int) will be called once more for that index, but it makes the header file easier to read, which I find slightly preferable. It's probably a matter of taste though, so feel free to decide yourself what you think is better.

But a comment in the header file that says what the function does, and that its return value is the number of items that have been processed won't hurt. What do you think?




- Frank Reininghaus


On Sept. 28, 2013, 7:38 p.m., Emmanuel Pescosta wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/112980/
> -----------------------------------------------------------
> 
> (Updated Sept. 28, 2013, 7:38 p.m.)
> 
> 
> Review request for Dolphin.
> 
> 
> Description
> -------
> 
> Show the right version states for expanded items.
> 
> 
> This addresses bug 267171.
>     http://bugs.kde.org/show_bug.cgi?id=267171
> 
> 
> Diffs
> -----
> 
>   dolphin/src/views/versioncontrol/updateitemstatesthread.h a281697 
>   dolphin/src/views/versioncontrol/updateitemstatesthread.cpp fa005f8 
>   dolphin/src/views/versioncontrol/versioncontrolobserver.h 501af7d 
>   dolphin/src/views/versioncontrol/versioncontrolobserver.cpp 402a2de 
> 
> Diff: http://git.reviewboard.kde.org/r/112980/diff/
> 
> 
> Testing
> -------
> 
> Works
> 
> 
> Thanks,
> 
> Emmanuel Pescosta
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.kde.org/mailman/private/kfm-devel/attachments/20130929/86011e57/attachment.htm>