ubuntu-system-settings

Merge lp:~mterry/ubuntu-system-settings/split-bg into lp:ubuntu-system-settings

split-bg
Merge into trunk

Proposed by Michael Terry on 2014-06-04

Status:

Merged

Approved by:

Iain Lane on 2014-06-06

Approved revision:

729

Merged at revision:

726

Proposed branch:

lp:~mterry/ubuntu-system-settings/split-bg

Merge into:

lp:ubuntu-system-settings

Diff against target:

148 lines (+70/-7)

4 files modified

debian/ubuntu-system-settings.postrm (+9/-0)
plugins/background/background.cpp (+53/-5)
plugins/background/background.h (+4/-0)
plugins/background/utilities.js (+4/-2)

To merge this branch:

bzr merge lp:~mterry/ubuntu-system-settings/split-bg

Undecided

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
Iain Lane		2014-06-04	Approve on 2014-06-06
PS Jenkins bot	continuous-integration		Approve on 2014-06-05
Review via email: mp+222092@code.launchpad.net

Commit message

Use XDG_GREETER_DATA_DIR to store background files in a folder that the greeter can also read. This fixes the recent regression in not being able to choose a ContentHub file for the welcome screen.

Description of the change

Use XDG_GREETER_DATA_DIR to store background files in a folder that the greeter can also read.

The greeter is a separate user and often can't read files inside the user's $HOME. But ContentHub puts files there. So we need to move those files to the greeter location.

If the package is purged, we clean up after ourselves.

I didn't worry about migration. If we wanted to do it, I imagine on startup, we could move files from $HOME to the greeter data dir and also make sure that the AccountsService field is updated.

== Checklist ==

* Is your branch in sync with latest trunk (e.g. bzr pull lp:trunk -> no changes)
- Yes

* Did you build your software in a clean sbuild/pbuilder chroot or ppa?
- Yes

* Did you build your software in a clean sbuild/pbuilder armhf chroot or ppa?
- Yes

* Has your component "TestPlan” been executed successfully on emulator, N4?
- Yes

* Has a 5 minute exploratory testing run been executed on N4?
- Yes

* If you changed the packaging (debian), did you subscribe a core-dev to this MP?
- I am a core-dev

* If you changed the UI, did you subscribe the design-reviewers to this MP?
- NA

* What components might get impacted by your changes?
- None really, though we rely on behavior of LightDM

* Have you requested review by the teams of these owning components?
- NA

lp:~mterry/ubuntu-system-settings/split-bg updated on 2014-06-04

727. By Michael Terry on 2014-06-04: Remove unnecessary Qml exposure

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-06-04:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-system-settings-ci/856/rebuild

review: Approve (continuous-integration)

Revision history for this message

Iain Lane (laney) wrote on 2014-06-04:

Cheers. I tried a review using the fancy inline comments, hope it worked.

Somehow I've not heard of this XDG_GREETER_DATA_DIR before. Is it actually an XDG standardised variable? I don't find it mentioned on fdo.

review: Needs Fixing

Revision history for this message

Michael Terry (mterry) wrote on 2014-06-04:

> Somehow I've not heard of this XDG_GREETER_DATA_DIR before.
> Is it actually an XDG standardised variable? I don't find
> it mentioned on fdo.

It's new in trusty's LightDM. And no, it's not an actual XDG standardized variable. LightDM just has a habit of taking the XDG namespace in vain.

Will fix up branch and try responding inline.

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-06-04:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-system-settings-ci/857/rebuild

review: Approve (continuous-integration)

lp:~mterry/ubuntu-system-settings/split-bg updated on 2014-06-05

728. By Michael Terry on 2014-06-05: Support modifying gsettings value as well as the AccountsService one

Revision history for this message

Michael Terry (mterry) wrote on 2014-06-05:

OK, addressed comments and also use the same logic for gsettings chooser (both sets of files should live in same place for ease of browsing them and sharing/comparing values).

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-06-05:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-system-settings-ci/858/rebuild

review: Approve (continuous-integration)

Revision history for this message

Iain Lane (laney) wrote on 2014-06-05:

I think I'd rather if we only used this location when necessary, i.e. when setting the greeter background or when they are using the same one. You should be able to modify utilities.js to do that.

Revision history for this message

Michael Terry (mterry) wrote on 2014-06-05:

Well, I thought it was nicer if there was one directory where all images were kept, rather than trying to merge the greeter and ContentHub directory contents when showing past custom backgrounds to the user. It also made the urls more comparable.

But I'm not wedded to that. I can leave the images in ContentHub if you prefer for gsettings.

Revision history for this message

Iain Lane (laney) wrote on 2014-06-05:

On Thu, Jun 05, 2014 at 04:56:44PM -0000, Michael Terry wrote:
> Well, I thought it was nicer if there was one directory where all images were kept, rather than trying to merge the greeter and ContentHub directory contents when showing past custom backgrounds to the user. It also made the urls more comparable.
>
> But I'm not wedded to that. I can leave the images in ContentHub if you prefer for gsettings.

For me the idea is that things are only shared with the greeter if it's
necessary - that's putting things in AS only when the greeter needs
them, and now putting files in this directory too.

I did forget about the grid though, so there will be two custom
background directories which updateCustomBackgrounds() will need to take
care of. I think it'd be fine to point GSettings to an image in the
greeter directory if it's already there. Another thing is that we'd need
to do a move if the user selects 'same background' and we need to copy
the dash bg to the greeter. This one shouldn't be too hard.

Hrm. What about deleting from the greeter directory? AFAIK currently
it'll just grow with no easy way for the user to get rid of stuff in
there.

Do what you think is best for the dash background case. I won't insist
on it.

--
Iain Lane [ <email address hidden> ]
Debian Developer [ <email address hidden> ]
Ubuntu Developer [ <email address hidden> ]

Revision history for this message

Michael Terry (mterry) wrote on 2014-06-05:

> Hrm. What about deleting from the greeter directory? AFAIK currently
> it'll just grow with no easy way for the user to get rid of stuff in
> there.

What happens with the ContentHub files currently? I didn't see any cleanup code there.

Revision history for this message

Iain Lane (laney) wrote on 2014-06-05:

On Thu, Jun 05, 2014 at 05:41:07PM -0000, Michael Terry wrote:
> > Hrm. What about deleting from the greeter directory? AFAIK currently
> > it'll just grow with no easy way for the user to get rid of stuff in
> > there.
>
> What happens with the ContentHub files currently? I didn't see any cleanup code there.

Indeed not, maybe it's just something to remember to do when resetting
is implemented.

--
Iain Lane [ <email address hidden> ]
Debian Developer [ <email address hidden> ]
Ubuntu Developer [ <email address hidden> ]

lp:~mterry/ubuntu-system-settings/split-bg updated on 2014-06-05

729. By Michael Terry on 2014-06-05: Allow looking in both local ContentHub folder as well as greeter data folder, and de-duplicate as needed

Revision history for this message

Michael Terry (mterry) wrote on 2014-06-05:

OK, I've changed it so that if you are just setting gsettings individually, we don't need to promote it into a greeter file. And the picker grid scans both locations.

But if you are setting the welcome screen or both, we do promote.

We also de-duplicate in the case of setting gsettings individually to a file that you previously promoted to the greeter (so you don't have both copies showing in the picker grid).

Revision history for this message

PS Jenkins bot (ps-jenkins) wrote on 2014-06-05:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-system-settings-ci/862/rebuild

review: Approve (continuous-integration)

Revision history for this message

Iain Lane (laney) wrote on 2014-06-06:

Thanks for updating, looks good to me now. I'll just give it a quick check on the device before top approving.

review: Approve

Revision history for this message

Iain Lane (laney) wrote on 2014-06-06:

Looks good. I've got a followup because the same/different toggle is buggy, but that's not your fault. Thanks Mike.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk

Subscribers

People subscribed via source and target branches

to all changes:

Michael Terry

Ubuntu Touch System Settings

 === added file 'debian/ubuntu-system-settings.postrm'
 --- debian/ubuntu-system-settings.postrm	1970-01-01 00:00:00 +0000
 +++ debian/ubuntu-system-settings.postrm	2014-06-05 19:33:36 +0000
@@ -0,0 +1,9 @@
++#!/bin/sh
++set -e
++
++if [ "$1" = "purge" ] ; then
++    rm -rf /var/lib/lightdm-data/*/ubuntu-system-settings/
++fi
++
++#DEBHELPER#
++exit 0
 === modified file 'plugins/background/background.cpp'
 --- plugins/background/background.cpp	2014-02-18 14:18:27 +0000
 +++ plugins/background/background.cpp	2014-06-05 19:33:36 +0000
@@ -89,10 +89,10 @@
  void Background::updateCustomBackgrounds()
+ {
      m_customBackgrounds.clear();
--    QString customPath = QStandardPaths::writableLocation(QStandardPaths::DataLocation)+"/Pictures";
--    QDir dir(customPath);
--    dir.setFilter(QDir::Files | QDir::NoSymLinks);
--    QFileInfoList tmpList = dir.entryInfoList();
++    QFileInfoList tmpList;
++    tmpList << getCustomBackgroundFolder().entryInfoList(QDir::Files | QDir::NoSymLinks);
++    if (getCustomBackgroundFolder() != getContentHubFolder())
++        tmpList << getContentHubFolder().entryInfoList(QDir::Files | QDir::NoSymLinks);
      if (!tmpList.isEmpty())
+     {
          foreach (QFileInfo f, tmpList)
@@ -101,6 +101,54 @@
      Q_EMIT customBackgroundsChanged();
+ }
++QUrl Background::prepareBackgroundFile(const QUrl &url, bool shareWithGreeter)
++{
++    QUrl prepared = url;
++
++    if (getCustomBackgroundFolder() != getContentHubFolder() &&
++        url.path().startsWith(getContentHubFolder().path()))
++    {
++        QDir backgroundFolder = getCustomBackgroundFolder();
++        QUrl newPath = QUrl::fromLocalFile(backgroundFolder.path() + "/" + url.fileName());
++
++        if (QFile(newPath.path()).exists())
++        {
++            // The file already exists in the shared greeter data folder...
++            // Likely we just pulled the same file from ContentHub again.
++            // We don't want to show both versions in the picker grid, so just
++            // promote it to greeter location so we still just have one copy.
++            if (QFile(newPath.path()).remove())
++                shareWithGreeter = true;
++        }
++
++        // Move file from local ContentHub dump to shared greeter data folder
++        if (shareWithGreeter &&
++            QDir::root().mkpath(backgroundFolder.path()) &&
++            QFile::rename(url.path(), newPath.path()))
++        {
++            updateCustomBackgrounds();
++            prepared = newPath;
++        }
++    }
++
++    return prepared;
++}
++
++QDir Background::getCustomBackgroundFolder()
++{
++    // We want a location we can share with the greeter
++    QString dataDir(qgetenv("XDG_GREETER_DATA_DIR"));
++    if (dataDir.isEmpty())
++        return getContentHubFolder();
++    else
++        return dataDir + "/ubuntu-system-settings/Pictures";
++}
++
++QDir Background::getContentHubFolder()
++{
++    return QStandardPaths::writableLocation(QStandardPaths::DataLocation) + "/Pictures";
++}
++
  QStringList Background::ubuntuArt()
+ {
      return m_ubuntuArt;
@@ -132,7 +180,7 @@
      if (file.isEmpty() || file.isNull())
          return;
--    if (!file.contains(QStandardPaths::writableLocation(QStandardPaths::DataLocation)))
++    if (!file.contains(getCustomBackgroundFolder().path()) && !file.contains(getContentHubFolder().path()))
          return;
      QUrl fileUri(file);
 === modified file 'plugins/background/background.h'
 --- plugins/background/background.h	2014-02-18 14:18:27 +0000
 +++ plugins/background/background.h	2014-06-05 19:33:36 +0000
@@ -24,6 +24,7 @@
  #include "accountsservice.h"
  #include <QDBusInterface>
++#include <QDir>
  #include <QObject>
  #include <QProcess>
  #include <QUrl>
@@ -49,6 +50,7 @@
      ~Background();
      QString backgroundFile();
      void setBackgroundFile(QUrl backgroundFile);
++    Q_INVOKABLE QUrl prepareBackgroundFile(const QUrl &url, bool shareWithGreeter);
      Q_INVOKABLE bool fileExists(const QString &file);
      Q_INVOKABLE void rmFile(const QString &file);
      QStringList customBackgrounds();
@@ -70,6 +72,8 @@
      void updateUbuntuArt();
      QString m_backgroundFile;
      QString getBackgroundFile();
++    QDir getCustomBackgroundFolder();
++    QDir getContentHubFolder();
  };
  #endif // BACKGROUND_H
 === modified file 'plugins/background/utilities.js'
 --- plugins/background/utilities.js	2013-12-06 19:20:15 +0000
 +++ plugins/background/utilities.js	2014-06-05 19:33:36 +0000
@@ -34,14 +34,16 @@
+ }
  function updateWelcome(uri) {
--    backgroundPanel.backgroundFile = uri;
++    backgroundPanel.backgroundFile = backgroundPanel.prepareBackgroundFile(uri, true);
+ }
  function updateHome(uri) {
--    background.pictureUri = uri;
++    background.pictureUri = backgroundPanel.prepareBackgroundFile(uri, false);
+ }
  function updateBoth(uri) {
++    // multiple prepares on a uri is fine
++    uri = backgroundPanel.prepareBackgroundFile(uri, true);
      updateWelcome(uri);
      updateHome(uri);
+ }