Use folderPane.js to convert other rdf-trees to js

RESOLVED FIXED in Thunderbird 59.0

Status

defect
RESOLVED FIXED
11 years ago
a year ago

People

(Reporter: jminta, Assigned: aceman)

Tracking

(Blocks 3 bugs, {memory-footprint, perf})

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments, 17 obsolete attachments)

13.50 KB, image/png
Details
14.81 KB, patch
mkmelin
: review+
Details | Diff | Splinter Review
151.60 KB, patch
frg
: review+
Details | Diff | Splinter Review
11.27 KB, patch
mkmelin
: review+
frg
: review+
Details | Diff | Splinter Review
54.90 KB, patch
mkmelin
: review+
frg
: review+
Details | Diff | Splinter Review
Reporter

Description

11 years ago
Posted patch patch v1 (obsolete) — 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

11 years ago
Attachment #347978 - Flags: review?(bienvenu) → review-

Comment 1

11 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.
Are there any of the other trees that don't expand automatically?

Comment 3

11 years ago
I don't believe there are any that don't expand automatically.

Updated

8 years ago
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]
Assignee

Comment 4

2 years ago
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
Assignee

Comment 5

2 years ago
Posted patch WIP patch v2 (obsolete) — 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)

Comment 6

2 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?
Assignee

Comment 7

2 years ago
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

2 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 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

2 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

2 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

2 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.
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.

Comment 14

2 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.
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

2 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.
I looks like nothing is missing. For the treelines see comment 13.
Assignee

Comment 18

2 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 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

2 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

2 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

2 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

2 years ago
Posted patch WIP test (obsolete) — 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.
Assignee

Comment 24

2 years ago
Posted patch WIP patch v3 (obsolete) — 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 25

2 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

2 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.
Posted patch bug464710checkbox.patch (obsolete) — 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+
Assignee

Updated

2 years ago
Blocks: 1345919
Assignee

Comment 28

2 years ago
Posted patch test v2 (obsolete) — 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)
Assignee

Comment 29

2 years ago
Posted patch patch v4 (obsolete) — 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)
Assignee

Comment 30

2 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

2 years ago
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)
Assignee

Comment 34

2 years ago
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-
Assignee

Comment 36

2 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

2 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

2 years ago
Posted patch patch v4.1 (obsolete) — 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)
Assignee

Comment 39

2 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)
(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

2 years ago
Posted patch test v3Splinter 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)
Assignee

Comment 42

2 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

2 years ago
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-
Assignee

Comment 45

2 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 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

2 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

2 years ago
Magnus, can you please check the patches here?
Flags: needinfo?(mkmelin+mozilla)
Assignee

Comment 49

a year 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
Yeah will have a look soon
Flags: needinfo?(mkmelin+mozilla)
Assignee

Comment 51

a year ago
Posted patch patch v4.2 (obsolete) — Splinter Review
Refreshed patch.
Attachment #8846436 - Attachment is obsolete: true
Attachment #8846436 - Flags: review?(mkmelin+mozilla)
Attachment #8938688 - Flags: review?(mkmelin+mozilla)
Assignee

Comment 52

a year ago
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+
Assignee

Comment 56

a year 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.
> 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)
Assignee

Comment 58

a year 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)
One way would be something like

function VirtualFtvItem() {
  this.getProperties = function(aColumn) {
    // go!
  }
}
VirtualFtvItem.prototype = new ftvItem();
Flags: needinfo?(mkmelin+mozilla)

Comment 60

a year 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

a year 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

a year ago
Posted patch patch v4.3 (obsolete) — 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)
Assignee

Comment 63

a year 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

a year 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

a year ago
Posted patch patch v4.4 (obsolete) — 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)
Assignee

Comment 66

a year ago
Posted patch patch v4.5Splinter 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)

Comment 67

a year 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

a year 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

a year 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

a year ago
Target Milestone: --- → Thunderbird 59.0

Updated

a year ago
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+
Assignee

Updated

a year ago
No longer blocks: 1345919
Duplicate of this bug: 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+

Comment 73

a year 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

a year 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).
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Keywords: leave-open
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.