Closed
Bug 464710
Opened 16 years ago
Closed 7 years ago
Use folderPane.js to convert other rdf-trees to js
Categories
(Thunderbird :: Mail Window Front End, defect)
Thunderbird
Mail Window Front End
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 59.0
People
(Reporter: jminta, Assigned: aceman)
References
(Blocks 1 open bug)
Details
(Keywords: memory-footprint, perf)
Attachments
(5 files, 17 obsolete files)
(deleted),
image/png
|
Details | |
(deleted),
patch
|
mkmelin
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
frg
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mkmelin
:
review+
frg
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
mkmelin
:
review+
frg
:
review+
|
Details | Diff | Splinter Review |
This is the first real test of the js-folder-pane's extensibility. The offline-folders dialog, along with the saved-search scope dialog each want to display trees of folders, though not the specific lists of folders provided by the main-window's modes.
These patches show how to add additional modes to folderPane.js, and thereby eliminates these rdf-driven trees.
Note that these changes were in the kill-rdf branch as 654:41ed0521f6f8 and 656:cf3baa820a87
Attachment #347978 -
Flags: review?(bienvenu)
Updated•16 years ago
|
Attachment #347978 -
Flags: review?(bienvenu) → review-
Comment 1•16 years ago
|
||
Comment on attachment 347978 [details] [diff] [review]
patch v1
this will be very cool. The current offline picker expands all accounts and folders, and I think the new offline picker should do the same.
The saved search scope picker comes up blank w/ this patch applied. Nothing interesting on the console.
You don't need the version=1.8 stuff for the script type, as I understand it.
Comment 2•16 years ago
|
||
Are there any of the other trees that don't expand automatically?
Comment 3•16 years ago
|
||
I don't believe there are any that don't expand automatically.
Updated•13 years ago
|
Whiteboard: [patchlove][has draft patch][needs new assignee]
Updated•12 years ago
|
Assignee: jminta → nobody
Status: ASSIGNED → NEW
Keywords: helpwanted
Whiteboard: [patchlove][has draft patch][needs new assignee] → [patchlove][has draft patch]
I think I can move this. The interface to the folder tree has changed since the 8 year old patch but it seems I've got it running for the virtual folder (saved search) dialog. Needed also some updates to folderPane.js so that it does not assume it runs in the full folder pane with all the UI elements.
The virtual folder (saved search) dialog and its folder picker (choosing which folders to search) seem to work with this patch.
Even with compiled out folderDataSources via patch https://hg.mozilla.org/try-comm-central/rev/6532dd6d32ef3488aff0911c3d6066e19e0ca659 .
Please play with this a bit if it is the right direction. I can then look at fixing up the other dialog.
Attachment #8843641 -
Flags: feedback?(mkmelin+mozilla)
Attachment #8843641 -
Flags: feedback?(jorgk)
Comment 6•8 years ago
|
||
I'll look at it tonight. Despite discussing this at length with Aceman on IRC last night, I still don't have an answer to this question:
Why is it preferable to
<tree id="synchronizeTree"
treelines="true"
flex="1"
hidecolumnpicker="true"
- datasources="rdf:msgaccountmanager rdf:mailnewsfolders"
- ref="msgaccounts:/"
- (much more stuff)
+ onkeypress="gOffline.onKeyPress(event);"
+ onclick="gOffline.onClick(event);">
instead of just saving nsMsgFolderDataSource.cpp from imminent bustage due to the removal of nsISupportsArray by using some other array type?
There are only 12 matches on nsISupportsArray in that file, some apparently in functions which aren't used. So why not just fix the functions which are used now? I think this is necessary to do in bug 1318806 since we won't get this bug here landed before the bustage arrives next week. If were busted, I will land https://hg.mozilla.org/try-comm-central/rev/6532dd6d32ef3488aff0911c3d6066e19e0ca659 but I'd prefer to fix the parts of nsMsgFolderDataSource.cpp we need *now*. Is it really so hard to use some other array type and make sure msgSelectOffline.xul still interfaces correctly?
Can I get an answer?
Comment on attachment 8843641 [details] [diff] [review]
WIP patch v2
Paenglab, can you please also check this if it looses any visual features of the old dialog? I noticed e.g. that on the old version there were lines connecting the parent folder and its children. This seems to be lost.
Also you can decide if we want to show table headers in any of the dialogs.
The saved search folder does not have headers, but the Offline->Synchronize dialog has headers (without the patch, with this patch it is broken yet).
Maybe we want to unify the appearance?
I've also noticed (in a debug build) how slow the old dialog was. Ticking the folders and then clicking OK is so slow that I can see how the selection ticks are removed (shown back as unticked) as part of closing the dialog. And then slowly the "X folders chosen" updates in the parent dialog.
This does not happen in the new version, closing the dialog is immediate.
Attachment #8843641 -
Flags: feedback?(richard.marti)
Comment 8•8 years ago
|
||
Comment on attachment 8843641 [details] [diff] [review]
WIP patch v2
I'm not sure which exact feedback you're after. I tried
File > New > Saved Search, Choose button
and
Right-click unified folder, Properties, Choose button.
Both still works. I have a userChrome.css that puts different background for odd/even rows in the thread pane (zebra stripes). These stripes also show in those panels without and with your patch. So we haven't lost anything.
I'm not sure whether the folder hierarchy looks slightly different. Anwyay: f+
Attachment #8843641 -
Flags: feedback?(jorgk) → feedback+
Comment 9•8 years ago
|
||
Comment on attachment 8843641 [details] [diff] [review]
WIP patch v2
For me this looks good.
Like Jörg wrote, the offline dialog is empty. But this is only a WIP.
About the treelines. Is this only a attribute you have to set when calling the tree?
About the treeheaders: We should show them on all such dialogs to be consistent.
Attachment #8843641 -
Flags: feedback?(richard.marti) → feedback+
Assignee | ||
Comment 10•8 years ago
|
||
(In reply to Jorg K (GMT+1) from comment #6)
> There are only 12 matches on nsISupportsArray in that file, some apparently
> in functions which aren't used. So why not just fix the functions which are
> used now? I think this is necessary to do in bug 1318806 since we won't get
> this bug here landed before the bustage arrives next week. If were busted, I
> will land
> https://hg.mozilla.org/try-comm-central/rev/
> 6532dd6d32ef3488aff0911c3d6066e19e0ca659 but I'd prefer to fix the parts of
> nsMsgFolderDataSource.cpp we need *now*.
Yes, we can do this as first step in bug 1318806.
The bug here is more for the progress of TB than just bustage fix. In line with bug 1318806 comment 11 and bug 420506 this looses some rdf uses in TB. If just after fixing this bug we can compile out a 80KB C++ file (nsMsgFolderDataSource.cpp) from TB that should be a big benefit.
> Is it really so hard to use some
> other array type and make sure msgSelectOffline.xul still interfaces
> correctly?
I don't think msgSelectOffline.xul is any different from virtualFolderListDialog.xul I just didn't update that part of the patch yet.
Anyway, I don't think any of these files uses the *Command and *Folder functions from nsMsgFolderDataSource.cpp.
Yes, instead of ifdeffing out parts of those functions using nsISupportsArray in nsMsgFolderDataSource.cpp we could convert to another type of array. Please list the callers of those functions so we can convert them. Nobody could provide them so far.
Assignee | ||
Comment 11•8 years ago
|
||
(In reply to Jorg K (GMT+1) from comment #8)
> I'm not sure which exact feedback you're after. I tried
> File > New > Saved Search, Choose button
> and
> Right-click unified folder, Properties, Choose button.
>
> Both still works. I have a userChrome.css that puts different background for
> odd/even rows in the thread pane (zebra stripes). These stripes also show in
> those panels without and with your patch.
Great, thanks.
> So we haven't lost anything.
> I'm not sure whether the folder hierarchy looks slightly different.
I see it lost the dotted lines connecting the folders.
> Anwyay: f+
Thanks.
Assignee | ||
Comment 12•8 years ago
|
||
(In reply to Richard Marti (:Paenglab) from comment #9)
> Like Jörg wrote, the offline dialog is empty. But this is only a WIP.
Yes, the offline dialog is left broken, but I will fix it in the same way I did for the virtual folder dialog.
So if that one works for both of you, I can continue.
> About the treelines. Is this only a attribute you have to set when calling
> the tree?
I don't know how that effect it achieved in RDF. I think inspecting a tree is hard (does not even work).
It tried to preserve the attributes of tree cells the original patch was setting. So maybe the original patch lost the dotted lines?
If the idea here is to reuse the folder pane code, then the appearance may also be the same. So if we do not need dotted lines in folder pane, maybe not here either.
> About the treeheaders: We should show them on all such dialogs to be
> consistent.
Thanks, will do.
Whiteboard: [patchlove][has draft patch]
Comment 13•8 years ago
|
||
(In reply to :aceman from comment #12)
> (In reply to Richard Marti (:Paenglab) from comment #9)
> > About the treelines. Is this only a attribute you have to set when calling
> > the tree?
>
> I don't know how that effect it achieved in RDF. I think inspecting a tree
> is hard (does not even work).
> It tried to preserve the attributes of tree cells the original patch was
> setting. So maybe the original patch lost the dotted lines?
> If the idea here is to reuse the folder pane code, then the appearance may
> also be the same. So if we do not need dotted lines in folder pane, maybe
> not here either.
If you can figure out easily, okay. But don't dig too deep. The folder tree is normally not so complicated as in message pane the messages can be, and thus not so important.
Comment 14•8 years ago
|
||
(In reply to Richard Marti (:Paenglab) from comment #9)
> Like Jörg wrote, the offline dialog is empty. But this is only a WIP.
I'm confused. I didn't write anything about an offline dialogue, I don't even know where that would be.
> About the treelines. Is this only a attribute you have to set when calling
> the tree?
What are tree lines? Comparing the local build with a Daily, the dialogue looks 99% the same, the difference comes from a different profile. Even the zebra stripes are honoured.
Comment 15•8 years ago
|
||
Sorry it was aceman in comment 7.
The dialog is in account manager in "Synchronization & Storage" the button at the top. It exists in IMAP and news accounts.
Comment 16•8 years ago
|
||
OK, I found that dialogue, thanks. Last remaining question: What about the "treelines"? Is anything missing from the new dialogue? I can't see anything missing.
Comment 17•8 years ago
|
||
I looks like nothing is missing. For the treelines see comment 13.
Assignee | ||
Comment 18•8 years ago
|
||
Jorg, you have the dotted lines in the screenshot. If that is with my patch, then all is great. I'll check why I am not seeing them.
I just add column headers.
Comment 19•8 years ago
|
||
Comment on attachment 8843641 [details] [diff] [review]
WIP patch v2
Review of attachment 8843641 [details] [diff] [review]:
-----------------------------------------------------------------
::: mail/base/content/msgSelectOffline.js
@@ +4,5 @@
> +
> +var gOffline = {
> + load: function() {
> + let oldProps = ftvItem.prototype.getProperties;
> + ftvItem.prototype.getProperties = function newFTVProps(aProps, aCol) {
no need to name functions anymore
Attachment #8843641 -
Flags: feedback?(mkmelin+mozilla)
Comment 20•8 years ago
|
||
(In reply to :aceman from comment #18)
> Jorg, you have the dotted lines in the screenshot. If that is with my patch,
> then all is great.
Yes, I do. Yes, with your patch (double-checked, IMAP offline folders panel blank). Yes, all great ;-)
Assignee | ||
Comment 21•8 years ago
|
||
I also see the dotted lines today and Paenglab too. It seems like something got fixed in m-c just now.
So that's great. Paenglab even found out what toggles the lines.
The question now is whether we want the lines in these 2 dialogs when the main folder pane doesn't have them.
I would suggest we make them the same (loosing the lines) but make a bug for controlling the lines via a pref in all folder trees. The lines do look nice for me, but I understand they may be too distracting and also FF does not have them in trees. So the default would be off.
Assignee | ||
Comment 22•8 years ago
|
||
(In reply to Magnus Melin from comment #19)
> no need to name functions anymore
Thanks. I updated constructs like this but missed this occurrence. I also converted 'for each', use Set() and Map() now.
Assignee | ||
Comment 23•8 years ago
|
||
This is a test that would catch breakage caused by https://hg.mozilla.org/try-comm-central/rev/d261a738bdad6e111e382161aeeaa999c76bc3bf .
Now with the patch here, it passes even with https://hg.mozilla.org/try-comm-central/rev/d261a738bdad6e111e382161aeeaa999c76bc3bf as the rdf functions there are no longer used.
It is WIP as I still want to add check for the other dialog.
Assignee | ||
Comment 24•8 years ago
|
||
This converts the other dialog (File->Offline->Download/Sync now).
Open problems:
1. If there are RSS folders in the list (in the saved search dialog), I get this error.
01:36:47.791 TypeError: aWindow.specialTabs is undefined 1 FeedUtils.jsm:930:5
getFavicon resource:///modules/FeedUtils.jsm:930:5
getImageSrc/< chrome://messenger/content/folderPane.js:953:7
chooseFoldersToSearch chrome://messenger/content/virtualFolderProperties.js:226:16
Alta88, can you please look at that? Is it important to call that code? Or can we somehow disable it for this type of folder tree (just display without any of the dynamic features).
2. In the Offline dialog, the column with the checkboxes is centered horizontally. In the saved search dialog it is left justified.
Paenglab, can you see where this is set?
I have also disabled the dotted lines for now. I'm not sure the tree looks good now in this dialog. Please play with it and also check my previous comment about the lines.
3. In the offline dialog, there are now checkboxes also for server rows. In the old dialog there weren't (you probably can't sync the whole server).
Attachment #347978 -
Attachment is obsolete: true
Attachment #8843641 -
Attachment is obsolete: true
Attachment #8844252 -
Flags: feedback?(richard.marti)
Attachment #8844252 -
Flags: feedback?(alta88)
Comment 25•8 years ago
|
||
Comment on attachment 8844252 [details] [diff] [review]
WIP patch v3
Well, you can:
1. Include chrome://messenger/content/specialTabs.js in the dialog xul, or
2. Test the dialog name and exit early in getImageSrc().
I think 1 is nicer, but it would get the icons each time it's opened due to how you're doing this. In feed Subscribe dialog, for example, it checks the main window folderpane cache first (and even updates the main cache if necessary) but it's different view logic:
https://dxr.mozilla.org/comm-central/rev/8be1ae77d2644e401fa6480a05ff234e70d0ccdf/mailnews/extensions/newsblog/content/feed-subscriptions.js#318.
Nice rdf kill here.
Attachment #8844252 -
Flags: feedback?(alta88)
Assignee | ||
Comment 26•8 years ago
|
||
(In reply to alta88 from comment #25)
> 1. Include chrome://messenger/content/specialTabs.js in the dialog xul, or
> 2. Test the dialog name and exit early in getImageSrc().
Thanks. So I can just exit before the FeedUtils.getFavicon() call in getImageSrc. I can test the "simplelist" attribute to know when to not run these async fetches or other dynamic actions. So if icons are not fetched, RSS folders will get some generic icon. I think that would be OK.
> I think 1 is nicer, but it would get the icons each time it's opened due to
> how you're doing this.
Yes, I'd probably prefer to not run any background tasks or network fetches from these simple dialogs.
Comment 27•8 years ago
|
||
(In reply to :aceman from comment #24)
> Created attachment 8844252 [details] [diff] [review]
> WIP patch v3
> 2. In the Offline dialog, the column with the checkboxes is centered
> horizontally. In the saved search dialog it is left justified.
>
> Paenglab, can you see where this is set?
> I have also disabled the dotted lines for now. I'm not sure the tree looks
> good now in this dialog. Please play with it and also check my previous
> comment about the lines.
The cycler="true" is needed in the treecol. See attached patch.
Updated•8 years ago
|
Attachment #8844252 -
Flags: feedback?(richard.marti) → feedback+
Assignee | ||
Comment 28•8 years ago
|
||
A rudimentary test that that the folder trees in the dialogs are not empty. Would catch breakage cased by disabling nsMsgFolderDatasource.cpp prematurely.
The tests will be useful in the future if we break folderPane.js by adding some features, that will work in folder pane, but throw in these offline and virtual folder dialogs. As some features are to be disabled in them.
Attachment #8844247 -
Attachment is obsolete: true
Attachment #8846282 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Comment 29•8 years ago
|
||
This is the main patch. It converts both dialogs and fixes remaining problems mentioned in comment 24. It also has some improvements, e.g. that you can select multiple folders and toggle them with a single press of space key.
I think we need review from TB and SM sides here. I see that all works in TB, however it needs check if I haven't broken SM in the process. Also, I have changed the location of the files compared to Joey's patch. I put all the files in /mailnews so that SM can "easily" use them once they are ready for it (easily once they adopt folderPane.js).
Also the packaging of virtualFolderListDialog.* is an open question. The file names in the package are the same for TB and SM but the source files are different. This is to avoid need to change the callers.
+#ifndef MOZ_THUNDERBIRD
content/messenger/virtualFolderListDialog.xul (base/content/virtualFolderListDialog.xul)
content/messenger/virtualFolderListDialog.js (base/content/virtualFolderListDialog.js)
+#else
+ content/messenger/virtualFolderListDialog.xul (base/content/virtualFolderListDialogJS.xul)
+ content/messenger/virtualFolderListDialog.js (base/content/virtualFolderListDialogJS.js)
+#endif
Attachment #8844252 -
Attachment is obsolete: true
Attachment #8846284 -
Flags: review?(mkmelin+mozilla)
Attachment #8846284 -
Flags: review?(frgrahl)
Assignee | ||
Comment 30•8 years ago
|
||
This is the code to disable building nsMsgFolderDataSource.* for TB. I understand now that the remaining callers of datasource=rdf:mailnewsfolders are converted, we could disable this to save some compile time and runtime footprint (the files are ~100KB of c++ in total).
There is a try run at
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=f57fd498a0c7ff1826ee31893b010dfd9582d4fc .
It contains all the patches here. From my previous work, there is no test in current testsuite that would catch if the patch here is wrong. Only the new test would catch it in the 2 converted dialog.
Attachment #8844557 -
Attachment is obsolete: true
Attachment #8846285 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Comment 31•8 years ago
|
||
Found some more now-unused defines.
Attachment #8846287 -
Flags: review?(mkmelin+mozilla)
Comment 32•8 years ago
|
||
Comment on attachment 8846284 [details] [diff] [review]
patch v4
I am redirecting the review to IanN. I am not so familiar with mailnews. Nevertheless I will check the patch if something breaks.
Attachment #8846284 -
Flags: review?(iann_bugzilla)
Attachment #8846284 -
Flags: review?(frgrahl)
Attachment #8846284 -
Flags: feedback?(frgrahl)
Comment 33•8 years ago
|
||
aceman could you check if the patch applies to a clean tree?
The hunk for mailnews/base/content/msgSelectOffline.xul does not apply for me.
Flags: needinfo?(acelists)
Assignee | ||
Comment 34•8 years ago
|
||
Sorry, you need to apply the "test v2" patch first.
Flags: needinfo?(acelists)
Comment 35•8 years ago
|
||
Comment on attachment 8846284 [details] [diff] [review]
patch v4
Sorry no cookies. Unable to compile SeaMonkey with the patch:
ValueError: Item already in manifest: chrome/messenger/content/messenger/msgSelectOffline.xul
mailnews\base\content\msgSelectOffline.xul clashes with suite\mailnews\msgSelectOffline.xul
Any idea how hard it would be to get rid of the rdf in suite too? Maybe I can do it so that the TB ifdefs can be removed.
Attachment #8846284 -
Flags: feedback?(frgrahl) → feedback-
Assignee | ||
Comment 36•8 years ago
|
||
(In reply to Frank-Rainer Grahl from comment #35)
> Comment on attachment 8846284 [details] [diff] [review]
> patch v4
>
> Sorry no cookies. Unable to compile SeaMonkey with the patch:
>
> ValueError: Item already in manifest:
> chrome/messenger/content/messenger/msgSelectOffline.xul
>
> mailnews\base\content\msgSelectOffline.xul clashes with
> suite\mailnews\msgSelectOffline.xul
Sorry, didn't notice SM also has this file.
You can change the patch in jar.mn to:
+#ifdef MOZ_THUNDERBIRD
+ content/messenger/msgSelectOffline.xul (base/content
/msgSelectOffline.xul)
+ content/messenger/msgSelectOffline.js (base/content/msgSelectOffline.js)
+#endif
> Any idea how hard it would be to get rid of the rdf in suite too?
For this particular patch, you would need to adopt mail/base/content/folderPane.js. It implements the main folder pane without RDF.
Note TB didn't yet remove all of RDF, some remnants are left in subscription management.
For removing all of RDF, there is bug 420506. Note the 700KB patch which is a start, but some parts of it are already landed and some are split into bugs like the one here.
Comment 37•8 years ago
|
||
Bug 420506 is interesting reading, together with https://wiki.mozilla.org/Thunderbird:Thoughts_on_Removing_RDF. So why is RDF (https://en.wikipedia.org/wiki/Resource_Description_Framework) bad and why do we want to remove it?
Assignee | ||
Comment 38•8 years ago
|
||
Potential fix for SM build.
Attachment #8846284 -
Attachment is obsolete: true
Attachment #8846284 -
Flags: review?(mkmelin+mozilla)
Attachment #8846284 -
Flags: review?(iann_bugzilla)
Attachment #8846436 -
Flags: review?(mkmelin+mozilla)
Attachment #8846436 -
Flags: review?(iann_bugzilla)
Assignee | ||
Comment 39•8 years ago
|
||
moz.build has a different syntax, #ifdef does nothing in it. So this version really compiles out nsMsgFolderDataSource.cpp from libxul.so/dll. No more symbols found in the file. Size of binary libxul.so (debug on linux) shrinks by 200K out of 689500K :)
Attachment #8846285 -
Attachment is obsolete: true
Attachment #8846287 -
Attachment is obsolete: true
Attachment #8846285 -
Flags: review?(mkmelin+mozilla)
Attachment #8846287 -
Flags: review?(mkmelin+mozilla)
Attachment #8846437 -
Flags: review?(mkmelin+mozilla)
Comment 40•8 years ago
|
||
(In reply to Jorg K (GMT+1) from comment #37)
> Bug 420506 is interesting reading, together with
> https://wiki.mozilla.org/Thunderbird:Thoughts_on_Removing_RDF. So why is RDF
> (https://en.wikipedia.org/wiki/Resource_Description_Framework) bad and why
> do we want to remove it?
It's not that RDF is bad per se, it's great for what it's intended and has no real competitors there. The problem is it was never implemented properly in mozilla and the parts that were implemented were not up to current standards. It's also not really intended to be used the way we use it - so all in all you have hard to work with code, that's buggy and also scheduled for removal in m-c.
Assignee | ||
Comment 41•8 years ago
|
||
OS X always has to be special. Main menu doesn't work on it, so use the appmenu.
I had to allow operating a splitmenu element (click the small arrow, not the main button) in click_menus_in_sequence for that to work.
Try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=334868c276e4e8af704dbc20362035d439140f48
Attachment #8846282 -
Attachment is obsolete: true
Attachment #8846282 -
Flags: review?(mkmelin+mozilla)
Attachment #8846455 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Comment 42•8 years ago
|
||
We can also loose nsMsgAccountManagerDS.cpp for TB. Another 200K shaved.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=334868c276e4e8af704dbc20362035d439140f48
Attachment #8846437 -
Attachment is obsolete: true
Attachment #8846437 -
Flags: review?(mkmelin+mozilla)
Attachment #8846456 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Comment 43•8 years ago
|
||
I think I'm done here. Please review :)
Comment 44•8 years ago
|
||
Comment on attachment 8846456 [details] [diff] [review]
disable rdf:mailnewsfolders and rdf:msgaccountmanager for TB v2
SeaMonkeys configure chokes on the current patch. It looks like the file list needs to be in alphabetical order.
Tried this which seems to work:
> + SOURCES += ['nsMsgAccountManagerDS.cpp',
> + 'nsMsgFolderDataSource.cpp',
> + ]
Attachment #8846456 -
Flags: feedback-
Assignee | ||
Comment 45•8 years ago
|
||
Thanks for checking.
Attachment #8846456 -
Attachment is obsolete: true
Attachment #8846456 -
Flags: review?(mkmelin+mozilla)
Attachment #8848765 -
Flags: review?(mkmelin+mozilla)
Attachment #8848765 -
Flags: review?(frgrahl)
Comment 46•8 years ago
|
||
Comment on attachment 8848765 [details] [diff] [review]
disable rdf:mailnewsfolders and rdf:msgaccountmanager for TB v2.1
SeaMonkey compiles fine
Attachment #8848765 -
Flags: review?(frgrahl) → review+
Comment 47•8 years ago
|
||
Comment on attachment 8846436 [details] [diff] [review]
patch v4.1
Seems correct for the SM parts r=me
Attachment #8846436 -
Flags: review?(iann_bugzilla) → review+
Assignee | ||
Comment 48•7 years ago
|
||
Magnus, can you please check the patches here?
Flags: needinfo?(mkmelin+mozilla)
Assignee | ||
Comment 49•7 years ago
|
||
This is getting more urgent now due to bug 1425962. Magnus, can you look at this in the near future to avoid bustage?
Blocks: 1425962
Assignee | ||
Comment 51•7 years ago
|
||
Refreshed patch.
Attachment #8846436 -
Attachment is obsolete: true
Attachment #8846436 -
Flags: review?(mkmelin+mozilla)
Attachment #8938688 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Comment 52•7 years ago
|
||
Refreshed patch.
Attachment #8848765 -
Attachment is obsolete: true
Attachment #8848765 -
Flags: review?(mkmelin+mozilla)
Attachment #8938689 -
Flags: review?(mkmelin+mozilla)
Comment 53•7 years ago
|
||
Comment on attachment 8938689 [details] [diff] [review]
disable rdf:mailnewsfolders and rdf:msgaccountmanager for TB v2.2
Review of attachment 8938689 [details] [diff] [review]:
-----------------------------------------------------------------
::: mailnews/build/nsMailModule.cpp
@@ +63,2 @@
> #include "nsMsgFolderDataSource.h"
> #include "nsMsgAccountManagerDS.h"
can we just move these into suite/ somewhere?
Attachment #8938689 -
Flags: review?(mkmelin+mozilla) → review+
Comment 54•7 years ago
|
||
Comment on attachment 8938688 [details] [diff] [review]
patch v4.2
Review of attachment 8938688 [details] [diff] [review]:
-----------------------------------------------------------------
::: mail/locales/en-US/chrome/messenger/virtualFolderListDialog.dtd
@@ +4,5 @@
>
> <!ENTITY virtualFolderListTitle.title "Select Folder(s)">
> <!ENTITY virtualFolderDesc.label "Select the folders to search:">
> +<!ENTITY folderName.label "Folder name">
> +<!ENTITY folderSearch.label "Search">
please just drop the spaces...
::: mailnews/base/content/msgSelectOffline.js
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +var msgWindow = null;
what's this for?
::: mailnews/base/content/virtualFolderListDialog.js
@@ +6,5 @@
> Components.utils.import("resource:///modules/mailServices.js");
> Components.utils.import("resource:///modules/iteratorUtils.jsm");
> Components.utils.import("resource:///modules/MailUtils.js");
>
> +// All of this is no longer used by Thunderbird.
move to suite then?
::: mailnews/base/content/virtualFolderListDialogJS.js
@@ +24,5 @@
> + }
> + }
> +
> + // Now tweak the folder tree for our purposes here.
> + let oldProps = ftvItem.prototype.getProperties;
Seems to me you should make a subclass of ftvItem and override getProperties to do what you want. That would also make it clearer where the ftvItem suddenly came from....
(Same thing earlier.)
Comment 55•7 years ago
|
||
> can we just move these into suite/ somewhere?
The template removals will break suite hard anyway and we need to follow so I think just remove them and the thunderbird only ifdefs.
Updated•7 years ago
|
Attachment #8846455 -
Flags: review?(mkmelin+mozilla) → review+
Assignee | ||
Comment 56•7 years ago
|
||
(In reply to Frank-Rainer Grahl (:frg) from comment #55)
> > can we just move these into suite/ somewhere?
>
> The template removals will break suite hard anyway and we need to follow so
> I think just remove them and the thunderbird only ifdefs.
You mean remove the files and break SM totally? Don't you need to look at the files to reimplement the needed functionality? Or will you just migrate the TB's folderpane.js one day to SM to get the functionality already done?
(In reply to Magnus Melin from comment #53)
> ::: mailnews/build/nsMailModule.cpp
> @@ +63,2 @@
> > #include "nsMsgFolderDataSource.h"
> > #include "nsMsgAccountManagerDS.h"
>
> can we just move these into suite/ somewhere?
Probably I didn't want to move them in case we find out we actually need them in TB. There are some more RDF and XUL template users in bug 1425962 left.
Comment 57•7 years ago
|
||
> You mean remove the files and break SM totally?
Yes
> Don't you need to look at the files to reimplement the needed functionality?
Still in the history and I am keeping a working 2.53 / 56 up. 57+/2.54 is not much use with all the broken add-ons and other stuff.
> Or will you just migrate the TB's folderpane.js one day to SM to get the functionality already done?
That would be my plan.
IanN, stefanh what do you think?
Flags: needinfo?(stefanh)
Flags: needinfo?(iann_bugzilla)
Updated•7 years ago
|
Attachment #8938688 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Comment 58•7 years ago
|
||
(In reply to Magnus Melin from comment #54)
> ::: mailnews/base/content/virtualFolderListDialogJS.js
> > + // Now tweak the folder tree for our purposes here.
> > + let oldProps = ftvItem.prototype.getProperties;
>
> Seems to me you should make a subclass of ftvItem and override getProperties
> to do what you want. That would also make it clearer where the ftvItem
> suddenly came from....
> (Same thing earlier.)
How is this done?
Flags: needinfo?(mkmelin+mozilla)
Comment 59•7 years ago
|
||
One way would be something like
function VirtualFtvItem() {
this.getProperties = function(aColumn) {
// go!
}
}
VirtualFtvItem.prototype = new ftvItem();
Flags: needinfo?(mkmelin+mozilla)
Comment 60•7 years ago
|
||
You guys have waited so long with this bug that I'll have to land it all as bustage-fix now and you need to sort out the nits later.
Meanwhile, here's a try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=ce378b47d48d72813e768e7bdf4b3b19d2e17308
Keywords: leave-open
Assignee | ||
Comment 61•7 years ago
|
||
(In reply to Magnus Melin from comment #59)
> function VirtualFtvItem() {
> this.getProperties = function(aColumn) {
> // go!
> }
> }
> VirtualFtvItem.prototype = new ftvItem();
I couldn't get this to work so I used this:
function ModifiedFtvItem(aFolder, aFolderFilter) {
ftvItem.call(this, aFolder, aFolderFilter);
}
ModifiedFtvItem.prototype = {
__proto__: ftvItem.prototype,
getProperties: function(aColumn) {
if (!aColumn || aColumn.id != "syncCol")
return ftvItem.prototype.getProperties.call(this, aColumn);
let properties = "syncCol";
if (this._folder.isServer)
return " isServer-true";
if (this._folder.getFlag(Ci.nsMsgFolderFlags.Offline))
properties += " synchronize-true";
return properties;
}
}
return accounts.map(acct => new ModifiedFtvItem(acct.incomingServer.rootFolder,
filterOffline));
However, this doesn't work as this only generates the top level (server) rows. Then when the next level of children (folders) is generated inside folderPane.js it creates new ftvItems() for them and not my ModifiedFtvItems. SO that my be the reason why we have to modify the original ftvItem() directly.
Assignee | ||
Comment 62•7 years ago
|
||
Fixes nits from Magnus, except the subclassing. Also following frg's suggestion this uses the new files also for Seamonkey, but it will not work there for now as it doesn't have folderPane.js.
Attachment #8938688 -
Attachment is obsolete: true
Attachment #8940009 -
Flags: review?(mkmelin+mozilla)
Attachment #8940009 -
Flags: review?(frgrahl)
Assignee | ||
Comment 63•7 years ago
|
||
As suggested, this removes the *DataSource* files completely and also the related defines and contracts.
Attachment #8938689 -
Attachment is obsolete: true
Attachment #8940010 -
Flags: review?(frgrahl)
Assignee | ||
Comment 64•7 years ago
|
||
Removes the nsIMsgFolder::SetInVFEditSearchScope method now that the single user is removed (virtualfolderlistdialog)
Attachment #8940012 -
Flags: review?(mkmelin+mozilla)
Attachment #8940012 -
Flags: review?(frgrahl)
Assignee | ||
Comment 65•7 years ago
|
||
Renamed the virtualFolderList files to make the patch more readable.
Attachment #8940009 -
Attachment is obsolete: true
Attachment #8940009 -
Flags: review?(mkmelin+mozilla)
Attachment #8940009 -
Flags: review?(frgrahl)
Attachment #8940037 -
Flags: review?(mkmelin+mozilla)
Attachment #8940037 -
Flags: review?(frgrahl)
Assignee | ||
Comment 66•7 years ago
|
||
Rename also msgSelectOffline.*
Attachment #8940037 -
Attachment is obsolete: true
Attachment #8940037 -
Flags: review?(mkmelin+mozilla)
Attachment #8940037 -
Flags: review?(frgrahl)
Attachment #8940042 -
Flags: review?(mkmelin+mozilla)
Attachment #8940042 -
Flags: review?(frgrahl)
Comment 67•7 years ago
|
||
Sadly we got ourselves in quite a mess here.
The test patch modifies a file which is later removed. That's unnecessary.
The SetInVFEditSearchScope patch tries to patch a file that has been renamed (virtualFolderListDialog.js -> virtualFolderListEdit.js).
Just shows that working at 3 AM is error-prone :-(
Comment 68•7 years ago
|
||
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/afc58e5a8f81
Add simple test to check if trees in msgSelectOfflineFolders.xul and virtualFolderListEdit.xul work. r=mkmelin
https://hg.mozilla.org/comm-central/rev/f4140f045ad0
Use folderPane.js to convert RDF trees in msgSelectOffline.xul and virtualFolderListDialog.xul to JS (most of conversion done by Joey Minta). r=mkmelin,frg rs=bustage-fix
https://hg.mozilla.org/comm-central/rev/298602f44434
remove RDF datasources nsMsgFolderDataSource.cpp and nsMsgAccountManagerDS.cpp. r=mkmelin,frg rs=bustage-fix
https://hg.mozilla.org/comm-central/rev/1cfb05317fd6
remove nsIMsgFolder::SetInVFEditSearchScope. r=mkmelin,frg rs=bustage-fix
Comment 69•7 years ago
|
||
I've landed this now with the tweaks discussed in comment #67. It compiled and virtual folder selection appeared to work.
Now it's PLR and follow-ups ;-)
Updated•7 years ago
|
Target Milestone: --- → Thunderbird 59.0
Updated•7 years ago
|
Flags: needinfo?(stefanh)
Comment 70•7 years ago
|
||
Comment on attachment 8940010 [details] [diff] [review]
disable rdf:mailnewsfolders and rdf:msgaccountmanager for TB v3
Broke SeaMonkey as expected so I call it a success. Now let see how long we need to fix it :)
Attachment #8940010 -
Flags: review?(frgrahl) → review+
Updated•7 years ago
|
Attachment #8940012 -
Flags: review?(frgrahl) → review+
Updated•7 years ago
|
Attachment #8940042 -
Flags: review?(frgrahl) → review+
Updated•7 years ago
|
Attachment #8940012 -
Flags: review?(mkmelin+mozilla) → review+
Comment 72•7 years ago
|
||
Comment on attachment 8940042 [details] [diff] [review]
patch v4.5
Review of attachment 8940042 [details] [diff] [review]:
-----------------------------------------------------------------
Ok, thanks for the explanation.
I suppose another option would have been to not bail out in ftvItem getProperties if the name isn't folderNameCol, and have the functionality in there.
Attachment #8940042 -
Flags: review?(mkmelin+mozilla) → review+
Comment 73•7 years ago
|
||
(In reply to Frank-Rainer Grahl (:frg) from comment #57)
> > You mean remove the files and break SM totally?
>
> Yes
>
> > Don't you need to look at the files to reimplement the needed functionality?
>
> Still in the history and I am keeping a working 2.53 / 56 up. 57+/2.54 is
> not much use with all the broken add-ons and other stuff.
>
> > Or will you just migrate the TB's folderpane.js one day to SM to get the functionality already done?
>
> That would be my plan.
>
> IanN, stefanh what do you think?
Yes, we need to move away from RDF and so we will end up with something similar to TB's folderpane.js
Flags: needinfo?(iann_bugzilla)
Assignee | ||
Comment 74•7 years ago
|
||
I think we can close this. The other trees like the IMAP/news subscribe dialogs are handled in a different bug (removal of XUL templates).
You need to log in
before you can comment on or make changes to this bug.
Description
•