----------------------------------------------------------- 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 > >
_______________________________________________ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
