Open Bug 110904 Opened 19 years ago Updated 5 years ago

attach signature fails silently on bad paths

Categories

(SeaMonkey :: MailNews: Account Configuration, defect)

x86
Windows XP
defect
Not set
minor

Tracking

(Not tracked)

ASSIGNED

People

(Reporter: caelum, Assigned: ewong)

Details

Attachments

(1 file, 5 obsolete files)

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
Confirmed

Platform: PC
OS: Windows 98
Mozilla Build: 2002041603

Marking NEW.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Product: Browser → Seamonkey
Assignee: sspitzer → mail
Assignee: mail → nobody
QA Contact: esther → message-display
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
Status: NEW → UNCONFIRMED
Still on SM2.0.2
Severity: normal → minor
Status: UNCONFIRMED → NEW
Component: MailNews: Message Display → MailNews: Account Configuration
OS: Windows ME → Windows XP
QA Contact: message-display → mailnews-account
Attached patch proposed patch (v1) (obsolete) — Splinter Review
Assignee: nobody → ewong
Status: NEW → ASSIGNED
Attachment #788856 - Flags: review?(mnyromyr)
Comment on attachment 788856 [details] [diff] [review]
proposed patch (v1)

There's an additional string added to mail/, so asking Blake Winton for review.
Attachment #788856 - Flags: review?(bwinton)
Attachment #788856 - Flags: review?(bwinton) → ui-review?(bwinton)
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.)
Attachment #788856 - Flags: review?(mnyromyr) → review-
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.  ;)
Attachment #788856 - Flags: ui-review?(bwinton) → ui-review+
Attached patch wip patch - v1 (obsolete) — Splinter Review
Need some pointers as to what needs to be done.
Attachment #788856 - Attachment is obsolete: true
Attachment #8358229 - Flags: feedback?(neil)
Attached patch wip v2 (obsolete) — Splinter Review
Attachment #8358229 - Attachment is obsolete: true
Attachment #8358229 - Flags: feedback?(neil)
Attachment #8358308 - Flags: feedback?(neil)
Attachment #8358308 - Attachment is obsolete: true
Attachment #8358308 - Flags: feedback?(neil)
Attached patch wip v2 (obsolete) — Splinter Review
Attached patch wip v3 (obsolete) — Splinter Review
Attachment #8358318 - Attachment is obsolete: true
Attachment #8358394 - Attachment is obsolete: true
Attachment #8358738 - Flags: review?(mnyromyr)
Comment on attachment 8358738 [details] [diff] [review]
proposed patch (v2)

guess this will need to be moved to Ian's queue. :P
Attachment #8358738 - Flags: review?(mnyromyr) → review?(iann_bugzilla)
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
Attachment #8358738 - Flags: review?(iann_bugzilla) → review-
You need to log in before you can comment on or make changes to this bug.