Merge lp:~mzanetti/ubuntu-clock-app/detect-qtmm-version into lp:ubuntu-clock-app

Proposed by Michael Zanetti
Status: Rejected
Rejected by: Bartosz Kosiorek
Proposed branch: lp:~mzanetti/ubuntu-clock-app/detect-qtmm-version
Merge into: lp:ubuntu-clock-app
Diff against target: 74 lines (+38/-2)
3 files modified
app/alarm/AlarmSound.qml (+2/-2)
app/components/AlarmAudio.qml (+35/-0)
app/components/CMakeLists.txt (+1/-0)
To merge this branch: bzr merge lp:~mzanetti/ubuntu-clock-app/detect-qtmm-version
Reviewer Review Type Date Requested Status
Albert Astals Cid (community) Abstain
Ubuntu Phone Apps Jenkins Bot continuous-integration Needs Fixing
Jenkins Bot continuous-integration Approve
Michał Sawicz (community) Abstain
Bartosz Kosiorek Needs Information
Review via email: mp+275177@code.launchpad.net

Commit message

add a hack to detect the available QtMultimedia version

This is supposed to handle API changes in our QtMultimedia distro-patches
without requiring to go through a deprecation phase

Description of the change

This would be the theoretical correct approach forward if we want the alarm sound preview to reflect the alarm volume. However, as the alarm volume seems to be restored to what the setting in the clock app is before the alarm is triggered, the current alarm role volume does not necessarily reflect the upcoming alarm's volume. Because of this, it is arguable how much sense it makes to include this workaround.

A simpler workaround to the api breakage problem, with roughly the same outcome can be found here:
https://code.launchpad.net/~mzanetti/ubuntu-clock-app/drop-audioRole/+merge/275179

To post a comment you must log in.
402. By Michael Zanetti

make api a little better

Revision history for this message
Jenkins Bot (ubuntu-core-apps-jenkins-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
Bartosz Kosiorek (gang65) wrote :

I tested it and it is working correctly for me on BQ E4.5 device.
It is nice improvement especially for QT 5.5 upgrade

One inline question below.

Do you think it could help in missing volume changes, during alarm preview?:
https://code.launchpad.net/~gang65/ubuntu-clock-app/ubuntu-clock-volume-preview/+merge/264783

Ricardo Salveti already investigated that issue, unfortunately without success.

review: Needs Information
Revision history for this message
Michael Zanetti (mzanetti) wrote :

fixed the inline comment.

For me, the volume controls during preview were working fine. Probably the issue was because before it was using the "alert" role, which seems to be wrong in any case.

As written in the description, I'm not really sure if we should merge this branch, or instead just go with the much simpler https://code.launchpad.net/~mzanetti/ubuntu-clock-app/drop-audioRole/+merge/275179

The outcome is pretty much the same with the current audio system.

403. By Michael Zanetti

s/print/console.log/

Revision history for this message
Jenkins Bot (ubuntu-core-apps-jenkins-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Bartosz Kosiorek (gang65) wrote :

@Michael
I would like to clarify, that volume preview is working perfectly on Desktop,
unfortunately it is not working on Phone.

https://code.launchpad.net/~gang65/ubuntu-clock-app/ubuntu-clock-volume-preview/+merge/264783

This is very strange issue. Unfortunately these patches not resolve volume preview issue.
Do you have other idea, why it is not working on Phone?

review: Needs Information
Revision history for this message
Michał Sawicz (saviq) wrote :

@Bartosz, it's likely related to bug #1485522, and also the fact that indicator-datetime, AFAIK, sets the volume to the one set in the clock app when playing an alarm sound.

In any case, I vote for the competing approach for now - until we clear the whole volume situation up.

review: Disapprove
Revision history for this message
Albert Astals Cid (aacid) :
review: Needs Fixing
Revision history for this message
Michał Sawicz (saviq) wrote :

I'm abstainig from this after all, see my reasoning on the counter-proposal:

https://code.launchpad.net/~mzanetti/ubuntu-clock-app/drop-audioRole/+merge/275179/comments/695921

review: Abstain
404. By Michael Zanetti

update check to 5.6, in case we'd use this branch

Revision history for this message
Jenkins Bot (ubuntu-core-apps-jenkins-bot) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Jenkins Bot (ubuntu-core-apps-jenkins-bot) wrote :
review: Approve (continuous-integration)
Revision history for this message
Nicholas Skaggs (nskaggs) wrote :

Let me know if you have further troubles. I'll be fixing the messages jenkins leaves to be a bit saner.

Revision history for this message
Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot) wrote :

FAILED: Continuous integration, rev:401
http://91.189.93.70:8080/job/ubuntu-clock-app-ci/856/
Executed test runs:

Click here to trigger a rebuild:
http://91.189.93.70:8080/job/ubuntu-clock-app-ci/856/rebuild

review: Needs Fixing (continuous-integration)
Revision history for this message
Albert Astals Cid (aacid) :
review: Abstain

Unmerged revisions

404. By Michael Zanetti

update check to 5.6, in case we'd use this branch

403. By Michael Zanetti

s/print/console.log/

402. By Michael Zanetti

make api a little better

401. By Michael Zanetti

add a hack to detect the available QtMultimedia version

This is supposed to handle API changes in our QtMultimedia distro-patches
without requiring to go through a deprecation phase

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'app/alarm/AlarmSound.qml'
2--- app/alarm/AlarmSound.qml 2015-08-28 20:06:32 +0000
3+++ app/alarm/AlarmSound.qml 2015-10-26 10:26:22 +0000
4@@ -23,6 +23,7 @@
5 import Ubuntu.Components 1.2
6 import QtQuick.Layouts 1.1
7 import Qt.labs.folderlistmodel 2.1
8+import "../components"
9
10 Page {
11 id: _alarmSoundPage
12@@ -147,9 +148,8 @@
13 }
14 }
15
16- Audio {
17+ AlarmAudio {
18 id: previewAlarmSound
19- audioRole: MediaPlayer.alert
20
21 function controlPlayback(fileURL) {
22 source = fileURL
23
24=== added file 'app/components/AlarmAudio.qml'
25--- app/components/AlarmAudio.qml 1970-01-01 00:00:00 +0000
26+++ app/components/AlarmAudio.qml 2015-10-26 10:26:22 +0000
27@@ -0,0 +1,35 @@
28+import QtQuick 2.4
29+
30+Item {
31+ id: root
32+ property string source: ""
33+ readonly property alias playbackState: priv.playbackState
34+
35+ function play() {
36+ priv.audio.play();
37+ }
38+ function stop() {
39+ priv.audio.stop();
40+ }
41+
42+ QtObject {
43+ id: priv
44+ property var audio: null
45+ property var playbackState: null
46+ }
47+
48+ Binding {
49+ target: priv
50+ property: "playbackState"
51+ value: priv.audio ? priv.audio.playbackState : 0
52+ }
53+
54+ Component.onCompleted: {
55+ try {
56+ priv.audio = Qt.createQmlObject("import QtMultimedia 5.6; Audio { source: root.source; audioRole: MediaPlayer.AlarmRole }", priv)
57+ } catch(err) {
58+ console.log("QtMultimedia 5.6 not installed. Falling back to 5.4.");
59+ priv.audio = Qt.createQmlObject("import QtMultimedia 5.4; Audio { source: root.source; audioRole: MediaPlayer.alarm }", priv)
60+ }
61+ }
62+}
63
64=== modified file 'app/components/CMakeLists.txt'
65--- app/components/CMakeLists.txt 2015-08-13 18:26:31 +0000
66+++ app/components/CMakeLists.txt 2015-10-26 10:26:22 +0000
67@@ -2,6 +2,7 @@
68 ActionIcon.qml
69 AnalogMode.qml
70 AnalogShadow.qml
71+ AlarmAudio.qml
72 Background.qml
73 Clock.qml
74 ClockCircle.qml

Subscribers

People subscribed via source and target branches