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)

defect
Not set
normal

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
Attached patch patch v1 (obsolete) (deleted) — 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)
Attachment #347978 - Flags: review?(bienvenu) → review-
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.
Are there any of the other trees that don't expand automatically?
I don't believe there are any that don't expand automatically.
Whiteboard: [patchlove][has draft patch][needs new assignee]
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.
Assignee: nobody → acelists
Blocks: 1318806
Status: NEW → ASSIGNED
Attached patch WIP patch v2 (obsolete) (deleted) — Splinter Review
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)
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 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 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+
(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.
(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.
(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.
Keywords: helpwantedfootprint, perf
Whiteboard: [patchlove][has draft patch]
(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.
(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.
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.
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.
I looks like nothing is missing. For the treelines see comment 13.
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 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)
(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 ;-)
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.
(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.
Attached patch WIP test (obsolete) (deleted) — Splinter Review
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.
Attached patch WIP patch v3 (obsolete) (deleted) — Splinter Review
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 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)
(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.
Attached patch bug464710checkbox.patch (obsolete) (deleted) — Splinter Review
(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.
Attachment #8844252 - Flags: feedback?(richard.marti) → feedback+
Blocks: 1345919
Attached patch test v2 (obsolete) (deleted) — Splinter Review
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)
Attached patch patch v4 (obsolete) (deleted) — Splinter Review
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)
Attached patch disable rdf:mailnewsfolders for for TB (obsolete) (deleted) — Splinter Review
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)
Attached patch disable rdf:mailnewsfolders for TB v1.1 (obsolete) (deleted) — Splinter Review
Found some more now-unused defines.
Attachment #8846287 - Flags: review?(mkmelin+mozilla)
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)
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)
Sorry, you need to apply the "test v2" patch first.
Flags: needinfo?(acelists)
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-
(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.
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?
Attached patch patch v4.1 (obsolete) (deleted) — Splinter Review
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)
Attached patch disable rdf:mailnewsfolders for TB v1.2 (obsolete) (deleted) — Splinter Review
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)
(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.
Attached patch test v3 (deleted) — Splinter Review
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)
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)
I think I'm done here. Please review :)
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-
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 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 on attachment 8846436 [details] [diff] [review] patch v4.1 Seems correct for the SM parts r=me
Attachment #8846436 - Flags: review?(iann_bugzilla) → review+
Magnus, can you please check the patches here?
Flags: needinfo?(mkmelin+mozilla)
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
Yeah will have a look soon
Flags: needinfo?(mkmelin+mozilla)
Attached patch patch v4.2 (obsolete) (deleted) — Splinter Review
Refreshed patch.
Attachment #8846436 - Attachment is obsolete: true
Attachment #8846436 - Flags: review?(mkmelin+mozilla)
Attachment #8938688 - Flags: review?(mkmelin+mozilla)
Refreshed patch.
Attachment #8848765 - Attachment is obsolete: true
Attachment #8848765 - Flags: review?(mkmelin+mozilla)
Attachment #8938689 - Flags: review?(mkmelin+mozilla)
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 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.)
> 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.
Attachment #8846455 - Flags: review?(mkmelin+mozilla) → review+
(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.
> 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)
Attachment #8938688 - Flags: review?(mkmelin+mozilla)
(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)
One way would be something like function VirtualFtvItem() { this.getProperties = function(aColumn) { // go! } } VirtualFtvItem.prototype = new ftvItem();
Flags: needinfo?(mkmelin+mozilla)
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
(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.
Attached patch patch v4.3 (obsolete) (deleted) — Splinter Review
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)
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)
Attached patch remove SetInVFEditSearchScope (deleted) — Splinter Review
Removes the nsIMsgFolder::SetInVFEditSearchScope method now that the single user is removed (virtualfolderlistdialog)
Attachment #8940012 - Flags: review?(mkmelin+mozilla)
Attachment #8940012 - Flags: review?(frgrahl)
Attached patch patch v4.4 (obsolete) (deleted) — Splinter Review
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)
Attached patch patch v4.5 (deleted) — Splinter Review
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)
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 :-(
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
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 ;-)
Target Milestone: --- → Thunderbird 59.0
Flags: needinfo?(stefanh)
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+
Attachment #8940012 - Flags: review?(frgrahl) → review+
Attachment #8940042 - Flags: review?(frgrahl) → review+
No longer blocks: 1345919
Attachment #8940012 - Flags: review?(mkmelin+mozilla) → review+
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+
(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)
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).
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: