Review: Needs Fixing
Several inlined comments. The one about removing all keys but the new ones
introduced in this change is the one that needs attention, others are more
minor.
Diff comments:
> === modified file 'src/plugin/placesmodel/placesmodel.cpp'
> --- src/plugin/placesmodel/placesmodel.cpp 2015-02-11 19:22:08 +0000
> +++ src/plugin/placesmodel/placesmodel.cpp 2015-09-07 21:27:29 +0000
> @@ -27,12 +28,14 @@
>
> PlacesModel::PlacesModel(QObject *parent) :
> QAbstractListModel(parent)
> + , m_userSavedLocationsName("userSavedLocations")
There's no reason to have userSavedLocationsName nor userRemovedLocationsName
as object's instance variables. They can not be set per instance nor they can
be modified after object creation, nor is there any reason why the should be.
Perhaps better to make them constants like:
namespace {
const char *userSavedLocationsName = "userSavedLocationsName";
...
}
or something like that
> + , m_userRemovedLocationsName("userRemovedLocations")
> + , m_going_to_rescanMtab(false)
> {
> m_userMountLocation = "/media/" + qgetenv("USER");
> // For example /run/user/1000
> m_runtimeLocations =
> QStandardPaths::standardLocations(QStandardPaths::RuntimeLocation);
>
> - QStringList defaultLocations;
> // Set the storage location to a path that works well
> // with app isolation
> QString settingsLocation =
> @@ -40,29 +43,43 @@
> + "/" + QCoreApplication::applicationName() + "/" +
> "places.conf";
> m_settings = new QSettings(settingsLocation, QSettings::IniFormat, this);
>
> + m_userSavedLocations =
> m_settings->value(m_userSavedLocationsName).toStringList();
> + m_userRemovedLocations =
> m_settings->value(m_userRemovedLocationsName).toStringList();
> +
> + //remove old key "storedLocations" or others no longer used
> + QStringList settingsKeys = m_settings->allKeys();
> + foreach (const QString& key, settingsKeys) {
> + if (key != m_userSavedLocationsName && key !=
> m_userRemovedLocationsName) {
This is error prone. No one will remember this piece of code when adding new
key to the settings. Instead it should only remove "storedLocations" if that
is the deprecated piece of code. Preferrably a TODO to remove the block all
together at some reasonable point at time (one year from now or whatever?)
> + m_settings->remove(key);
> + }
> + }
> +
> // Prepopulate the model with the user locations
> // for the first time it's used
> - defaultLocations.append(locationHome());
> - defaultLocations.append(locationDocuments());
> - defaultLocations.append(locationDownloads());
> - defaultLocations.append(locationMusic());
> - defaultLocations.append(locationPictures());
> - defaultLocations.append(locationVideos());
> - // Add root also
> - defaultLocations.append("/");
> -
> - if (!m_settings->contains("storedLocations")) {
> - m_locations.append(defaultLocations);
> - } else {
> - m_locations = m_settings->value("storedLocations").toStringList();
> + addDefaultLocation(locationHome());
> + addDefaultLocation(locationDocuments());
> + addDefaultLocation(locationDownloads());
> + addDefaultLocation(locationMusic());
> + addDefaultLocation(locationPictures());
> + addDefaultLocation(locationVideos());
> +
> + //Network locations
> + addDefaultLocation(locationSamba());
> +
> + //mounted locations
> + addDefaultLocation("/");
> + initNewUserMountsWatcher();
> + rescanMtab();
> +
> + //other user saved locations
> + foreach (const QString& userLocation, m_userSavedLocations) {
> + addLocationWithoutStoring(userLocation);
> }
> + m_settings->sync();
>
> foreach (const QString &location, m_locations) {
> qDebug() << "Location: " << location;
> }
> -
> - initNewUserMountsWatcher();
> - rescanMtab();
> }
>
> PlacesModel::~PlacesModel() {
> @@ -250,18 +305,35 @@
> endRemoveRows();
> }
>
> +
> void PlacesModel::addLocation(const QString &location)
> -{
> - if (addLocationWithoutStoring(location)) {
> - // Store the location permanently
> - m_settings->setValue("storedLocations", m_locations);
> +{
> + bool sync_seettings = false;
sync_seettings -> sync_settings
> + //verify it the user had deleted it before and now is inserting it again
> + int indexRemoved = m_userRemovedLocations.indexOf(location);
> + if(indexRemoved > -1) {
Spaces: if (indexRemoved > -1) {
> + m_userRemovedLocations.removeAt(indexRemoved);
> + m_settings->setValue(m_userRemovedLocationsName,
> m_userRemovedLocations);
> + sync_seettings = true;
> + }
> + if (addLocationWithoutStoring(location)) {
> + // Store the location permanently if it is not default location
> + if (!isDefaultLocation(location) &&
> !m_userSavedLocations.contains(location))
> + {
> + m_userSavedLocations.append(location);
> + m_settings->setValue(m_userSavedLocationsName,
> m_userSavedLocations);
> + sync_seettings = true;
> + }
> + }
> + if (sync_seettings) {
> + m_settings->sync();
> }
> }
>
> bool PlacesModel::addLocationWithoutStoring(const QString &location)
> {
> - // Do not allow for duplicates
> - if (!m_locations.contains(location)) {
> + // Do not allow for duplicates and look for removed locations from
> settings
> + if (!m_locations.contains(location) &&
> !m_userRemovedLocations.contains(location)) {
This changes the functionality of this function critically. So I would
recommend renaming the function to something that reflects what it does.
Perhaps addLocationIfNotRemovedWithoutStoring() or something like that (shorter
name would be better of course :)). The fact that explicitly removed locations
are not added is quite important change here.
> // Tell Qt that we're going to be changing the model
> // There's no tree-parent, first new item will be at
> // m_locations.count(), and the last one too
--
https://code.launchpad.net/~carlos-mazieri/ubuntu-filemanager-app/samba-ui-02/+merge/270335
Your team Ubuntu File Manager Developers is subscribed to branch
lp:ubuntu-filemanager-app.
--
Mailing list: https://launchpad.net/~ubuntu-touch-coreapps-reviewers
Post to : ubuntu-touch-coreapps-reviewers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~ubuntu-touch-coreapps-reviewers
More help : https://help.launchpad.net/ListHelp
