Re: Review Request: Extend FolderView::configChanged() to reload configuration values
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5049/ --- (Updated 2010-09-15 17:24:39.880260) Review request for Plasma and Fredrik Höglund. Changes --- More fixes for configChanged as pointed out by notmart 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 (updated) - /trunk/KDE/kdebase/apps/plasma/applets/folderview/folderview.cpp 1173894 Diff: http://svn.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
Re: Review Request: Extend FolderView::configChanged() to reload configuration values
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5049/ --- (Updated 2010-09-15 17:37:23.970844) Review request for Plasma and Fredrik Höglund. Changes --- formatting++ 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 (updated) - /trunk/KDE/kdebase/apps/plasma/applets/folderview/folderview.cpp 1173894 Diff: http://svn.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
Re: Review Request: Extend FolderView::configChanged() to reload configuration values
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5049/ --- (Updated 2010-09-14 16:39:31.059811) Review request for Plasma and Fredrik Höglund. Changes --- After much poking around and thanks to notmart and aseigo, finally implemented it :D 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 (updated) - /trunk/KDE/kdebase/apps/plasma/applets/folderview/folderview.cpp 1173894 Diff: http://svn.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
Re: Review Request: Extend FolderView::configChanged() to reload configuration values
--- This is an automatically generated e-mail. To reply, visit: http://svn.reviewboard.kde.org/r/5049/ --- (Updated 2010-09-14 17:24:49.849536) Review request for Plasma and Fredrik Höglund. Changes --- More fixes for filters and MimeList 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 (updated) - /trunk/KDE/kdebase/apps/plasma/applets/folderview/folderview.cpp 1173894 Diff: http://svn.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
Re: Review Request: Extend FolderView::configChanged() to reload configuration values
--- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/5049/ --- (Updated 2010-08-22 17:05:40.407310) 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 --- Im looking for testers at the moment, but i know for a fact that after applying the patch that kdebase compiles.Will report back when i can get hold of testers and see if it actually works Thanks, Rohan ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Extend FolderView::configChanged() to reload configuration values
--- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/5049/ --- (Updated 2010-08-22 17:07:53.057046) Review request for Plasma and Fredrik Höglund. Changes --- Final patch to fix all the stuff i could find.Seems to be working after testing on my machine 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 (updated) - /trunk/KDE/kdebase/apps/plasma/applets/folderview/folderview.cpp 1165986 Diff: http://reviewboard.kde.org/r/5049/diff Testing (updated) --- 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
Re: Review Request: Extend FolderView::configChanged() to reload configuration values
--- 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
Review Request: Extend FolderView::configChanged() to reload configuration values
--- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/5049/ --- 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 1164320 Diff: http://reviewboard.kde.org/r/5049/diff Testing --- Im looking for testers at the moment, but i know for a fact that after applying the patch that kdebase compiles.Will report back when i can get hold of testers and see if it actually works Thanks, Rohan ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Extend FolderView::configChanged() to reload configuration values
--- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/5049/ --- (Updated 2010-08-16 18:13:06.548619) Review request for Plasma and Fredrik Höglund. Changes --- Define a few variables 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 (updated) - /trunk/KDE/kdebase/apps/plasma/applets/folderview/folderview.cpp 1164320 Diff: http://reviewboard.kde.org/r/5049/diff Testing --- Im looking for testers at the moment, but i know for a fact that after applying the patch that kdebase compiles.Will report back when i can get hold of testers and see if it actually works Thanks, Rohan ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Extend FolderView::configChanged() to reload configuration values
--- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/5049/ --- (Updated 2010-08-17 12:09:14.676873) Review request for Plasma and Fredrik Höglund. Changes --- Fix Double declarations, dont even ask :P 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 (updated) - /trunk/KDE/kdebase/apps/plasma/applets/folderview/folderview.cpp 1164662 Diff: http://reviewboard.kde.org/r/5049/diff Testing --- Im looking for testers at the moment, but i know for a fact that after applying the patch that kdebase compiles.Will report back when i can get hold of testers and see if it actually works Thanks, Rohan ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Extend FolderView::configChanged() to reload configuration values
--- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/5049/ --- (Updated 2010-08-17 12:12:32.350700) Review request for Plasma and Fredrik Höglund. Changes --- Remove whitespaces 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 (updated) - /trunk/KDE/kdebase/apps/plasma/applets/folderview/folderview.cpp 1164662 Diff: http://reviewboard.kde.org/r/5049/diff Testing --- Im looking for testers at the moment, but i know for a fact that after applying the patch that kdebase compiles.Will report back when i can get hold of testers and see if it actually works Thanks, Rohan ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
Re: Review Request: Extend FolderView::configChanged() to reload configuration values
--- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/5049/#review7089 --- Ship it! this patch was really needed to make folderview correctly controllable via javascript. to me is good, modulo a couple of style issues below. if it's ok from fredrik for me it can go in /trunk/KDE/kdebase/apps/plasma/applets/folderview/folderview.cpp http://reviewboard.kde.org/r/5049/#comment7209 remove trailing spaces /trunk/KDE/kdebase/apps/plasma/applets/folderview/folderview.cpp http://reviewboard.kde.org/r/5049/#comment7210 open curly brace on the previous line /trunk/KDE/kdebase/apps/plasma/applets/folderview/folderview.cpp http://reviewboard.kde.org/r/5049/#comment7211 here too - Marco On 2010-08-16 14:31:05, Rohan Garg wrote: --- This is an automatically generated e-mail. To reply, visit: http://reviewboard.kde.org/r/5049/ --- (Updated 2010-08-16 14:31:05) 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 1164320 Diff: http://reviewboard.kde.org/r/5049/diff Testing --- Im looking for testers at the moment, but i know for a fact that after applying the patch that kdebase compiles.Will report back when i can get hold of testers and see if it actually works Thanks, Rohan ___ Plasma-devel mailing list Plasma-devel@kde.org https://mail.kde.org/mailman/listinfo/plasma-devel
