Closed Bug 984948 Opened 10 years ago Closed 10 years ago

folder list in message filter dialog is empty

Categories

(SeaMonkey :: MailNews: General, defect)

defect
Not set
normal

Tracking

(seamonkey2.26 fixed, seamonkey2.27 fixed, seamonkey2.28 fixed)

RESOLVED FIXED
seamonkey2.28
Tracking Status
seamonkey2.26 --- fixed
seamonkey2.27 --- fixed
seamonkey2.28 --- fixed

People

(Reporter: moz, Unassigned)

References

Details

(Keywords: regression)

Attachments

(5 files, 2 obsolete files)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:29.0) Gecko/20100101 Firefox/29.0 SeaMonkey/2.26a2 (Beta/Release)
Build ID: 20140312013001

Steps to reproduce:

add new message filter or edit existing one with action copy or move to folder


Actual results:

folder list for target of action stays empty. Existing filters keep working. Only GUI for selection is broken. applies to pop3, IMAP and news-Accounts

Probably related error messages:

Error: : Component returned failure code: 0x80550005 [nsIMsgFolder.msgDatabase]
Source File: chrome://messenger/content/mailWidgets.xml
Line: 1938

Error: this.mFilterList is null
Source File: chrome://messenger/content/FilterListDialog.js
Line: 79




Expected results:

List of available target folders should be visible.
Target window for 'Move' is empty for an existing filter on POP3 account SM-Trunk Linux x86_64.
Regression window:

Last good: 2014-01-27 04:05:00 PST
http://hg.mozilla.org/mozilla-central/rev/611698b4a246
http://hg.mozilla.org/comm-central//rev/5a029594d5f0

First bad: 2014-01-28 00:32:00 PST
http://hg.mozilla.org/mozilla-central/rev/4da3e21a0e5f
http://hg.mozilla.org/comm-central//rev/dc59ad3ee2f7
Attached image filter_target.png
Mhm, better to give a picture as example
No problem in Thunderbird 30.0a  20140316235303 GMT.

Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:30.0) Gecko/20100101 Thunderbird/30.0a1
The only thing suspicious is Bug 878805
Backout of the 4 patches of Bug 878805 was not possible, except the last one. But. *g* First i removed all parts not belonging to SM, then the rest which still would fail to backout, then i renamed the patches alphabetically so that my script would back them out in the correct order and then build a new SM while backing out the modified patches.

Well, the problem was gone. There is a great probability now that the culprit is within the patches of Bug 878805.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Setting dependency on Bug 878805 - Check UI consistency across all Thunderbird folderpickers
Depends on: 878805
(In reply to Philip Chee via email)

I think this would be a great time to unfork FilterListDialog.js (suite's is radically different and rdf based).

This would mean unforking
/suite/mailnews/search/SearchDialog.xul and
/suite/mailnews/search/FilterListDialog.xul

and getting rid of the above plus
/suite/mailnews/msgFolderPickerOverlay.xul and
/suite/mailnews/search/FilterListDialog.js and
/suite/locales/en-US/chrome/mailnews/msgFolderPickerOverlay.dtd and
/mailnews/base/content/msgFolderPickerOverlay.js (suite is the only user)

See bug 507676 - seems like churn when suite could simply unfork.
Oh I agree, but I have totally no idea how the old rdf or the new filterlist code works.
I guess this is HELPWANTED, or rather bug 507676 is.
Depends on: 507676
Keywords: helpwanted
all you have to do is copy
/mail/base/content/FilterListDialog.js
/mail/base/content/FilterListDialog.xul
/mail/base/content/SearchDialog.js 
/mail/base/content/SearchDialog.xul

to /suite/mailnews/search as a start and build, see if there's something else that might be needed, then get rid of unused msgFolderPickerOverlay files.

i don't know how you pick which /mail modules suite uses, build time maybe to get /mail stuff automatically.
Keywords: regression
OS: Linux → All
Hardware: x86_64 → All
(In reply to alta88 from comment #11)

> i don't know how you pick which /mail modules suite uses, build time maybe
> to get /mail stuff automatically.
SeaMonkey uses nothing from /mail/ Mail is Thunderbird UI. We do share /mailnews/. SeaMonkey Mail code is in /suite/mailnews/

So I did a little digging. The Filter Editor lives in /mailnews/ so we should have exactly the same Filter Editor. But the Folder Picker widget is blank in SeaMonkey.

function onEditFilter() //SeaMonkey version
  var selectedFilter = currentFilter();
  var curFilterList = currentFilterList();
  var args = {filter: selectedFilter, filterList: curFilterList};

  window.openDialog("chrome://messenger/content/FilterEditor.xul", "FilterEditor", "chrome,modal,titlebar,resizable,centerscreen", args);

function onEditFilter() //Thunderbird version
  let selectedFilter = currentFilter();
  if (!selectedFilter)
    return;

  let args = {filter: selectedFilter, filterList: gCurrentFilterList};

  window.openDialog("chrome://messenger/content/FilterEditor.xul", "FilterEditor", "chrome,modal,titlebar,resizable,centerscreen", args);

So I don't see any substantive differences.
(In reply to Philip Chee from comment #12)
> (In reply to alta88 from comment #11)
> 
> > i don't know how you pick which /mail modules suite uses, build time maybe
> > to get /mail stuff automatically.
> SeaMonkey uses nothing from /mail/ Mail is Thunderbird UI. We do share
> /mailnews/. SeaMonkey Mail code is in /suite/mailnews/
> 

somewhere along the way, the 4 files in comment 11 forked.  who knows how they ended up in /suite/mailnews/search/ but the Tb versions have had a lot of work done and thus the suggestion to unfork/copy them to /suite/mailnews/search/.  to keep them synced, you can copy at build time.  up to you.

> So I did a little digging. The Filter Editor lives in /mailnews/ so we
> should have exactly the same Filter Editor. But the Folder Picker widget is
> blank in SeaMonkey.
> 

you're digging in the wrong place.  clearly shared code won't be the problem if it works in Tb.  the error is due to suite's treeview implementation in menulists which requires a non null folder at setup time, and doesn't handle null (ie the folder picker in Tb puts up the Choose Folder label due to design or the folder doesn't exist).  Tb doesn't use the treeview for folder menulists. the sole remaining folder treeview is in the saved search/virtual folder multifolder select dialog.
This affects our next beta, so either we should backout the original patch or try to rather fix this bug here quickly, for example by unforking (See Comment 11 and Comment 13), not sure how easy that is.
Attached patch suite.patch (obsolete) — Splinter Review
this patch copies
/mail/base/content/FilterListDialog.js
/mail/base/content/FilterListDialog.xul

to suite/mailnews/search/.  the .js file imports only shared modules; the .xul file's script/dtd are the same and there are additional existing css files included (from overlays that are removed).  we'll see if it's this easy..
Assignee: nobody → alta88
Attachment #8400316 - Flags: review?(bugzilla)
I assume copying of the TB files is a quick fix for the nearest release of SM.
But actual unforking would be to merge the 2 copies into /mailnews/base/search . I would support that. But it can be done in a new bug after branching for TB32.
Not merging the files would keep them forked and allow for new differences in the future. But maybe I didn't understand and that is what is wanted?
This Problem I created a Bug for Yesterday or day before For Mac OSX.9.2 and SeaMonkey 26 Beta 1  for same problem. Had to revert to Version 25 Beta in order for ability to create Message filters
Comment on attachment 8400316 [details] [diff] [review]
suite.patch

Not sure why, but it does not work this way. To get the patch to apply, I had to copy the FilterListDialog.js file from mail/... again as there were conflicts on patch apply.
When trying to open the filter dialog, I get this error message:
XML Parsing Error: undefined entity
Location: chrome://messenger/content/FilterListDialog.xul
Line Number 14, Column 1:<window id="filterListDialog"
^

MDN says this can be caused by a failure to load a dtd file. Opening chrome://messenger/locale/FilterListDialog.dtd in the browser works fine though.
Attachment #8400316 - Flags: review?(bugzilla) → review-
Actually copying mail/locales/en-US/chrome/messenger/FilterListDialog.dtd to suite/locales/en-US/chrome/mailnews/FilterListDialog.dtd fixes this problem (of course this would work for trunk only). But then the folder list still seems to be empty..
(In reply to Frank Wein [:mcsmurf] from comment #22)
> Actually copying mail/locales/en-US/chrome/messenger/FilterListDialog.dtd to
> suite/locales/en-US/chrome/mailnews/FilterListDialog.dtd fixes this problem

Right. The issue was window.title vs filterListDialog.title. The "undefined entity" error message told you that (unfortunately the line and column information given in the error message is often useless/wrong).

(In reply to Frank Wein [:mcsmurf] from comment #22)
> But then the folder list still seems to be empty..

I wouldn't be surprised if our implementations of other parts that the code is referring to are quite different. This is also the reason why I felt copying anything from mail/ here would not be advised for any quick branch fix, which should be our priority. There I think we should go the back-out route. For trunk I agree copy/adapt might be a good strategy, provided we test it thoroughly (and get it to work in the first place, but that is just a matter of time).
(In reply to Jens Hatlak (:InvisibleSmiley) from comment #23)
> (In reply to Frank Wein [:mcsmurf] from comment #22)
> > But then the folder list still seems to be empty..
> 
> I wouldn't be surprised if our implementations of other parts that the code
> is referring to are quite different. This is also the reason why I felt
> copying anything from mail/ here would not be advised for any quick branch
> fix, which should be our priority. There I think we should go the back-out
> route. For trunk I agree copy/adapt might be a good strategy, provided we
> test it thoroughly (and get it to work in the first place, but that is just
> a matter of time).

The filter backend should be the same as TB's in /mailnews/base/search, only the frontend filter list dialog is forked. The filters should also be stored in the same place.
So I don't think the implementations are that different, there should be just some small tweak to do.
aceman: So would backing out be acceptable for TB 29 at least? Next big TB release is 31 afaik. Or we need come up with a quick solution for this for SeaMonkey to fix what you mentioned in Comment 13. Even if we want to copy all the files over, fixing the localization strings is not possible for beta anymore. For aurora it may be doable (late-l10n).
I have not written comment 13 :)
Backing out of TB29 beta is a question from standard8, but as long as you find the needed patches (there were some fixes on top of bug 878805) it should be possible. The patch itself is a visual polish, not a critical feature. It will do no harm if it is pulled out of TB29 beta that never gets released. But alta88 is the author so he should comment on it.

Can the differing strings be fixed on the TB side? Is there something more than the 1 string with differing ID from comment 23? But the filterListDialog.title is probably the better name to keep.
The failure of the simple copy patch was due to, of course, strings.  The following patch works (suite has actually been built/tested a bit with it).

As for backing out of Tb29, there have been some patches on top of bug 878805.  The ramifications are unclear, likely minor, but it would seem just backing out of 29 is doable, as long as it remains in 30/31.
Attached patch suite.patchSplinter Review
Attachment #8400316 - Attachment is obsolete: true
Attachment #8407519 - Flags: review?(bugzilla)
Comment on attachment 8407519 [details] [diff] [review]
suite.patch

>-<!ENTITY filterListDialog.title "Message Filters">
>+<!ENTITY window.title "Message Filters">

As mentioned in previous comments, it would be good if we would not do that (along with correcting the XUL for it) and instead port the nicer name that we have back to Thunderbird.
(In reply to alta88 from comment #28)
> Created attachment 8407519 [details] [diff] [review]
> suite.patch

WFM on Trunk SM Linux x86_64
I am still Holding to SeaMonkey 25 Beta and resisting Moving to 26 Beta1 because of this
Any "Cures" should be applied to Mac version of SM as well
So maybe I totally missed something while testing a few things, but I can simply fix this bug by adding
<!ENTITY recentFolders.label "Recent">
<!ENTITY recentFolders.accesskey "R">

to suite/locales/en-US/chrome/mailnews/messenger.dtd as searchWidgets.xml replaces the msgFolderPickerOverlay.dtd reference with a messenger.dtd reference. Of course for trunk we could still replace the FilterListDialog.js/xul with the TB version, but for branches we could look into fixing it somewhere there. Maybe we could even restore the msgFolderPickerOverlay.dtd reference as msgFolderPickerOverlay.dtd still exists in mail/ on comm-beta/TB 29 (see http://hg.mozilla.org/comm-central/diff/2db24bbb4a11/mailnews/base/search/content/searchWidgets.xml for patch that changed this). That file is gone though in comm-aurora/TB 30, so need a different solution there.
This seems to fix the issue for SeaMonkey, currently testing if Thunderbird still works fine (by code inspection it should, but making sure it does).
Attachment #8407873 - Flags: review?(alta88)
Comment on attachment 8407873 [details] [diff] [review]
comm-beta/SeaMonkey 2.26/TB 29 patch

Review of attachment 8407873 [details] [diff] [review]:
-----------------------------------------------------------------

good catch.  the goal was to remove all msgFolderPickerOverlay.[js|xul|dtd] from /mail, and if you take the FilterListDialog.[js|xul] and strings there will be one sole remaining use in suite's searchDialog.[js|xul].

Bug 964425 removed the actual msgFolderPickerOverlay.dtd file, so for comm 30 all you need to do is back out that patch.
Attachment #8407873 - Flags: review?(alta88) → review+
Well, the patch from Bug 964425 already is on aurora, I think string changes on aurora should be avoided.
It's not really a string change, it's restoring a file with an existing entity, no?  Unless I'm mistaken, there should be no l10n work at all.
(In reply to alta88 from comment #36)
> It's not really a string change, it's restoring a file with an existing
> entity, no?  Unless I'm mistaken, there should be no l10n work at all.

Localizers probably have already removed the file from their localizations. Any way we do it, this will be a late string change in any case, but we should keep it as small as possible on anything string-frozen, which includes Aurora.
Comment on attachment 8407519 [details] [diff] [review]
suite.patch

Review of attachment 8407519 [details] [diff] [review]:
-----------------------------------------------------------------

[FTR this is just a drive-by patch-based heads up, not an actual review. Didn't apply/test anything.]

::: suite/locales/en-US/chrome/mailnews/FilterListDialog.dtd
@@ +33,5 @@
>  <!ENTITY folderPickerPrefix.label "Run selected filter(s) on:">
>  <!ENTITY folderPickerPrefix.accesskey "c">
> +<!ENTITY helpButton.label "Help">
> +<!ENTITY helpButton.accesskey "H">
> +<!ENTITY closeCmd.key "W"> 

Trailing whitespace on closeCmd.key line

::: suite/locales/en-US/chrome/mailnews/filter.properties
@@ +62,5 @@
>  filterAction3=deleted
>  filterAction4=marked as read
>  filterAction5=thread killed
>  filterAction6=thread watched
> +filterAction7=starred

I think this change is wrong. SM doesn't use "starred" anywhere else, we still refer to this state as "flagged".
[Approval Request Comment]
Regression caused by (bug #): Bug 878805
User impact if declined: Broken message filter dialog in SeaMonkey
Testing completed (on m-c, etc.): Works fine locally in SeaMonkey and Thunderbird
Risk to taking this patch (and alternatives if risky): Almost no risk, adds back a reference to an already existing .dtd file
String changes made by this patch: - (the .dtd file already exists on the comm-beta branch)

Totally forgot to add back the entry to jar.mn. According to #l10n channel this should be ok as the tools don't use jar.mn to determinate what files to translate/delete. Also see http://mxr.mozilla.org/l10n-mozilla-beta/find?string=/mail/chrome/messenger/msgFolderPickerOverlay.dtd the file still seems to be there for all locales.
Attachment #8408071 - Flags: review?
Attachment #8408071 - Flags: approval-comm-beta?
Attachment #8408071 - Flags: review? → review?(alta88)
Attachment #8407873 - Attachment is obsolete: true
Attachment #8408071 - Flags: review?(alta88) → review+
Somewhat ugly patch, but that's the only more or less clean solution I can think of right now. I tested it locally that it works with Thunderbird and SeaMonkey on aurora branch.
Attachment #8408238 - Flags: review?(alta88)
Comment on attachment 8408238 [details] [diff] [review]
comm-aurora/SeaMonkey 2.27/TB 30 patch

Review of attachment 8408238 [details] [diff] [review]:
-----------------------------------------------------------------

it's just to get through the transition to comm 31 where hopefully things will realign.
Attachment #8408238 - Flags: review?(alta88) → review+
Comment on attachment 8408071 [details] [diff] [review]
comm-beta/SeaMonkey 2.26/TB 29 patch

a=me
Attachment #8408071 - Flags: approval-comm-beta? → approval-comm-beta+
Version: SeaMonkey 2.26 Branch → Trunk
Comment on attachment 8408238 [details] [diff] [review]
comm-aurora/SeaMonkey 2.27/TB 30 patch

[Approval Request Comment]
Regression caused by (bug #): Bug 878805
User impact if declined: Broken message filter dialog in SeaMonkey
Testing completed (on m-c, etc.): Works fine locally, branch-only patch
Risk to taking this patch (and alternatives if risky): Very few risk, small patch
String changes made by this patch: -
Attachment #8408238 - Flags: approval-comm-aurora?
Comment on attachment 8407519 [details] [diff] [review]
suite.patch

I can confirm that this large patch fixes the issue on trunk, FWIW.
Comment on attachment 8407519 [details] [diff] [review]
suite.patch

This change is out of scope for this bug. (It isn't really in scope for bug 507676 either, since it includes a number of unnecessary regressions.)
Attachment #8407519 - Flags: review?(bugzilla)
Attached patch Possible patchSplinter Review
Attachment #8409326 - Flags: review?(bugzilla)
(In reply to neil@parkwaycc.co.uk from comment #47)
> Created attachment 8409326 [details] [diff] [review]
> Possible patch

These changes are enough to fix the problem for my SM Trunk Linux x86_64.
Just tried to use the new 26 Beta2  version and corrupted my SeaMonkey install So bad I had to go to FireFox (pre-Australis version) and Down load Beta 25 again. From Mozilla
 Using Mac OSX.9.2 when clicked on was message (after restart of SeaMonkey) Application Defective ether corrupt or components missing.
 Went back to Beta25 version. Think I am going to stick until go to Beta 27 give you time to get your acts together.
Assignee: alta88 → nobody
Keywords: helpwanted
Comment on attachment 8408238 [details] [diff] [review]
comm-aurora/SeaMonkey 2.27/TB 30 patch

a=me for comm-aurora
Attachment #8408238 - Flags: approval-comm-aurora? → approval-comm-aurora+
Comment on attachment 8409326 [details] [diff] [review]
Possible patch

Stealing review from mcsmurf. This patch works for me r+
Attachment #8409326 - Flags: review?(bugzilla) → review+
Pushed comm-central changeset 1485af0790aa.
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.28
So was the idea of replacing the files with TB's versions abandoned?
(In reply to :aceman from comment #55)
> So was the idea of replacing the files with TB's versions abandoned?

I'd say only as far as this bug is concerned, which has the focus on fixing a regression. For that, with branches in mind, the safest approach is best. Beyond that, probably in another bug, your approach might be perfectly fine (as in take your patch as-is).
confirmed:  the last patch and release of 2.27a2 is fixed!
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: