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)
Thunderbird
Preferences
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)
26.46 KB,
image/png
|
Details | |
53.82 KB,
image/png
|
Details | |
61.49 KB,
image/png
|
Details | |
53.25 KB,
image/png
|
Details | |
4.86 KB,
patch
|
DomDenham
:
review+
Paenglab
:
ui-review+
|
Details | Diff | Splinter Review |
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,...
Updated•14 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Reporter | ||
Updated•14 years ago
|
Whiteboard: [good first bug]
Updated•13 years ago
|
Assignee: nobody → theabyssdragon
Updated•13 years ago
|
Status: NEW → ASSIGNED
Comment 1•11 years ago
|
||
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
Updated•11 years ago
|
Whiteboard: [good first bug] → [good first bug][mentor=mkmelin]
Assignee | ||
Comment 2•10 years ago
|
||
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)
Comment 3•10 years ago
|
||
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.
Comment 4•10 years ago
|
||
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".
Assignee | ||
Comment 5•10 years ago
|
||
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)
Assignee | ||
Comment 6•10 years ago
|
||
Comment 7•10 years ago
|
||
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?
Updated•10 years ago
|
Assignee: nobody → DomDenham
Assignee | ||
Comment 8•10 years ago
|
||
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)
Assignee | ||
Comment 9•10 years ago
|
||
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 10•10 years ago
|
||
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+
Comment 11•10 years ago
|
||
can we get an updated patch , you can carry the r+ and set the checking-needed keyword when you upload the new patch.
Comment 12•10 years ago
|
||
get ui-r from e.g. Richard before adding checkin-needed
Flags: needinfo?(mkmelin+mozilla)
Assignee | ||
Comment 13•10 years ago
|
||
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 14•10 years ago
|
||
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+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 15•10 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•