Code review comment for lp:~carlos-mazieri/ubuntu-filemanager-app/app-devel-pre4

1)

Non-virtual destructors on classes that either inherit or are inherited other classes with virtual functions, these should be virtual destructors:
~Location()
~DiskLocation()
~LocationsFactory()

Especially important are the Location ones and the classes deriving from it. A basic rule-of-thumb is to have a virtual destructor on any class that has virtual functions or that derives from a class that has virtual destructors. Deviating from this rule-of-thumb is sometimes acceptable but there should be heavy reasons for it, and some comment for why it is so would be appreciated. Right now I do not see anything in the code that would warrant them not-being virtual destructors.

It'd be good to check the code on the whole that there's not more than what I noticed in this code-review.

2)

There is const_cast<...> calls. It's a red flag, casting should only be used if definitely needed. Either document why it's needed or try to refactor the code to get rid of it. For example:
const Location * LocationsFactory::setNewPath(const QString& uPath)

Why does it need to return const pointer as it's used in such a way that it's modified? Why not have it return non-const pointer and get rid of the const_casts.

3)

in someplaces when checking pointer there is:
if (m_somePointer == 0)
and other times just:
if (m_somePointer)

Either is fine IMO, but it'd be good to keep the code-base consistent. So use one or the other.

4)

This perplexes me:

void Location::setFromInfoItem(const DirItemInfo *itemInfo)
917 +{
918 + if (m_info)
919 + {
920 + delete m_info;
921 + }
922 + m_info = const_cast<DirItemInfo*> (itemInfo);
923 +}

the name of the function, setFromInfoItem, along with the const parameter to me gives an impression that it will make a copy for the class to use. Instead it does a cast and uses the pointer directly. Perhaps do not use parameter as "const" and rename the function to setInfoItem(DireItemInfo *itemInfo) ? Here I may not understand some details, so feel free to correct me.

5)

Typos:

LocationsFactory::stringAfterSlahes() -> LocationsFactory::stringAfterSlashes()
DiskLocation::stoptExternalFsWatcher() -> DiskLocation::stopExternalFsWatcher()