ubuntu-touch-coreapps-reviewers team mailing list archive

[Andrew Hayzen <ahayzen@xxxxxxxxx>]

Review: Needs Fixing

Looking good so far, a few inline comments, I haven't run the code yet these are just observations :-)

Also the code should pass PEP8 and pyflakes, see the link [0] for the current errors.

And could you add an entry for yourself in the debian/changelog (simply run $ dch) and link bug 1261587 as has been done in other entries.

0 - http://pastebin.ubuntu.com/14082325/

Diff comments:

> === modified file 'app/ui/LibraryEmptyState.qml'
> --- app/ui/LibraryEmptyState.qml	2015-10-28 01:05:33 +0000
> +++ app/ui/LibraryEmptyState.qml	2015-12-17 23:57:07 +0000
> @@ -22,6 +22,8 @@
>  
>  Page {
>      id: libraryEmptyPage
> +    objectName: "emptyLibrary"
> +    visible: false

Is the visible: false required?

>      anchors {
>          fill: parent
>      }
> 
> === modified file 'tests/autopilot/music_app/content/mediascanner-2.0/mediastore.sql'
> --- tests/autopilot/music_app/content/mediascanner-2.0/mediastore.sql	2015-11-03 02:43:10 +0000
> +++ tests/autopilot/music_app/content/mediascanner-2.0/mediastore.sql	2015-12-17 23:57:07 +0000
> @@ -1,8 +1,8 @@
>  BEGIN TRANSACTION;
>  DELETE FROM `schemaVersion`;
> -INSERT INTO `schemaVersion` VALUES(10);
> +INSERT INTO `schemaVersion` VALUES(9);

Why are we downgrading the schemasVersion? Note that rc-proposed mediascanner could be infront of Wily (like the same as Xenial)

>  
> -DROP TABLE media;
> +DROP TABLE  media;

Can the double space be removed?

>  CREATE TABLE media (
>      filename TEXT PRIMARY KEY NOT NULL CHECK (filename LIKE '/%'),
>      content_type TEXT,
> 
> === added directory 'tests/autopilot/music_app/content/mediascanner-2.0/songs'
> === renamed file 'tests/autopilot/music_app/content/1.ogg' => 'tests/autopilot/music_app/content/mediascanner-2.0/songs/1.ogg'
> === renamed file 'tests/autopilot/music_app/content/2.ogg' => 'tests/autopilot/music_app/content/mediascanner-2.0/songs/2.ogg'
> === renamed file 'tests/autopilot/music_app/content/3.mp3' => 'tests/autopilot/music_app/content/mediascanner-2.0/songs/3.mp3'
> === modified file 'tests/autopilot/music_app/tests/__init__.py'
> --- tests/autopilot/music_app/tests/__init__.py	2015-02-03 14:42:34 +0000
> +++ tests/autopilot/music_app/tests/__init__.py	2015-12-17 23:57:07 +0000
> @@ -190,7 +193,12 @@
>          f = open(mediascannerpath + "/mediastore.sql", 'rb')
>          sql = f.read().decode("utf-8")
>          cur = con.cursor()
> -        cur.executescript(sql)
> +        try:
> +            cur.executescript(sql)
> +            con.commit()
> +        except:
> +            logger.debug("Mediscanner patching failed %s" % sys.exc_info()[0])

You could do the following then you can remove the import sys I believe.
except Exception as e:
  logger.debug("Mediscanner patching failed %s" % e)

> +            raise
>          con.close()
>  
>          logger.debug(


-- 
https://code.launchpad.net/~vamrocks602/music-app/test-empty-library/+merge/280794
Your team Music App Developers is subscribed to branch lp:music-app.