Code review comment for lp:~mzanetti/unity8/music-preview

MichaƂ Sawicz (saviq) wrote :

8 + property bool isCurrent: false

20 + Binding {
21 + target: item
22 + property: "isCurrent"
23 + value: previewListView.currentIndex == index
24 + }
25 +

102 + Connections {
103 + target: root
104 + onIsCurrentChanged: {
105 + if (!root.isCurrent) {
106 + audioPlayer.stop();
107 + }
108 + }
109 + }

ListView.isCurrentItem not working?

=====

99 + onErrorStringChanged: print("Audio player error:", errorString)

Use console.warn() please, so that we can shut this up when we implement a log printer.

=====

101 + }
102 + Connections {

176 + }
177 + MouseArea {

Newline please. There's a few other places, too.

====

112 + anchors {
113 + left: parent.left
114 + right: parent.right
115 + }

121 + anchors {
122 + left: parent.left
123 + right: parent.right
124 + }

Compress to one line?

=====

117 + height: childrenRect.height

That's the default for a Column.

=====

125 + visible: trackRepeater.count > 0

Shouldn't the whole Column be made invisible?

====

139 + property bool isPlayingItem: false

143 + onSourceChanged: trackItem.isPlayingItem = false;

151 + isPlayingItem = true;

I'd be worried this might get out of sync... I know here it's pretty reliable, but just consider this: I usually add a "property Item playingItem" to the Repeater/*View, and then do "isPlayingItem: repeater.playingItem == thisItem", what do you think?

=====

157 + width: parent.width

162 + anchors.verticalCenter: parent.verticalCenter

Use anchors for width, too, please?

=====

164 + UbuntuShape {
165 + id: playButtonShape

Why not Button/AbstractButton? There's no touchdown effect on tap.

=====

159 + property int column1Width: units.gu(3)
160 + property int column2Width: width - (2 * spacing) - column1Width - column3Width
161 + property int column3Width: units.gu(4)

Eek... /me wants Qt 5.2's layouts ;)

=====

206 + UbuntuShape {
207 + id: progressBarFill

No way to use the SDK's progress bar that small? Please chat with them whether they'd want it there.

=====

223 + top: parent.bottom

I'm always frowning at items outside of their parents...

=====

229 +

Bad newline.

=====

We sure QtMultimedia will work in all our testing environments? Should we maybe think of a mock QtMultimedia backend?

=====

Missing Depends on qtmultimedia.

review: Needs Fixing

« Back to merge proposal