110904 - attach signature fails silently on bad paths

Status: ASSIGNED

(SeaMonkey :: MailNews: Account Configuration, defect)

Description

[Rafael Kitover]

In Mail, if you go to Edit->"Mail & NewsGroup Account Settings" and type in a
file name for a signature file that is non-existant, then press OK, there is no
error message to the effect. Next time in that window, the box is blank.
Changing it to a working value fixes the problem.

This is in Build ID: 2001111303

Comment 1

[Keyser Sose]

Confirmed

Platform: PC
OS: Windows 98
Mozilla Build: 2002041603

Marking NEW.

Comment 2

[Robert Kaiser]

MASS-CHANGE:
This bug report is registered in the SeaMonkey product, but has been without a comment since the inception of the SeaMonkey project. This means that it was logged against the old Mozilla suite and we cannot determine that it's still valid for the current SeaMonkey suite. Because of this, we are setting it to an UNCONFIRMED state.

If you can confirm that this report still applies to current SeaMonkey 2.x nightly builds, please set it back to the NEW state along with a comment on how you reproduced it on what Build ID, or if it's an enhancement request, why it's still worth implementing and in what way.
If you can confirm that the report doesn't apply to current SeaMonkey 2.x nightly builds, please set it to the appropriate RESOLVED state (WORKSFORME, INVALID, WONTFIX, or similar).
If no action happens within the next few months, we move this bug report to an EXPIRED state.

Query tag for this change: mass-UNCONFIRM-20090614

Comment 3

[:Hb]

Still on SM2.0.2

Comment 4

[Edmund Wong (:ewong)]

Attachment: proposed patch (v1)

Comment 5

[Edmund Wong (:ewong)]

Comment on attachment 788856 [details] [diff] [review]
proposed patch (v1)

There's an additional string added to mail/, so asking Blake Winton for review.

Comment 6

[Karsten Düsterloh]

Comment on attachment 788856 [details] [diff] [review]
proposed patch (v1)

The actual idea, I'd say, would be to mimic the behaviour of the <filefield> elements we're using in the "normal" preferences dialog, e.g. Mail&News → Notifications → Custom Sound File: a read-only (but copyable) text field with the file path, plus a button for a file picker. We won't actively check the text field for existence of the file, but we would accept only valid (or no) files from the file picker. 
So, no additional nag windows are necessary. (Especially not when it keeps popping up if using a signature file is deactivated.)

Also, you forgot to deactivate the text field in the "identity settings" dialog. ("Manage Identities" on the main page of the account, then edit a single identity.)

Comment 7

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 788856 [details] [diff] [review]
proposed patch (v1)

I agree that using a file field would be better, but if you're going to, say, let add-ons overwrite the value directly, perhaps having the error message would also be good.

I'm happy with the string, so ui-r=me for what that's worth.  ;)

Comment 8

[Edmund Wong (:ewong)]

Attachment: wip patch - v1

Need some pointers as to what needs to be done.

Comment 9

[Edmund Wong (:ewong)]

Attachment: wip v2

Comment 10

[Edmund Wong (:ewong)]

Attachment: wip v2

Comment 11

[Edmund Wong (:ewong)]

Attachment: wip v3

Comment 12

[Edmund Wong (:ewong)]

Attachment: proposed patch (v2)

Comment 13

[Edmund Wong (:ewong)]

Comment on attachment 8358738 [details] [diff] [review]
proposed patch (v2)

guess this will need to be moved to Ian's queue. :P

Comment 14

[Ian Neal]

Comment on attachment 8358738 [details] [diff] [review]
proposed patch (v2)

>+++ b/mailnews/base/prefs/content/am-identity-edit.js
>@@ -285,31 +281,54 @@ function setupSignatureItems()
> {
>   var signature = document.getElementById("identity.signature");
>   var browse = document.getElementById("identity.sigbrowsebutton");
>   var htmlSigText = document.getElementById("identity.htmlSigText");
>   var htmlSigFormat = document.getElementById("identity.htmlSigFormat");
>   var attachSignature = document.getElementById("identity.attachSignature");
>   var checked = attachSignature.checked;
> 
>+  var sigPrefString = signature.getAttribute("prefstring");
>+  var sigPref;
>+  var sigFile;
>+  var file;
>+
>+  if (signature.identitykey)
>+  {
>+    sigPref = sigPrefString.replace("%identitykey%", signature.identitykey);
As sigPref is only used within the if why not do var sigPref here?
>+
>+    if (Services.prefs.getPrefType(sigPref) == Components.interfaces.nsIPrefBranch.PREF_STRING)
>+    {
>+      sigFile = Services.prefs.getCharPref(sigPref);
>+      file = getFile(sigFile);
This seems to be the only place sigFile is used, so why not inline it?
>+    }
>+  }
>+
As the whole of this section is only used once later on, why not move it into that if statement?


>   if (checked && !getAccountValueIsLocked(signature))
>+  {
>     signature.removeAttribute("disabled");
<- Insert it here
>+    if (file)
>+      signature.file = file;
>+  }
>   else
>+  {
>     signature.setAttribute("disabled", "true");
>+    browse.setAttribute("disabled", "true");
>+  }


>@@ -364,8 +383,46 @@ function loadSMTPServerList()
>+function selectSigFile()
>+{
>+  var fp = Components.classes["@mozilla.org/filepicker;1"]
>+                     .createInstance(Components.interfaces.nsIFilePicker);
>+  var signature = document.getElementById('identity.signature');
>+  var prefName;
>+  var sigPref;
>+
>+  if (signature.identitykey)
>+  {
>+    prefName = signature.getAttribute("prefstring")
>+                        .replace("%identitykey%", signature.identitykey);
>+
>+    if (Services.prefs.getPrefType(prefName) != Components.interfaces.nsIPrefBranch.PREF_INVALID)
>+      sigPref = Services.prefs.getCharPref(prefName);
>+  }
You do all this work, and then don't use the result!

>+++ b/mailnews/base/prefs/content/am-main.js
>@@ -48,8 +48,20 @@ function manageIdentities()
>   window.openDialog("am-identities-list.xul", "", "chrome,modal,resizable=no,centerscreen", args);
> 
>   if (args.result) {
>     // now re-initialize the default identity settings in case they changed
>     identity = account.defaultIdentity; // refetch the default identity in case it changed
>     initIdentityValues(identity);
>   }
> }
>+
>+function getFile(aString)
>+{
>+  if (!aString)
>+    return null;
>+
>+  var uri = Components.classes["@mozilla.org/toolkit/command-line;1"]
>+                      .createInstance(Components.interfaces.nsICommandLine)
>+                      .resolveURI(aString);
>+  return uri instanceof Components.interfaces.nsIFileURL ?
>+         uri.file.QueryInterface(Components.interfaces.nsILocalFile) : null;
>+}
Are there plans to use this helper anywhere else, if not, why isn't it part of am-identity-edit.js?

r- as I want to look at the new patch