Merge lp:~mzanetti/ubuntu-clock-app/drop-audioRole into lp:ubuntu-clock-app

[Michael Zanetti]

drop alarm preview's audioRole

This doesn't really work as expected anyways and would cause issues
with an upcoming API change after upstreaming the roles patch to Qt 5.5.

This is a somewhat radical approach to get past the upcoming api breakage. A more sophisticated approach can be found here:
https://code.launchpad.net/~mzanetti/ubuntu-clock-app/detect-qtmm-version/+merge/275177

However, due to various current issues in the system, the outcome is mostly the same and this doesn't add a somewhat complex workaround like the other does.

[Jenkins Bot (ubuntu-core-apps-jenkins-bot)]

PASSED: Continuous integration, rev:401
https://core-apps-jenkins.ubuntu.com/job/clock-app-ci/9/
Executed test runs:
    None: https://core-apps-jenkins.ubuntu.com/job/generic-update-mp/39/console

Click here to trigger a rebuild:
https://core-apps-jenkins.ubuntu.com/job/clock-app-ci/9/rebuild

review: Approve (continuous-integration)

[Victor Thompson (vthompson)]

Adding Jim Hodapp as a reviewer for the QtMultimedia interface change.

[Bartosz Kosiorek (gang65)]

I would vote for that solution, as it is simple and the results are the same as with:
https://code.launchpad.net/~mzanetti/ubuntu-clock-app/detect-qtmm-version/+merge/275177

I would add comment that "audioRole" was removed to keep compatibility with Qt 5.5

review: Needs Information

[Bartosz Kosiorek (gang65)]

review: Approve

[Michael Zanetti (mzanetti)]

Thanks Bartosz. I've added a more explicit hint to Qt 5.5 to the commit message.

[Michał Sawicz (saviq)]

Yeah, agreed, we don't get much out of the competing solution.

review: Approve

[Jim Hodapp (jhodapp)]

I would like to better understand why you're dropping the role completely instead of moving to the NotificationRole? As it stands right now, it'll use the old MultimediaRole at the media-hub and PulseAudio layer. So if you have music playing in music-app and then you go into the clock-app to preview the alarm sound, it should actually pause the background music instead of the duck-in effect. If that's what you desire then this is fine.

review: Needs Information

[Bartosz Kosiorek (gang65)]

For me it even better, to pause current music and play preview alarm. It will let me, better adjust the volume of the alarm.

I would like to pause audiobook, when alarm starts ringing. I will not miss anything from my audiobook. It will be annoying for me to rewind audiobook everytime alarm starts ringing.

What do you think Jim?

review: Needs Information

[Michał Sawicz (saviq)]

W dniu 22.10.2015 o 14:52, Jim Hodapp pisze:
> Review: Needs Information
>
> I would like to better understand why you're dropping the role completely instead of moving to the NotificationRole? As it stands right now, it'll use the old MultimediaRole at the media-hub and PulseAudio layer. So if you have music playing in music-app and then you go into the clock-app to preview the alarm sound, it should actually pause the background music instead of the duck-in effect. If that's what you desire then this is fine.

See the alternative proposal from the description.

In any case, what you describe does not work today, the preview plays
over music of reduced volume.

I don't think it should be Notification (or current - alert) anyway,
it's an alarm, after all? The counter argument is that if you want
Silent mode to keep the alarm preview silent, but then you can't change
the actual alarm volume with hardware buttons while playing the preview.
And you shouldn't be able to, or you will think the volume you're
changing is alarm, when in fact, it's the ringtone/alert volume.

W dniu 22.10.2015 o 15:15, Bartosz Kosiorek pisze:
> Review: Needs Information
>
> For me it even better, to pause current music and play preview alarm.
> It will let me, better adjust the volume of the alarm.
>
> I would like to pause audiobook, when alarm starts ringing. I will
> not miss anything from my audiobook. It will be annoying for me to
> rewind audiobook everytime alarm starts ringing.

Sure, that's a goal, but we can't get that today.

Also "every time alarm starts ringing", how many *alarms* (not
reminders) do you have set up?

[Bartosz Kosiorek (gang65)]

I have two types of alarms:
1. Wake up alarms - to wake up early morning
2. Daily Standup alarms - to remind me that I have an meeting every weekday.

[Michał Sawicz (saviq)]

Number 2. sounds like a lower priority reminder than an alarm, and there's other apps for that (Notes/Reminders, Calendar). Unless you want it to play regardless of Silent mode? In any case, that's two, probably recurring daily, alarms. You don't reset them multiple times a day, not even a week, so the preview playing at the actual alarm level would be my preferred solution, also using hardware buttons when the preview's playing should IMO change the resulting alarm volume, but that does mean it might need to be per-alarm, not global. There was a heated discussion about that on the mailing list recently.

OTOH there's bug #1362078, which deals with alarm volume as well.

[Jim Hodapp (jhodapp)]

> W dniu 22.10.2015 o 14:52, Jim Hodapp pisze:
> > Review: Needs Information
> >
> > I would like to better understand why you're dropping the role completely
> instead of moving to the NotificationRole? As it stands right now, it'll use
> the old MultimediaRole at the media-hub and PulseAudio layer. So if you have
> music playing in music-app and then you go into the clock-app to preview the
> alarm sound, it should actually pause the background music instead of the
> duck-in effect. If that's what you desire then this is fine.
>
> See the alternative proposal from the description.
>
> In any case, what you describe does not work today, the preview plays
> over music of reduced volume.

Yes indeed, and this was the original design. We still want this in my opinion unless the design team says otherwise.
>
> I don't think it should be Notification (or current - alert) anyway,
> it's an alarm, after all?

I agree, but we are discussing only the alarm preview in this MR. Unless the actual alarm uses the same audio role as the preview does?

> The counter argument is that if you want
> Silent mode to keep the alarm preview silent, but then you can't change
> the actual alarm volume with hardware buttons while playing the preview.
> And you shouldn't be able to, or you will think the volume you're
> changing is alarm, when in fact, it's the ringtone/alert volume.

[Michał Sawicz (saviq)]

W dniu 22.10.2015 o 21:37, Jim Hodapp pisze:
> I agree, but we are discussing only the alarm preview in this MR. Unless the actual alarm uses the same audio role as the preview does?

No, but why would this be alert role, while the real alarm would be
alarm role?

[Jim Hodapp (jhodapp)]

On Thu, Oct 22, 2015 at 4:12 PM, Michał Sawicz <email address hidden>
wrote:

> W dniu 22.10.2015 o 21:37, Jim Hodapp pisze:
> > I agree, but we are discussing only the alarm preview in this MR. Unless
> the actual alarm uses the same audio role as the preview does?
>
> No, but why would this be alert role, while the real alarm would be
> alarm role?
>

It wouldn't be, they'd be the same. I just don't believe it should be the
general video or music roles, which one of those will end up being the
default role to be equivalent to the old multimedia role.

>
> --
>
> https://code.launchpad.net/~mzanetti/ubuntu-clock-app/drop-audioRole/+merge/275179
> You are reviewing the proposed merge of
> lp:~mzanetti/ubuntu-clock-app/drop-audioRole into lp:ubuntu-clock-app.
>

[Michał Sawicz (saviq)]

> It wouldn't be, they'd be the same. I just don't believe it should be the
> general video or music roles, which one of those will end up being the
> default role to be equivalent to the old multimedia role.

Well, so that's the thing - today the role is set to "alert". While I agree long-term it shouldn't be multimedia, today we can't provide the optimal experience with alarm, either.

You did, however, show one use case where alert (or alarm? does it behave the same?) is better for this - in that it reduces multimedia volume. For that, I can be convinced it's better to jump through the hoop in the counter-proposal.

I'm abstaining from this, then. Can be convinced either way.

review: Abstain

[Ubuntu Phone Apps Jenkins Bot (ubuntu-phone-apps-jenkins-bot)]

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

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

review: Needs Fixing (continuous-integration)

=== modified file 'app/alarm/AlarmSound.qml'
--- app/alarm/AlarmSound.qml	2015-08-28 20:06:32 +0000
+++ app/alarm/AlarmSound.qml	2015-10-21 12:20:03 +0000
@@ -149,7 +149,6 @@
 
     Audio {
         id: previewAlarmSound
-        audioRole: MediaPlayer.alert
 
         function controlPlayback(fileURL) {
             source = fileURL