Closed Bug 591317 Opened 14 years ago Closed 10 years ago

Allow to play the system sound for a new message (in order to test it)

Categories

(Thunderbird :: Preferences, defect)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 29.0

People

(Reporter: el.cameleon.1, Assigned: DomDenham)

References

Details

(Whiteboard: [good first bug][mentor=mkmelin])

Attachments

(5 files, 3 obsolete files)

User-Agent:       Mozilla/5.0 (X11; Linux i686; rv:2.0b4) Gecko/20100818 Firefox/4.0b4
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; fr-FR; rv:1.9.2.8) Gecko/20100802 Lightning/1.0b2 Thunderbird/3.1.2

It would be glad to be able to test directly into the preference panel if the default sound system is great or works as expected.

Reproducible: Always

Steps to Reproduce:
1. Open the preference menu
2. Check "Play a sound"

Actual Results:  
=> you cannot heard default sound system, but only the personalise sound (ie: wav file selected by the user)

Expected Results:  
Be able to play the 2 kind of sound in order to help me to be sure that sound is great, level is correct,...
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [good first bug]
Assignee: nobody → theabyssdragon
Status: NEW → ASSIGNED
Code should be around here:
http://hg.mozilla.org/comm-central/annotate/6c605636afe1/mail/components/preferences/general.xul#l83

Looks like it just calling the same function as the custom sound used could work:
http://hg.mozilla.org/comm-central/annotate/6c605636afe1/mail/components/preferences/general.js#l81
Assignee: chris → nobody
Whiteboard: [good first bug] → [good first bug][mentor=mkmelin]
Please see the attached patch.

The previewSound function already handles the default sound, it seems it just needed the play button enabling when selecting to use the default sound.

I can't find a test for the visibility of the "play" button, let me know if you think this needs one adding (or if I'm just failing to see it, as I suspect it'll need updating).
Attachment #8355023 - Flags: review?(mkmelin+mozilla)
I don't think we need to test it. 

However, maybe the Play button should be next to the Play a sound label (under Customize). Or what do you think, Richard?

Would also be good if it got disabled when custom file is not filled (and custom is chosen)

Please also remove the trailing whitespace on the changed line.
Attached image SoundPrefs.png
I made a quick design how it could look.
Above is the Play button before the two radio buttons and below what Magnus proposed.
Both would work but I'd prefer the above because it's arranged like a sentence: "Play the default sound" or "Play the user sound".
Attached patch Play button also moved (obsolete) — Splinter Review
I've moved the play button to match Richard's suggestion, it's also now disabled when custom is chosen but no location is entered, although no checking takes place to make sure the location is actually valid (this could probably be improved as a new bug with the play button alerting the user to any relevant errors).
Attachment #8355023 - Attachment is obsolete: true
Attachment #8355023 - Flags: review?(mkmelin+mozilla)
Attachment #8355073 - Flags: review?(mkmelin+mozilla)
Attached image soundplay2.png
I don't like how Play is so dominant there, it's not supposed to be the "main attraction". Can we place it like this instead?
Assignee: nobody → DomDenham
Attached image general-play.png
Does it make more sense with the play button in line with the "Play a sound" checkbox as in the attached screenshot, rather than the radio group (I think this is what you were origonally suggesting Magnus)?

Also I'm not sure if the browse button should be limited on its right align or not? Looking at the current preferences it would seem nothing currently right aligns in the way the play button does in attachment 8355649 [details]. An alternative would be giving the the file name field a fixed width and having the button the browse button directly after it (as on the "preferences>attachment>incoming" pane).
Flags: needinfo?(mkmelin+mozilla)
Attached patch 591317.patch (obsolete) — Splinter Review
This is the patch file for the screenshot attachment 8355689 [details], which I believe matches the current other preferences tabs as closely as possible.

As we are rejigging the preferences window I assume this will need a ui-review?
Attachment #8355073 - Attachment is obsolete: true
Attachment #8355073 - Flags: review?(mkmelin+mozilla)
Attachment #8356312 - Flags: review?(mkmelin+mozilla)
Comment on attachment 8356312 [details] [diff] [review]
591317.patch

Review of attachment 8356312 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me! r=mkmelin, but yes, it needs to get ui-r too

::: mail/components/preferences/general.js
@@ +139,3 @@
>      soundTypeEl.disabled = soundsDisabled;
>      document.getElementById('browseForSound').disabled = soundsDisabled || soundTypeEl.value != 1;
> +    document.getElementById('playSound').disabled = soundsDisabled || (soundUrlLocation.length == 0 && soundTypeEl.value != 0);

we usually prefer falsy checking code, so instead of
soundUrlLocation.length == 0 you can just use !soundUrlLocation

::: mail/components/preferences/general.xul
@@ +108,5 @@
> +          <button id="browseForSound" label="&browse.label;" 
> +                  accesskey="&browse.accesskey;" oncommand="gGeneralPane.browseForSoundFile();">
> +            <observes element="soundUrlLocation" attribute="disabled"/>
> +          </button>
> +        </hbox>          

please remove the trailing whitespace while you're touching this code.
(if you see the patch through the review link they are marked in red)
Attachment #8356312 - Flags: review?(mkmelin+mozilla) → review+
can we get an updated patch , you can carry the r+ and set the checking-needed keyword when you upload the new patch.
get ui-r from e.g. Richard before adding checkin-needed
Flags: needinfo?(mkmelin+mozilla)
I've removed the trailing white spaces on changed lines, changed the check on soundUrlLocation and corrected the identing on lines 89/90 on general.xul.

Thanks for the help.
Attachment #8356312 - Attachment is obsolete: true
Attachment #8358030 - Flags: ui-review?(richard.marti)
Attachment #8358030 - Flags: review+
Comment on attachment 8358030 [details] [diff] [review]
General Preferences now has play button enabled when using the default sound and play button moved. r=mkmelin

Looks good.
Attachment #8358030 - Flags: ui-review?(richard.marti) → ui-review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/779f464eb237
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 29.0
See Also: → 357097
You need to log in before you can comment on or make changes to this bug.