[Merge] lp:~phablet-team/ubuntu-system-settings/path-fixes into lp:ubuntu-system-settings

[Lukáš Tinkl]

Review: Needs Fixing

Left a few inline comments, sorry :)

Diff comments:

> 
> === modified file 'plugins/background/background.cpp'
> --- plugins/background/background.cpp	2014-07-08 09:18:09 +0000
> +++ plugins/background/background.cpp	2016-09-22 14:22:27 +0000
> @@ -50,6 +50,18 @@
>      return QString();
>  }
>  
> +QString Background::defaultPhoneBackgroundFile()

getter -> const qualifier

> +{
> +    auto file = "/usr/share/unity8/graphics/phone_background.jpg";
> +    return qgetenv("SNAP").append(file);
> +}
> +
> +QString Background::defaultTabletBackgroundFile()

getter -> const qualifier

> +{
> +    auto file = "/usr/share/unity8/graphics/tablet_background.jpg";
> +    return qgetenv("SNAP").append(file);
> +}
> +
>  void Background::setBackgroundFile(QUrl backgroundFile)

const QUrl &backgroundFile

You don't want to pass the heavy QUrl by value

>  {
>      if (!backgroundFile.isLocalFile())
> 
> === modified file 'src/plugin-manager.cpp'
> --- src/plugin-manager.cpp	2014-08-28 07:22:28 +0000
> +++ src/plugin-manager.cpp	2016-09-22 14:22:27 +0000
> @@ -80,7 +81,17 @@
>  {
>      Q_Q(PluginManager);
>      clear();
> -    QDir path(baseDir, "*.settings");
> +
> +    /* Create a list of search paths (e.g. /usr/share, /usr/local/share) and
> +     * append the baseDir. The reason for not using locateAll is that locateAll
> +     * does not seem to work with a dir and file pattern, which means it will
> +     * look for all .settings files, not just those in well-known locations. */
> +    QStandardPaths::StandardLocation loc = QStandardPaths::GenericDataLocation;
> +    QFileInfoList searchPaths;
> +    Q_FOREACH(const QString &path, QStandardPaths::standardLocations(loc)) {
> +        QDir dir(QString("%1/%2").arg(path).arg(baseDir), "*.settings");

more efficient: QStringLiteral("%1/%2").arg(path, baseDir)

> +        searchPaths.append(dir.entryInfoList());
> +    }
>  
>      /* Use an environment variable USS_SHOW_ALL_UI to show unfinished / beta /
>       * deferred components or panels */
> @@ -95,7 +106,7 @@
>      if (ctx)
>          ctx->engine()->rootContext()->setContextProperty("showAllUI", showAll);
>  
> -    Q_FOREACH(QFileInfo fileInfo, path.entryInfoList()) {
> +    Q_FOREACH(QFileInfo fileInfo, searchPaths) {

const QFileInfo &fileInfo

You definitely don't want to detach here while iterating, QFileInfo is costly

>          Plugin *plugin = new Plugin(fileInfo);
>          QQmlEngine::setContextForObject(plugin, ctx);
>          QMap<QString, Plugin*> &pluginList = m_plugins[plugin->category()];


-- 
https://code.launchpad.net/~phablet-team/ubuntu-system-settings/path-fixes/+merge/306115
Your team Ubuntu Phablet Team is subscribed to branch lp:~phablet-team/ubuntu-system-settings/path-fixes.