Sourt out virtualfolderproperties include story

RESOLVED WORKSFORME

Status

Thunderbird
Mail Window Front End
RESOLVED WORKSFORME
10 years ago
6 years ago

People

(Reporter: Joey Minta, Unassigned)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

10 years ago
Created attachment 326603 [details] [diff] [review]
patch v1

Same story, different dialog.  Don't ask me why people felt the need to include these functions in commandglue.js when they're only called from within the dialog.  I've moved them over in a cleaned up form.  Also, dialogOverlay.xul is obsolete, so we really shouldn't be using it, ever.
Attachment #326603 - Flags: superreview?(bienvenu)
Attachment #326603 - Flags: review?(bienvenu)
(Reporter)

Comment 1

10 years ago
Oh, I should note that this patch is going to conflict with the other global cleanup patches (see Bug 440616), but those are blocked by the searchdialog patch, so I'm not holding my breath.

Comment 2

10 years ago
Comment on attachment 326603 [details] [diff] [review]
patch v1

I tried editing a virtual folder's search terms and got this error:

************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "'[JavaScript Error: "getSearchTermString is not defined" {file: "
chrome://messenger/content/virtualFolderProperties.js" line: 181}]' when calling
 method: [nsIDOMEventListener::handleEvent]"  nsresult: "0x80570021 (NS_ERROR_XP
C_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "<unknown>"  data: yes]
************************************************************

this used to be in commandglue.js and isn't in virtualFolderProperties.js...

My tree is pretty much clean at this point - perhaps there's an interaction with the other patch in your tree?
Attachment #326603 - Flags: superreview?(bienvenu)
Attachment #326603 - Flags: superreview-
Attachment #326603 - Flags: review?(bienvenu)
Attachment #326603 - Flags: review-

Comment 3

9 years ago
david, good time to revisit this?

(In reply to comment #2)
> (From update of attachment 326603 [details] [diff] [review])
> I tried editing a virtual folder's search terms and got this error:
> 
> ************************************************************
> * Call to xpconnect wrapped JSObject produced this error:  *
> [Exception... "'[JavaScript Error: "getSearchTermString is not defined" {file:
> "
> chrome://messenger/content/virtualFolderProperties.js" line: 181}]' when
> calling
>  method: [nsIDOMEventListener::handleEvent]"  nsresult: "0x80570021
> (NS_ERROR_XP
> C_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "<unknown>"  data: yes]
> ************************************************************
> 
> this used to be in commandglue.js and isn't in virtualFolderProperties.js...
> 
> My tree is pretty much clean at this point - perhaps there's an interaction
> with the other patch in your tree?

Comment 4

9 years ago
I suspect asuth's refactoring makes this moot.

Comment 5

7 years ago
asuth, do you agree?

(In reply to comment #4)
> I suspect asuth's refactoring makes this moot.

Comment 6

7 years ago
nomis, alta, is this something you could pick up?

(In reply to comment #4)
> I suspect asuth's refactoring makes this moot.

davida sez "not yet"
Whiteboard: [patchlove][has draft patch][needs new assignee?]

Comment 7

7 years ago
might this cause any real world problems for users?
or our future development?
Status: ASSIGNED → NEW

Updated

7 years ago
Assignee: jminta → acelists

Comment 8

6 years ago
I'll try to look at this.

Comment 9

6 years ago
It looks like the patch is outdated now. The functions CreateVirtualFolder and getSearchTermString to move are nowhere to be seen (in /mail or /mailnews). They are defined nowhere, just called from one test file.

Should I still apply the rest of the changes in the patch? Some of them would be applicable, but I don't know if they make sense.
Wayne, aceman, my apologies on not responding to this bug when pinged a year ago-ish.

My refactoring moved the logic in question to:
http://mxr.mozilla.org/comm-central/source/mailnews/base/src/virtualFolderWrapper.js

Presumably this sorted out the duplicate copies of the logic too.

This seems to leave some minor dialog related changes, specifically the removal of some includes, the removal of
  doSetOKCancel(onOK, onCancel);
and the offsetting addition of
  ondialogcancel="return onCancel();"
This suggests that jminta was eliminating the redundancy of there also existing an ondialogaccept property.

I do not think we can safely remove those includes given the amount of time that passed without re-evaluating them.  Since virtualFolderProperties.xul is unlikely to ever be a performance bottleneck, I don't think it's worth it unless part of a greater cleanup effort that's done very carefully.

I'm going to WFM this bug since its core goal was addressed, but if anyone disagrees, feel free to reopen.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → WORKSFORME

Comment 11

6 years ago
Thanks for explanation.
Assignee: acelists → nobody
Whiteboard: [patchlove][has draft patch][needs new assignee?]
Version: Trunk → 3.0
You need to log in before you can comment on or make changes to this bug.