Review Request: Extend FolderView::configChanged() to reload configuration values

[Harald Sitter]

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


Some lines are a mystery to me.


/trunk/KDE/kdebase/apps/plasma/applets/folderview/folderview.cpp
<http://reviewboard.kde.org/r/5049/#comment7294>

    Mind that url is empty here, I hope it is intentional that you set an empty URL.



/trunk/KDE/kdebase/apps/plasma/applets/folderview/folderview.cpp
<http://reviewboard.kde.org/r/5049/#comment7290>

    This is duplicated with configAccepted!
    
    Due to what this variable actual represents I do not find it wise to have two occurances of the same list. Instead it should be turned into something static const or a macro if absolutely necessary.



/trunk/KDE/kdebase/apps/plasma/applets/folderview/folderview.cpp
<http://reviewboard.kde.org/r/5049/#comment7291>

    Please copy from m_filterType here, cg.readEntry() is *a lot* more expensive than an int copy.
    
    Also, unless my search is bogus, niether m_filterType nor filterType get changed, so what is the use of this var?



/trunk/KDE/kdebase/apps/plasma/applets/folderview/folderview.cpp
<http://reviewboard.kde.org/r/5049/#comment7292>

    ... on line 439 - what is the point of the comparision betwen m_url and url considering they are always the same (if the if condition containing 439 is true) or always different (if the condition is false m_url will be whatever was read and url empty).
    
    ... on line 463 - what is the point of this comparision between filterType and m_filterType despite none of them having a line that changes the value?
    
    Also, please fix the formatting here to include whitespaces before and after the comparision operator.



/trunk/KDE/kdebase/apps/plasma/applets/folderview/folderview.cpp
<http://reviewboard.kde.org/r/5049/#comment7295>

    This always sets m_url to empty since url is empty.



/trunk/KDE/kdebase/apps/plasma/applets/folderview/folderview.cpp
<http://reviewboard.kde.org/r/5049/#comment7296>

    This always writes an empty url since above you changed m_url to an empty url.



/trunk/KDE/kdebase/apps/plasma/applets/folderview/folderview.cpp
<http://reviewboard.kde.org/r/5049/#comment7293>

    Same problem as with the previous stuff ... considering m_filterType could not have changed in line 508 (since filterType had no chance to have changed up until then), what is the point of this write?


- Harald


On 2010-08-22 17:07:53, Rohan Garg wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/5049/
> -----------------------------------------------------------
> 
> (Updated 2010-08-22 17:07:53)
> 
> 
> Review request for Plasma and Fredrik Höglund.
> 
> 
> Summary
> -------
> 
> Extend FolderView::configChanged() such that all configuration options are read there on FolderView start. This will ensure that configuration changes made "behind its back" (e.g. by the Desktop Scripting) will take effect.
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdebase/apps/plasma/applets/folderview/folderview.cpp 1165986 
> 
> Diff: http://reviewboard.kde.org/r/5049/diff
> 
> 
> Testing
> -------
> 
> Testing done, seems to be OK at my end
> 
> 
> Thanks,
> 
> Rohan
> 
>

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