Merge lp:~fboucault/unity-2d/windowimageprovider_remove_timestamp_hack into lp:unity-2d

[Florian Boucault]

[dash & spread] Do not use a timestamp to make sure the window screenshots are
refreshed but instead make use of the 'cache' property of the Image QML element
recently introduced in Qt Quick 1.1

Removed unused ScreenInfo::currentTime()

[Lohith D Shivamurthy (dyams)]

Two issues I noticed

1) Is it not a good time to remove this too?
<code >QString ScreenInfo::currentTime() { ... } </code>

2) The comment needs to be fixed. windowimageprovider.cpp line#104
Current : /* After doing this, split the rest of the id on the character "|". The first
            part is the window ID of the decorations, the latter of the actual content. */
Updated : /* Split the id on the character "|". The first part is the window ID of the decorations and the latter is the actual content. */

Would it be possible to address these two issues? Please let me know.

review: Needs Fixing (code)

[lp:~fboucault/unity-2d/windowimageprovider_remove_timestamp_hack]

808. By Florian Boucault on 2011-12-07

Removed now unused ScreenInfo::currentTime()

809. By Florian Boucault on 2011-12-07

Fixed comment wording in WindowImageProvider.

[Florian Boucault (fboucault)]

Thanks a lot for this review Lohith. I believed I have now addressed the 2 issues.

[Gerry Boland (gerboland)]

Just a minor thing:

QString windowIds = id;

there no need for the copy now. Otherwise is good.

[Gerry Boland (gerboland)]

This is breaking the spread for me. Launch spread (works ok), exit it, then launch it again. It fails to work, errors like:

unity-2d-spread: [WARNING] file:///home/gerry/dev/windowimageprovider_remove_timestamp_hack//spread/Window.qml:64:5: QML Image: Failed to get image from provider: image://window/17184478|23068809

review: Needs Fixing

[Florian Boucault (fboucault)]

> Just a minor thing:
>
> QString windowIds = id;
>
> there no need for the copy now. Otherwise is good.

Yeah, I kept it in order to minimize the diff.

[Florian Boucault (fboucault)]

> This is breaking the spread for me. Launch spread (works ok), exit it, then
> launch it again. It fails to work, errors like:
>
> unity-2d-spread: [WARNING] file:///home/gerry/dev/windowimageprovider_remove_t
> imestamp_hack//spread/Window.qml:64:5: QML Image: Failed to get image from
> provider: image://window/17184478|23068809

It sounds like it is unrelated to that patch. I will do more stress testing.

[lp:~fboucault/unity-2d/windowimageprovider_remove_timestamp_hack]

810. By Florian Boucault on 2011-12-12

Merged lp:unity-2d

[Florian Boucault (fboucault)]

Gerry, I do not experience this issue unless:
- metacity's compositing is disabled
- a window is on another workspace and metacity's capture before unmap is disabled
- a window is minimized and metacity's capture before unmap is disabled

Whether or not the patch in this MR is applied you should experience this.

[Gerry Boland (gerboland)]

Yes on further investigation I believe the issue I saw is unrelated to this patch. Hence I approve this. Thank you!

review: Approve

=== modified file 'libunity-2d-private/src/screeninfo.cpp'
--- libunity-2d-private/src/screeninfo.cpp	2011-11-29 17:36:45 +0000
+++ libunity-2d-private/src/screeninfo.cpp	2011-12-12 10:03:27 +0000
@@ -62,14 +62,6 @@
     }
 }
 
-/* FIXME: This should be removed when we find a cleaner way to bypass the
-   QML Image cache. See SpreadWindow.qml and WindowImageProvider::requestImage
-   for details. */
-QString ScreenInfo::currentTime()
-{
-    return QString::number(time(NULL));
-}
-
 QRect ScreenInfo::availableGeometry() const
 {
     int screen = QX11Info::appScreen();
 
=== modified file 'libunity-2d-private/src/screeninfo.h'
--- libunity-2d-private/src/screeninfo.h	2011-11-29 17:36:45 +0000
+++ libunity-2d-private/src/screeninfo.h	2011-12-12 10:03:27 +0000
@@ -28,11 +28,6 @@
 public:
     static ScreenInfo* instance();
 
-    /* The following method doesn't strictly belong to a "screen" class
-       logically. It would be perhaps more appropriate to make an
-       "utility" class for it, but this will do for now. */
-    Q_INVOKABLE QString currentTime();
-
     /* Getters */
     WorkspacesInfo *workspaces() { return &m_workspacesInfo; }
     unsigned int activeWindow() const { return m_activeWindow; }
 
=== modified file 'libunity-2d-private/src/windowimageprovider.cpp'
--- libunity-2d-private/src/windowimageprovider.cpp	2011-08-09 14:32:13 +0000
+++ libunity-2d-private/src/windowimageprovider.cpp	2011-12-12 10:03:27 +0000
@@ -99,16 +99,11 @@
                                               QSize *size,
                                               const QSize &requestedSize)
 {
-    /* Throw away the part of the id after the @ (if any) since it's just a timestamp
-       added to force the QML image cache to request to this image provider
-       a new image instead of re-using the old. See SpreadWindow.qml for more
-       details on the problem. */
-    int atPos = id.indexOf('@');
-    QString windowIds = (atPos == -1) ? id : id.left(atPos);
+    QString windowIds = id;
 
-    /* After doing this, split the rest of the id on the character "|". The first
-       part is the window ID of the decorations, the latter of the actual content. */
-    atPos = windowIds.indexOf('|');
+    /* Split the id on the character "|". The first part is the window ID of
+       the decorations, the latter of the actual content. */
+    int atPos = windowIds.indexOf('|');
     Window frameId = ((atPos == -1) ? windowIds : windowIds.left(atPos)).toULong();
     Window contentId = ((atPos == -1) ? windowIds : windowIds.mid(atPos + 1)).toULong();
 
 
=== modified file 'places/dash.qml'
--- places/dash.qml	2011-11-29 12:13:27 +0000
+++ places/dash.qml	2011-12-12 10:03:27 +0000
@@ -179,22 +179,13 @@
                    is when declarativeView.active becomes true, so that a
                    screenshot of the windows behind the dash is taken at that
                    point.
-                   'source' also needs to change so that the screenshot is
-                   re-taken as opposed to pulled from QML's image cache.
-                   This workarounds the fact that the image cache cannot be
-                   disabled. A new API to deal with this was introduced in Qt Quick 1.1.
-
                    See http://doc.qt.nokia.com/4.7-snapshot/qml-image.html#cache-prop
                 */
-                property variant timeAtActivation
-                Connections {
-                    target: declarativeView
-                    onActiveChanged: blurredBackground.timeAtActivation = screen.currentTime()
-                }
 
                 /* Use an image of the root window which essentially is a
                    capture of the entire screen */
-                source: declarativeView.active ? "image://window/root@" + blurredBackground.timeAtActivation : ""
+                source: declarativeView.active ? "image://window/root" : ""
+                cache: false
 
                 fillMode: Image.PreserveAspectCrop
                 x: -declarativeView.globalPosition.x
 
=== modified file 'spread/Window.qml'
--- spread/Window.qml	2011-06-23 17:08:53 +0000
+++ spread/Window.qml	2011-12-12 10:03:27 +0000
@@ -16,7 +16,7 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  */
 
-import QtQuick 1.0
+import QtQuick 1.1
 import "utils.js" as Utils
 
 /*
@@ -67,17 +67,9 @@
         anchors.fill: parent
         fillMode: Image.Stretch
 
-        /* HACK: QML uses an internal cache for Image objects that seems to use as
-           key the source property of the image.
-           This is great for normal images but in this case we really want the
-           screenshot to reload everytime.
-           Since I could not find any way to disable this cache, I am using this
-           hack which essentially appends the current time to the source URL of the
-           Image, tricking the cache into doing a request to the image provider.
-        */
         source: "image://window/" + windowInfo.decoratedXid + "|"
-                                  + windowInfo.contentXid + "@"
-                                  + screen.currentTime()
+                                  + windowInfo.contentXid
+        cache: false
 
         /* Disabled during animations for performance reasons */
         smooth: !animating