From kde-core-devel Wed Sep 21 13:31:10 2011 From: "Christoph Feck" Date: Wed, 21 Sep 2011 13:31:10 +0000 To: kde-core-devel Subject: Re: Review Request: new kded daemon to check .thumbnail directory Message-Id: <20110921133110.3324.57509 () vidsolbach ! de> X-MARC-Message: https://marc.info/?l=kde-core-devel&m=131661189921112 MIME-Version: 1 Content-Type: multipart/mixed; boundary="--===============7654196300193744800==" --===============7654196300193744800== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://git.reviewboard.kde.org/r/102083/#review6698 ----------------------------------------------------------- This review may be incomplete, I mainly did it so that you get more feedbac= k, ideally from others, too. directoryusagenotifier/cleanupdirectory.h This should be the only constructor. = If someone really needs to pass a QString, he can use CleanUpDirectory(= KUrl(string)). This stresses the fact that he actually should have used KUr= l in the first place. directoryusagenotifier/cleanupdirectory.h To be consistent, add m_ directoryusagenotifier/cleanupdirectory.cpp This "if" has Space inside the braces. Since this is all new code, the = coding style should be consistent. Since there are other style inconsistenc= ies, I suggest to run it through astyle-kdelibs, which can be found in kdes= dk/scripts. directoryusagenotifier/directoryusagenotifier.h #include etc. directoryusagenotifier/directoryusagenotifier.cpp My "kdebugdialog" says that 7020 is used for "kded4", you either need a= different ID, or switch to dynamic IDs. directoryusagenotifier/directoryusagenotifier.desktop It's nice that you want to help translating, but those translations are= deleted on next scripty run :) Remove translations. directoryusagenotifier/directoryusagenotifier.kcfg If the default is to check every 2 minutes, then this is way too often.= I cannot recommend a good default, but having my system cause disk content= ion by scanning for directory space usage every two minutes is inacceptable. directoryusagenotifier/directoryusagenotifier_prefs_base.ui Is there a way to get the localized/customized suffix? I have my KDE se= t to "MB". directoryusagenotifier/directoryusagenotifier_prefs_base.ui Please don't use HTML strings here. It specifies a fixed font and is ho= rrible for translators. It is okey to add simple formatting using
= etc. - Christoph On Sept. 2, 2011, 3:35 p.m., Jaime Torres Amate wrote: > = > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://git.reviewboard.kde.org/r/102083/ > ----------------------------------------------------------- > = > (Updated Sept. 2, 2011, 3:35 p.m.) > = > = > Review request for kdelibs. > = > = > Summary > ------- > = > This is not yet complete, it is missing the code to delete the files, tra= iling spaces and comments in spanish. (coming in next patch version) > = > There are also some things I'm not sure how should be done... > The translations in .notifyrc and .desktop files, should be removed or ju= st keep the lines with an empty traslation? > Classes names, method names and variable names are OK? > = > = > This addresses bug 79943. > http://bugs.kde.org/show_bug.cgi?id=3D79943 > = > = > Diffs > ----- > = > CMakeLists.txt baf36cc = > directoryusagenotifier/CMakeLists.txt PRE-CREATION = > directoryusagenotifier/COPYING PRE-CREATION = > directoryusagenotifier/Messages.sh PRE-CREATION = > directoryusagenotifier/README PRE-CREATION = > directoryusagenotifier/cleanupdirectory.h PRE-CREATION = > directoryusagenotifier/cleanupdirectory.cpp PRE-CREATION = > directoryusagenotifier/directoryusagenotifier.h PRE-CREATION = > directoryusagenotifier/directoryusagenotifier.cpp PRE-CREATION = > directoryusagenotifier/directoryusagenotifier.desktop PRE-CREATION = > directoryusagenotifier/directoryusagenotifier.kcfg PRE-CREATION = > directoryusagenotifier/directoryusagenotifier.notifyrc PRE-CREATION = > directoryusagenotifier/directoryusagenotifier_prefs_base.ui PRE-CREATIO= N = > directoryusagenotifier/module.h PRE-CREATION = > directoryusagenotifier/module.cpp PRE-CREATION = > directoryusagenotifier/settings.kcfgc PRE-CREATION = > directoryusagenotifier/tests/CMakeLists.txt PRE-CREATION = > directoryusagenotifier/tests/cleanupunittest.cpp PRE-CREATION = > = > Diff: http://git.reviewboard.kde.org/r/102083/diff > = > = > Testing > ------- > = > It works as expected. > = > = > Thanks, > = > Jaime Torres > = > --===============7654196300193744800== Content-Type: text/html; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable
This is an automatically generated e-mail. To reply, visit: http://git.revie= wboard.kde.org/r/102083/ 

This revie=
w may be incomplete, I mainly did it so that you get more feedback, ideally=
 from others, too.

= =
directoryusagenotifier/cleanupdirectory.h (Diff revision 3) 

   
public:


  

 

  

    
    

    41
    
   explicit CleanUpDirectory(const KUrl &<=
span class=3D"n">directory, const qint64 maximumSp=
aceUsed);

  


 





This should be the only constructor.

If someone really needs to pass a QString, he can use CleanUpDirectory(KUrl=
(string)). This stresses the fact that he actually should have used KUrl in=
 the first place.

















 =

 =




 

 
  

   
    directoryusagenotifier/cleanupdirectory.h
    

     (Diff revision 3)

    
   
  

 

 
  


   

   
public:


  

 

  

    
    

    56
    
  KFileItemList entriesToSort;

  


 





To be consistent, add m_

















 =

 =




 

 
  

   
    directoryusagenotifier/cleanupdirectory.cpp
    

     (Diff revision 3)

    
   
  

 

 
  


   

   
void CleanUpDirectory::deleteLeastUsed()


  

 

  

    
    

    101
    
    if ( m_spaceCurrentlyUsed - spaceCleaned > m_maximumSpaceUsed ) =
{

  


 





This "if" has Space inside the braces. Since this is all n=
ew code, the coding style should be consistent. Since there are other style=
 inconsistencies, I suggest to run it through astyle-kdelibs, which can be =
found in kdesdk/scripts.

















 =

 =




 

 
  

   
    directoryusagenotifier/directoryusagenotifier.h
    

     (Diff revision 3)

    
   
  

 

 
  


   


  

 

  

    
    

    29
    
#include <KJob>

  


 





#include <KDE/KJob> etc.

















 =

 =




 

 
  

   
    directoryusagenotifier/directoryusagenotifier.cpp
    

     (Diff revision 3)

    
   
  

 

 
  


   

   
DirectoryUsageNotifier::DirectoryUsageNotifier( QObject* parent )


  

 

  

    
    

    74
    
    kDebug(7020) << "constructed with =3D=
 " << dirs[0]=
.mPath << "to check every "

  


 





My "kdebugdialog" says that 7020 is used for "kded4&q=
uot;, you either need a different ID, or switch to dynamic IDs.

















 =

 =




 

 
  

   
    directoryusagenotifier/directoryusagenotifier.desktop
    

     (Diff revision 3)

    
   
  

 

 
  


   


  

 

  

    
    

    4
    
Name[es]=3DNotificador de excesivo tama=C3=B1o=
 de directorio

  


 





It's nice that you want to help translating, but those translati=
ons are deleted on next scripty run :) Remove translations.

















 =

 =




 

 
  

   
    directoryusagenotifier/directoryusagenotifier.kcfg
    

     (Diff revision 3)

    
   
  

 

 
  


   


  

 

  

    
    

    28
    
            <default>2</default>

  


 





If the default is to check every 2 minutes, then this is way too oft=
en. I cannot recommend a good default, but having my system cause disk cont=
ention by scanning for directory space usage every two minutes is inaccepta=
ble.

















 =

 =




 

 
  

   
    directoryusagenotifier/directoryusagenotifier_prefs_base.ui
    

     (Diff revision 3)

    
   
  

 

 
  


   


  

 

  

    
    

    37
    
      <string> MiB</string>
<=
/td>
  


 





Is there a way to get the localized/customized suffix? I have my KDE=
 set to "MB".

















 =

 =




 

 
  

   
    directoryusagenotifier/directoryusagenotifier_prefs_base.ui
    

     (Diff revision 3)

    
   
  

 

 
  


   


  

 

  

    
    

    57
    
      <string>&lt;!DOCTYPE HTML PUBL=
IC &quot;-//W3C//DTD HTML 4.0//EN&quot; &quot;http://www.w3.org=
/TR/REC-html40/strict.dtd&quot;&gt;

  


 





Please don't use HTML strings here. It specifies a fixed font an=
d is horrible for translators. It is okey to add simple formatting using &l=
t;qt> <br> <b> etc.









- Christoph






On September 2nd, 2011, 3:35 p.m., Jaime Torres Amate wrote:









 

  


Review request for kdelibs.


By Jaime Torres Amate.




Updated Sept. 2, 2011, 3:35 p.m.






Descripti=
on 



 

  
   
This is not yet complete, it is missing the code to delete t=
he files, trailing spaces and comments in spanish. (coming in next patch ve=
rsion)

There are also some things I'm not sure how should be done...
The translations in .notifyrc and .desktop files, should be removed or just=
 keep the lines with an empty traslation?
Classes names, method names and variable names are OK?


  		
 






Testing <=
/h1>


 

  
   
It works as expected.


  		
 








 Bugs: 


 79943







Diffs=
 


    

 
  • CMakeLists.txt (baf36cc)
    • 

 
  • directoryusagenotifier/CMakeLists.txt (PRE=
-CREATION)
    • 

 
  • directoryusagenotifier/COPYING (PRE-CREATI=
ON)
    • 

 
  • directoryusagenotifier/Messages.sh (PRE-CR=
EATION)
    • 

 
  • directoryusagenotifier/README (PRE-CREATIO=
N)
    • 

 
  • directoryusagenotifier/cleanupdirectory.h =
(PRE-CREATION)
    • 

 
  • directoryusagenotifier/cleanupdirectory.cpp (PRE-CREATION)
    • 

 
  • directoryusagenotifier/directoryusagenotifier.cpp (PRE-CREATION)
    • 

 
  • directoryusagenotifier/directoryusagenotifier.desktop (PRE-CREATION)
    • 

 
  • directoryusagenotifier/directoryusagenotifier.kcfg (PRE-CREATION)
    • 

 
  • directoryusagenotifier/directoryusagenotifier.notifyrc (PRE-CREATION)
    • 

 
  • directoryusagenotifier/directoryusagenotifier_prefs_base.ui (PRE-CREATION)
    • 

 
  • directoryusagenotifier/module.h (PRE-CREAT=
ION)
    • 

 
  • directoryusagenotifier/module.cpp (PRE-CRE=
ATION)
    • 

 
  • directoryusagenotifier/settings.kcfgc (PRE=
-CREATION)
    • 

 
  • directoryusagenotifier/tests/CMakeLists.txt (PRE-CREATION)
    • 





View Diff





  		
 











  

 



--===============7654196300193744800==--