Problems if foldername has trailing white-space (webexperiment, compFields.fcc)
Categories
(Thunderbird :: Add-Ons: Extensions API, defect)
Tracking
(thunderbird_esr91 wontfix)
Tracking | Status | |
---|---|---|
thunderbird_esr91 | --- | wontfix |
People
(Reporter: G.Gersdorf, Assigned: TbSync)
References
(Regression)
Details
(Keywords: regression)
Attachments
(2 files)
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:95.0) Gecko/20100101 Firefox/95.0
Steps to reproduce:
Create a folder with trailing white-space.
Select this folder as sent folder
Send a message
In a webexperiment, set the compFields.fcc to a folder with trailing white-space
Actual results:
A new folder without the trailing white-space is created and used as sent folder.
Even worse, if the original foldername contains non-Ascii characters (e.g. 'Späce '), the newly created folder contains encoded characters (e.g. 'Sp&AOQ-ce').
the fcc is ignored and the default sent folder is used.
Expected results:
Foldername should be used as-is.
Comment 1•3 years ago
|
||
Hi Günter, thanks, and I believe you... but why on earth would anyone create a mail folder name with trailing whitespace? What type of whitespace is it? Such folder names should be prohibited imo. But if they are created on IMAP server, we'd probably need to handle them anyway. Can you elaborate where and why the whitespace was added to the folder name?
Updated•3 years ago
|
Assignee | ||
Comment 2•3 years ago
|
||
Are you able to provide an STR which does not involve an Experiment? Can this bug be triggered as a standard user interacting with a default Thunderbird?
Comment 3•3 years ago
|
||
My bet is that this is most likely one of the many regressions that bug 1571672 has caused. There's a ton of fixes which I believe are going into a 91.4.1 dot release, so they may help.
Reporter | ||
Comment 4•3 years ago
|
||
@Thomas
No one would create a foldername with trailing whitespace, but it happens. My add-on 'CopySent2Current' makes heavy use of the compFields.fcc field. One of my users ask me why the add-on doesn't work on a particular folder. He send me a picture of his folders and i recognize the trailing whitespace.
I would think this problem is caused by a call to trim() in the function getFcc() in the module MimeMessageUtils.jsm
@John
what is 'STR'?
I think the problem with compFields.fcc can't be triggered without an experiment, since the fcc field is not exposed in the compose API. But point one of my original post can be done by anyone.
@Mark
I don't think that the bug is related to bug 1571672. There is no problem with german umlauts if the foldername does not have a trailing white-space.
Assignee | ||
Comment 5•3 years ago
|
||
STR = steps to reproduce
I think the problem with compFields.fcc can't be triggered without an experiment, since the fcc field is not exposed in the compose API. But point one of my original post can be done by anyone.
There is no internal/core use of the fcc field?
Updated•3 years ago
|
Reporter | ||
Comment 6•3 years ago
|
||
I think the fcc field is not normally used. If its empty the identity.fccFolder is used.
Assignee | ||
Comment 7•3 years ago
|
||
Ok, thanks for reporting. I will check what we can do.
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 8•3 years ago
|
||
I think this should be moved elsewhere, as it is not related to WebExtension API.
I tried to have a look at the code. We have
- getMsgFolderURIFromPrefs() -> uses identiy.fccFolder
- getFcc() -> uses identiy.fcc
I have no idea why we have those two different places to store the fcc. Do you Günter?
In getFcc() the value gets trimmed:
https://searchfox.org/comm-central/rev/68c2c1641ee0e1c9f320ff304fb8276d540a18ea/mailnews/compose/src/MimeMessageUtils.jsm#201
So it checks whether the folder exists and if so, returns the trimmed folder name, which will probably cause your issue down the road, when the returned fcc value is used to get the actual folder.
Who could know the reason behind the trim line? Magnus?
Assignee | ||
Comment 9•3 years ago
|
||
This is probably where the wrong folder than gets created:
https://searchfox.org/comm-central/rev/68c2c1641ee0e1c9f320ff304fb8276d540a18ea/mailnews/compose/src/MessageSend.jsm#907
Günther, could you verify that with TB 91.4.1 or TB Beta, the issue with the non-ascii characters has been fixed and only the trim issue remains?
Reporter | ||
Comment 10•3 years ago
|
||
getFcc does not use identity.fcc (this does not exist), it uses compFields.fcc
And neither 91.4.1 nor 96b3 fixes the problem 1 of my original post.
If you create a folder with trailing space (e.g. 'INBOX/Space ') and select that folder as sent folder identity.fccFolder is set to 'INBOX/Space' (without the trailing space).
Assignee | ||
Comment 11•3 years ago
|
||
getFcc does not use identity.fcc (this does not exist), it uses compFields.fcc
Thanks.
And neither 91.4.1 nor 96b3 fixes the problem 1 of my original post.
If you create a folder with trailing space (e.g. 'INBOX/Space ') and select that folder as sent folder identity.fccFolder is set to 'INBOX/Space' (without the trailing space).
I meant the äöü issue, to confirm Marks statement in Comment #3.
Edit: Removed an observation which turned out to be wrong, caused by a misconfiguration on my side.
Assignee | ||
Comment 12•3 years ago
|
||
@Günter: I do not know if removing the trim mentioned in Comment 8 is feasible or if that has other side effects. While waiting a response from Magnus, I was looking at the compFields.fcc2
field, which is accessible through the composer UI and which I would like to make available through the WebExtension set/getComposeDetails() function.
Will your add-on work using that field or is there a specific reason you need to use compFields.fcc
?
Reporter | ||
Comment 13•3 years ago
|
||
There is no issue (and has never been) with äöü. The problem only arises if the foldername has a trailing space AND the folder is used as a special folder.
And i think, both compFields.fcc and compFields.fcc2 are normally used. compFields.fcc contains the sent folder from the identity and compFields.fcc2 is set to the folder choosen in the compose UI (and defaults to 'nocopy://')
And my add-on also uses both fields. fcc is set to the folder which can be choosen from the composer UI (and replaces the sent folder from the identity) and fcc2 is set to the sent folder from the identity if the user has set an 'additionally copy to sent folder' global option.
Assignee | ||
Comment 14•3 years ago
|
||
There is no issue (and has never been) with äöü. The problem only arises if the foldername has a trailing space AND the folder is used as a special folder.
The topic of this bug is "Problems if foldername has trailing white-space or non-ASCII characters like "äöü" (webexperiment, compFields.fcc)". Additionally, you stated in Comment #0:
Even worse, if the original foldername contains non-Ascii characters (e.g. 'Späce '), the newly created folder contains encoded characters (e.g. 'Sp&AOQ-ce').
All I want to find out: Has that Späce
issue been resolved by now, as Mark suggested, so I can concentrate on the trailing space issue? Or is it still using the bad encoding when creating the new folder due to the trailing space?
Reporter | ||
Comment 15•3 years ago
|
||
The 'or non-ASCII characters like "äöü" (webexperiment, compFields.fcc)' part of the topic was added by someone else, it was not part of the original topic.
And yes, the 'Späce' issue is fixed (in the sense that this was never an issue!)
Assignee | ||
Comment 16•3 years ago
|
||
If I understood the code correct, setting .fcc
allows to temporarily disable or override the default fcc per message, which is not used in core. And .fcc2
is mapped to the UI selection the user can use to set an additional fcc.
Would your add-on benefit from making these two fields accessible via WebExt API? My suggestion: Setting the WebExt primaryFcc/secondaryFcc to false would disable them and setting them to a WebExt folder would set/override them.
Comment 17•3 years ago
|
||
I think the problem may be that when you are setting compFields.fcc, you are not using an encoded url. An encoded url would have the space encoded, which would then not get trimmed.
The .trim
that is present seems to be a translation of the original code to fix the header: https://searchfox.org/comm-esr91/rev/b4fc9021bb71e2e7ff454b2181a95f4ccaa6f857/mailnews/compose/src/nsMsgSend.cpp#2160
That code isn't quite equivalent to the new code, but I'm guessing we probably didn't need all the old code anyway.
I think that would work. I've been trying to simulate this with going offline & sending a message and then going back online and letting it send, but it looks like that is getting messed up elsewhere and a separate folder gets created.
Comment 18•3 years ago
|
||
In general I don't think any of our code (wx nor core) should allow creating folders with trailing spaces. I'm not sure how widely servers (happen) to support it either. Exchange doesn't allow it.
Reporter | ||
Comment 19•3 years ago
|
||
@John
Would your add-on benefit from making these two fields accessible via WebExt API?
This would probably allow me to make the add-on pure webext.
@Mark
I've tried so set an encoded url but that does not work.
Please note the difference of uri and url. For a folder 'Space '
folder.URI contains 'imap://ggbs@ggbs.de/INBOX/Space ' while
folder.folderURL contains 'imap://ggbs@ggbs.de/INBOX/Space%20'
so i think fcc has to be set to an uri not an url.
@Magnus
As long as there are IMAP servers and mailprogramms/webmailers which allow creating mailboxes with trailing spaces Thunderbird must handle them. And well, currently TB has no problems with such folders, e.g. it's possible to move messages to such folders. The problem only arises if the folder is used as a special folder!
Assignee | ||
Comment 20•3 years ago
|
||
Updated•3 years ago
|
Assignee | ||
Comment 21•3 years ago
|
||
Depends on D144419
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 22•3 years ago
|
||
@Günter: The patch exposes fcc2 (the value connected to the UI selection) as ComposeDetails.fcc. (Nobody besides you knows that it is fcc2 internally). The internal fcc value is not set by Thunderbird and I hesitate to expose that (as it might be subject to removal).
What is the purpose of setting it in your add-on? Just to be able to create two copies?
Reporter | ||
Comment 23•3 years ago
|
||
No. I set fcc to override the sent folder selected in the account settings for this one mail.
The normal case is: Mail is saved to the folder defined in the accounts settings, if that is defined. If the user selects a folder via UI the mail is additionally saved to that folder.
If i would use fcc2, always two copies would be saved (as long as a sent folder is defined in the accounts settings, which i think is the normal case)
Assignee | ||
Comment 24•3 years ago
|
||
So, I should expose fcc as "defaultFcc" and fcc2 as "additionalFcc". Other suggestions for nice human readable names?
In addition, should I make defaultFcc return the actual used standard folder if it is not set, instead of ""?
Updated•3 years ago
|
Reporter | ||
Comment 25•3 years ago
|
||
So, I should expose fcc as "defaultFcc" and fcc2 as "additionalFcc". Other suggestions for nice human readable names?
Sounds good
In addition, should I make defaultFcc return the actual used standard folder if it is not set, instead of ""?
Probably yes, this would allow to distinguish between "Sent folder defined" and "No sent folder defined" in the accounts settings.
Updated•3 years ago
|
Assignee | ||
Comment 26•3 years ago
|
||
@Günter: Returning the identity fcc was not working out so great. You can now set "defaultFcc" to default
, disabled
or a MailFolder.
What I changed is, that overriding the default fcc by setting a MailFolder will now always do fcc, even though the user pref is disabled. I think this is a better fit for the override character of the setting: Either the default setting is used or the override.
The old code skipped getFcc(), if that pref was set and bypassed the override:
https://searchfox.org/comm-central/rev/16430df4c00f639809ba0aea5cf6c31575d55419/mailnews/compose/src/MimeMessageUtils.jsm#188
Objections?
Reporter | ||
Comment 27•3 years ago
|
||
I believe that if you remove the test for userIdentity.doFcc from getFcc() disabling the 'Place a copy in' from the 'copies and folder' account settings won't work any longer.
And if think the
if (composeFields.fcc.startsWith("nocopy://")) {
line from your patch should be something like
if (composeFields.fcc=="" || composeFields.fcc.startsWith("nocopy://")) {
because if 'Place a copy in' is disabled fcc is ""
Assignee | ||
Comment 28•3 years ago
|
||
Thanks for your feedback.
I did not remove the doFcc check, I just moved it below the initial check if an fcc value has been set. I still return "" if doFcc is false and no fcc override has been set. But if you think it is better to have just a partial override, I can restore the old behaviour (the folder set in defaultFcc will be ignored, if the used identity has fcc disabled). But it will break my overall concept (see below).
For your second remark: The "disabled" value does not reflect the state of the identity, but the state of the override. This is what I want to achieve:
"default" : follow the default behaviour
"disabled": force the fcc to be disabled (regardless what the default identity setting is)
MailFolder: force the fcc to store a copy in that folder (regardless what the default identity setting is)
That is the most straightforward interface, I think. Do you agree, or am I missing something?
Reporter | ||
Comment 29•3 years ago
|
||
Understood. Ok so far.
Is this correct: if i listen to onBeforeSend and no one had set fcc (e.g. via setComposeDetails()), than 'disabled' in the fcc field of the details reflects the fcc state of the account?
Assignee | ||
Comment 30•3 years ago
|
||
Is this correct: if i listen to onBeforeSend and no one had set fcc (e.g. via setComposeDetails()), than 'disabled' in the fcc field of the details reflects the fcc state of the account?
"disabled" in the defaultFcc field will only be set, if someone has manually called setComposeDetails()
and set it to "disabled" or if an Experiment has set composeFields.fcc = "nocopy://"
. The untouched value will be "default" (regardless of what that means).
I think I should rename composeDetails.defaultFcc
to composeDetails.fcc
, as it never returns the state of the default identity fcc, but the fcc configuration of that single email: Either it is using the default, enforces a custom value or enforces no fcc at all.
Assignee | ||
Updated•3 years ago
|
Updated•3 years ago
|
Assignee | ||
Updated•3 years ago
|
Updated•3 years ago
|
Comment 31•3 years ago
|
||
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/a6c96dac60c2
Do not trim valid fcc uri. r=mkmelin
Assignee | ||
Updated•3 years ago
|
Comment 32•3 years ago
|
||
Pushed by mkmelin@iki.fi:
https://hg.mozilla.org/comm-central/rev/8490805c05d5
Expose msgCompFields.fcc and msgCompFields.fcc2 via compose.ComposeDetails. r=mkmelin
Description
•