357097 - "Play a sound" textboxes are not editable, and don't disable Preview button when nothing has been selected

Status: ASSIGNED

(SeaMonkey :: Preferences, defect)

Description

[Giacomo Magnini]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; it; rv:1.8.0.7) Gecko/20060913 Fedora/1.5.0.7-1.fc5 Firefox/1.5.0.7 pango-text
Build Identifier: 

When you activate the "Play a sound" option in both prefs->navigator->downloads and prefs->mailnews->notifications, both Browse and Preview buttons become active, even if the textbox for the sound file is empty. This is obviously wrong, and is a different behaviour from prefs->privacy & security->popup windows similar panel option.
Moreover, the textboxes in both pref panels (downloads and notifications) are not editable. Again this is different from the popup windows behaviour.

Reproducible: Always

Comment 1

[[e-mail]]

So, I actually think Mail & Newsgroups Notifications gets this right.

Comment 2

[Giacomo Magnini]

Attachment: Fix behaviour for downloads pref panel

I know Neil is going to kill me for this... :)
Yeah, notifications prefs is ok, and both have "textbox readonly", so the fields are not editable... Do we want to make also window popups similar textbox readonly for consistency?

Comment 3

[Giacomo Magnini]

Attachment: Tentative fix for making all of the textboxes behave the same, using filters in the filepicker

(Includes the fix in the previous patch)

Comment 4

[[e-mail]]

Comment on attachment 242900 [details] [diff] [review]
Tentative fix for making all of the textboxes behave the same, using filters in the filepicker

>+        filepicker.appendFilter("Sounds","*.wav; *.wave");
Nit: space after comma (both times)

>+                accesskey="&playSound.accesskey;"
This patch can't be reviewed without the dtd changes.

>+               prefstring="privacy.popups.sound_url" preftype="string"/>
Why the preftype?

Comment 5

[Giacomo Magnini]

Attachment: Updated to fix comments

Actually this includes the patch for bug 357096.

Comment 6

[Ian Neal]

It may be worth looking at bug 266192 (which I'd forgotten about).

Comment 7

[Ian Neal]

Comment on attachment 242929 [details] [diff] [review]
Updated to fix comments

>Index: mailnews/base/prefs/resources/content/pref-notifications.js
>===================================================================
>+  fp.appendFilter("Sounds", "*.wav; *.wave");
>   fp.appendFilters(nsIFilePicker.filterAll);
> 
You are hardcoding "Sounds" into JS, you need to put it into a string bundle or similar, see bug 266192 for one way to do it.

Comment 8

[Giacomo Magnini]

(In reply to comment #6)
> It may be worth looking at bug 266192 (which I'd forgotten about).
> 

I've attached a patch there that does what Neil asked in his review comments, but doesn't work as well... Any comment on how to fix that would be great, so I can understand fully how to "port" the same fix to pref-popup as well.
BTW, does pref-popup need pref locking as well?

Comment 9

[Serge Gautherie (:sgautherie)]

Giacomo, jag,
Are you still working on this ?

Comment 10

[Philip Chee]

See TB Bug 591317 - Allow to play the system sound for a new message (in order to test it)

Comment 11

[Philip Chee]

Comment on attachment 242625 [details] [diff] [review]
Fix behaviour for downloads pref panel

This patch is severely bit rotted and needs to be re-written

Comment 12

[Philip Chee]

Attachment: Disable Play button if sound URL is empty

Comment 13

[Ian Neal]

Comment on attachment 8814192 [details] [diff] [review]
Disable Play button if sound URL is empty

What about mailnews notifications?
f+ for what you have done so far