[Marble-devel] Marble FileStorageWatcher

[Torsten Rahn]

Hi Bastian,

Looks great in general (although I have mostly reviewed formal stuff and 
haven't reviewed too much of the actual implementation -- except for looking 
at it coarsely).

I like the amount and quality of your code documentation! :-)

Ok, I'm wearing my API hat / Coding guidelines hat now
(and do lots of nitpicking in some cases): 

> void startFileWatcher();
> void stopFileWatcher();

I guess you've swallowed a bit too much init.d there ;-)
We encourage to choose APIs that use Qt style conventions:

http://doc.trolltech.com/qq/qq13-apis.html

Your methods are named like starting and stopping a service. In Qt you usually 
just enable or disable features instead: So just having a single method would 
be a better solution: 

setFeature(bool)

First the API of the Marble Widget should avoid technical terms that focus on 
the implementation and should emphasize what the implementation accomplishes 
from the user's point of view (the filewatcher is just a technical 
implementation to solve a problem - namely taking control of the amount of map 
data that gets cached ). 

In this case I wonder even why one would want to have a method in the public 
API of MarbleWidget to steer a thread at all.

The default is "Unlimited" anyways, so the thread should be disabled by 
default.
If the setPersistentTileCacheLimit is set to a non-zero size the thread should 
get activated automatically. 
So Just turning the thread off if setPersistentTileCacheLimit is set to zero 
should be enough.

So I suggest to remove the start/stop-methods altogether from the public 
MarbleWidget API and just make setPersistentTileCacheLimit manage the thread 
starting and stopping instead.

> // Only remove 20 files without cheching

caching :-)

>   emit somethingChanged();

Could you be more specific what that "something" is? Please rename it taking 
into account

http://doc.trolltech.com/qq/qq13-apis.html

;-)

> #define FILE_DELETION_NUMBER 20

Please avoid #defines for everything that is not just for debug flags. This is 
not C :-) So please use const instead, see:

http://techbase.kde.org/Development/Tutorials/Common_Programming_Mistakes#Constant_data

> FileStorageWatcherThread::ensurePlanetSize

Another nitpick: Sounds a bit like you'd want to control the size of a planet. 
Maybe something like: FileStorageWatcherThread::ensureSizePerPlanet ?
(same for ensureThemeSize)

>	if ( subDirectory.toInt() < 5 ) {

No magic numbers please (even if you document all your code as well as you do)

>	QString tileDirectory = themeDirectory + "/" + subDirectory;
	QString tileDirectory = themeDirectory + '/' + subDirectory;
(Krazy will complain about this otherwise.


>	    if (   (    filePath.toLower().endsWith( ".jpg" ) 
>	             || filePath.toLower().endsWith( ".png" )
>	             || filePath.toLower().endsWith( ".gif" )
>	             || filePath.toLower().endsWith( ".svg" ) )

I'd just do toLower once instead -- especially in a loop :-)

>		&& ( info.lastModified().secsTo( QDateTime::currentDateTime() ) > 120 ) )

no magic numbers, please :-)

>    emit sigNewFile( bytes );
>    emit sigRemovedFile( bytes );
>    emit sigCleared();

Should be: 

emit fileCreated( bytes );
emit fileRemoved( bytes );
emit filesCleared();

Please don't prefix signal names with "sig" and likewise slot names not with 
"slot" (and not with "on" either).

Signals should always be named in the _past tense_ and should just describe 
the event that just happened: valueChanged( .... ).

Slots should describe whatever is being done in the slot-method and are always 
in _present tense_: e.g. setValue(...).

Personally I also always try to avoid "new" for function names as "new" is a 
C++ keyword.

> // Lives insinde the new Thread
nitpick: inside

>	    // We havent stopped because of to many files
s/to/too/

Another thing: It would be great if the spinbox in the settings dialog would 
show the text "Unlimited" instead of 0. :-) -- at least if it's not too much 
of a hazzle to implement.

Torsten



























On Thursday 12 March 2009 18:51:39 Bastian Holst wrote:
> Hello,
>
> I attached a patch adding support for Persistent Tile Cache limitation in
> Marble. It is mostly a watcher which gets signals for added and removed
> files. After the startup of Marble it checks the size of the disk cache
> manually and adds the files sizes of new files. If the cache becomes too
> large, it deletes files from the hard disk.
> I hope somebody takes a look at this :).
>
> Regards
> Bastian
>
> I wrote:
> > Hi,
> >
> > I work on a watcher for Marble which keeps track of the size of the
> > persistent/hard disk cache of Marble. I heard you are working on speeding
> > up downloads.
> > At the moment I have the following solution:
> > - I made a subclass of QThread watching the cache size.
> > - This Thread is run after the startup of marble, so it doesn't make it
> > slower. Then it measures the file size, this takes a lot of time (for my
> > first approach I didn't make a Thread, it took (for the first time) about
> > 10 s, that was to slow.
> > - Additionally I added slots (for adding [sending file name/(perhaps
> > better sending file size, file could be deleted again)] and removing
> > files [sending file size]). The corresponding signals are implemented as
> > signals of StoragePolicy and the implementations like FileStoragePolicy
> > have to send it.
> > - Deleting files is another story.
> >
> > Do you see any problem at this approach, especially making StoragePolicy
> > a QObject and adding signals.
> >
> > Bastian Holst
> > _______________________________________________
> > Marble-devel mailing list
> > Marble-devel at kde.org
> > https://mail.kde.org/mailman/listinfo/marble-devel