Closed Bug 1466705 Opened 2 years ago Closed 2 years ago

Fix subscribe dialog for IMAP and news - continuation of bug 1425962

Categories

(MailNews Core :: General, enhancement)

enhancement
Not set

Tracking

(thunderbird_esr60+ fixed, thunderbird60 fixed, thunderbird61 wontfix, thunderbird62 fixed)

RESOLVED FIXED
Thunderbird 62.0
Tracking Status
thunderbird_esr60 + fixed
thunderbird60 --- fixed
thunderbird61 --- wontfix
thunderbird62 --- fixed

People

(Reporter: jorgk-bmo, Assigned: aceman)

References

Details

Attachments

(3 files, 31 obsolete files)

2.38 KB, patch
jorgk-bmo
: review+
Details | Diff | Splinter Review
43.42 KB, patch
jorgk-bmo
: review+
Details | Diff | Splinter Review
44.23 KB, patch
Details | Diff | Splinter Review
+++ This bug was initially created as a clone of Bug #1425962 +++

In bug 1425962 we implemented a basic JS flat list subscribe. We need to fix this now and do a proper C++ tree again.

The approach is to use
https://hg.mozilla.org/users/Pidgeot18_gmail.com/patch-queues/file/tip/patches-derdf/subscribe-invert and https://hg.mozilla.org/users/Pidgeot18_gmail.com/patch-queues/file/tip/patches-derdf/subscribe-tree
as a starting point.
Looks like this needs to be applied first.

No rocket science here, all this does is
- rename class nsSubscribableServer to SubscribableServer
  (which appears unnecessary)
- make SubscribableServer inherit from nsITreeView
  and provide the necessary methods, some of which are just dummies.
I renamed the class back to nsSubscribableServer. That cuts down the patch from 26KB to 14KB and removes needless noise.
White-space tweaks.
Attachment #8984144 - Attachment is obsolete: true
Attachment #8984215 - Attachment is obsolete: true
All hunks failed when applying the original patch. So I transferred all the hunks by hand. The code was really old, so I had to remove use of Atoms.

There are a few questions in the code left. So let's see what it does.
OK, something is working, but I don't see a tree. I get a list if folders, but with the hierarchy separator, like:
INBOX/xyz/t. The folder names are not MUTF-7 decoded, so some look strange. And the check boxes can be clicked.

So somehow we need to split the paths at the separator, decode the names and assemble it all to a tree. The search doesn't work either.

Aceman do you want to have a go? You've go us to the point where the lists are displayed, so perhaps this all makes sense to you.
Tweaked some comments after some experiments.

This actually shows a tree on news.mozilla.org :-)
Attachment #8984241 - Attachment is obsolete: true
This fixes the IMAP MUTF-7 issue. So at least folder names are looking OK now.

Still, the check boxes don't work.
Attachment #8984257 - Attachment is obsolete: true
Depends on: 934872
Attached patch subscribe-tree.patch (obsolete) — Splinter Review
Merged both patches, added authors and commit message.

Aceman said via IRC that for the checkboxes to work we need (to revert?) some CSS changes.

Looking promising. With concentrated effort this could be done in a few days now.
Attachment #8984219 - Attachment is obsolete: true
Attachment #8984266 - Attachment is obsolete: true
Fixed messed-up comment that was accidentally hit by a global replace :-(
Attachment #8984272 - Attachment is obsolete: true
Attached patch subscribe.patch - v1c (obsolete) — Splinter Review
Fixes proper folder icons for news and tries to show full newsgroup names 9still a bit incorrectly). Cleans up some unneeded code.
Attached patch subscribe.patch - v2 (obsolete) — Splinter Review
Removed some commented-out code, played a bit with pulling IMAP folder names apart. I'm still not sure what we need to do to create a tree.
Attachment #8984274 - Attachment is obsolete: true
Attachment #8984308 - Attachment is obsolete: true
Current status:
- IMAP folders don't show up as tree.
- Checkboxes can't be checked.
- News server tree isn't shown properly when panel first loads, arrow indicates expanded, but it isn't.
  Quick fix: Always launch collapsed.
- Some dead JS code from bug 1425962, like createSubscribeTree().
Attached patch subscribe.patch - v3 (obsolete) — Splinter Review
IMAP working now, just a delimiter problem. I'll post an interdiff link in the next comment so you can see it.
Attachment #8984392 - Attachment is obsolete: true
https://bugzilla.mozilla.org/attachment.cgi?oldid=8984308&action=interdiff&newid=8984554&headers=1
Comparing v1c with v3, v2 was just plain wrong.

This is all that was needed for IMAP:
    char hierarchyDelimiter;
    GetDelimiter(&hierarchyDelimiter);
    nsCOMPtr<nsISubscribableServer> listener2 = do_QueryInterface(listener);
    if (listener2)
      listener2->SetDelimiter(hierarchyDelimiter);

Looks like there are two nsISubscribableServer objects being maintained. The second one didn't have the correct delimiter. So here its transferred from one object to the next.
Attached patch subscribe.patch - v3b (obsolete) — Splinter Review
This fixes the folder names for IMAP. So now IMAP and news have the same two common problems:
- Checkboxes can't be checked.
- Server tree isn't shown properly when panel first loads, arrow indicates expanded, but it isn't.
  Quick fix: Always launch collapsed.

Starting to look good. Over to Aceman now.
Attachment #8984554 - Attachment is obsolete: true
Attached patch subscribe.patch - v4 (obsolete) — Splinter Review
This fixes the newsgroup names and also restores the italic font and no checkbox for unsubscribable rows (e.g. expandable nodes that do not represent a real newsgroup).

I also think there are still 2 trees being populated. You sync the delimiter and showFullName between the two. But all the other properties (like isSubscribable and isSubscribed) aren't synced. And that is why the checkboxes aren't ticked for subscribed newsgroups/folders. I have added addresses of the pointers to the node objects into the debugging printfs so that the duplicate objects can be seen.

I don't know why there need to be 2 trees, why there is the 'inner' member and also why the subscribeListener seems to implement the whole tree and why this code looks like a loop:
var inner = Cc["@mozilla.org/messenger/subscribableserver;1"]
              .createInstance(Ci.nsISubscribeListener)
              .QueryInterface(Ci.nsITreeView);
    gSubscribableServer.subscribeListener = inner;
    inner.QueryInterface(Ci.nsISubscribableServer)
         .subscribeListener = MySubscribeListener;
    inner.setIncomingServer(gSubscribableServer);

Also I do not see why listener->AddFolder() calls AddTo() again when that is done explicitly around the call to the AddFolder().
Attachment #8984557 - Attachment is obsolete: true
Yes, agreed, this is a concern. When working on the sync for IMAP, I removed the listener->AddFolder() call and then the tree in the panel remained empty. I think it's a "display" tree and a "shadow" tree, but I don't know the reason for this distinction. Clearly, Joshua's patch adds the listener->AddFolder() calls and also
+    gSubscribableServer.subscribeListener = inner;
+    inner.QueryInterface(Ci.nsISubscribableServer)
+         .subscribeListener = MySubscribeListener;
+    inner.setIncomingServer(gSubscribableServer);
so it was his intent. So we might have to ask Joshua.
Attached patch subscribe2.patch - v5 WIP -JK (obsolete) — Splinter Review
Here's an attempt to remove Joshua's AddFolder() and tweak the AddTo() function so it always adds to the tree of the listener. Debug looks like:
=== AddTo(0000015522A91870): INBOX/INBOX
=== adding base
=== AddTo(0000015521F129D0): INBOX/INBOX
=== adding to listener (composed object)

So the server object is 0000015522A91870 and it's listener is 0000015521F129D0 and the AddTo() is forwarded to it.

Sadly nothing is displayed in the UI, so I must be missing something. There are also no calls to GetCell*() :-(
Attached patch subscribe2.patch - v6 WIP -JK (obsolete) — Splinter Review
Here's a version that shifts the action to the server tree. Maybe this is more promising. Sadly again, nothing on the screen :-(
Comment on attachment 8985141 [details] [diff] [review]
subscribe2.patch - v6 WIP -JK

Oops, I missed
JavaScript error: chrome://messenger/content/subscribe.js, line 75: NS_NOINTERFACE: Component returned failure code: 0x80004002 (NS_NOINTERFACE) [nsISupports.QueryInterface]
on this line:
gSubscribeTree.view = gSubscribableServer.QueryInterface(Ci.nsITreeView);

So if we can't set the tree, then there won't be a display.
This is a continuation of v6, which seems to work.
It reduces the subscribelistener to the single function of OnDonePopulating and it does not implement the subscribableServer. The dialog tree UI is backed by the subscribableServer, that gets exposed from the nntp/imapincomingserver via the new folderView attribute.
In this patch this works now:
-display of the tree, also with accented folders in imap
-checkboxes for 'subscribed' work and you can correctly subscribe/unsubscribe from folders/newsgroups
-all the found folders show initially expanded (open) in the tree
-Refresh button works

This still has some rough edges:
1. console warnings
###!!! ASSERTION: row count did not change by the amount suggested, check caller: 'rowCount == mRowCount', file /var/NVME/TB-hg/mozilla/layout/xul/tree/nsTreeBodyFrame.cpp, line 1803

This is caused by improper use of mTree->RowCountChanged(). I have changed the order of some of these calls, but it does not seem enough. When this is messed up, it causes crashes of TB.

2. I remove some code in subscribe.js::SubscribeOnClick() that called  gSubscribableServer.startPopulatingWithUri(). I do not see we support any loading of parts of the tree. That is why I removed also the tree.getAttribute("ref") handling. Does anybody know what that was about?
Attachment #8984129 - Attachment is obsolete: true
Attachment #8984130 - Attachment is obsolete: true
Attachment #8984624 - Attachment is obsolete: true
Attachment #8985140 - Attachment is obsolete: true
Attachment #8985141 - Attachment is obsolete: true
Attachment #8985311 - Flags: feedback?(jorgk)
Attachment #8985311 - Flags: feedback?(Pidgeot18)
Comment on attachment 8985311 [details] [diff] [review]
subscribe-servertree.patch v7 WIP

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

::: mailnews/base/src/nsSubscribableServer.cpp
@@ +377,5 @@
>  nsSubscribableServer::StopPopulating(nsIMsgWindow *aMsgWindow)
>  {
> +  mStopped = true;
> +
> +printf("ACE: copy tree to listener, %p\n", (void*)mTree.get());

I hope not!!
Attachment #8985311 - Flags: feedback?(jorgk) → feedback+
Yeah, the text is now outdated. It just builds the mRowMap still inside the same nsSubscribableServer object.
This looks very nice :-) - Some comments:
- I'd like to run this without the console debug prints to get a feel for the speed
  on news servers.
- I subscribed to two folders which were unsubscribed and crashed when hitting the "Subscribe" button.
- I'll add some folders with CJK names to my test server.
(In reply to Jorg K (GMT+2) from comment #29)
> - I subscribed to two folders which were unsubscribed and crashed when
> hitting the "Subscribe" button.
Now it crashed on "OK". Otherwise, very nice-ö ;-)
This shouldn't crash now.

I see a small glitch that the folders are shown in a different order in the folder pane than in the subscribe dialog. In the folder pane we use locale aware ordering, which isn't available in the c++ backend.
Attachment #8985311 - Attachment is obsolete: true
Attachment #8985311 - Flags: feedback?(Pidgeot18)
Attachment #8985453 - Flags: feedback?(Pidgeot18)
(In reply to Jorg K (GMT+2) from comment #29)
> This looks very nice :-) - Some comments:
> - I'd like to run this without the console debug prints to get a feel for
> the speed on news servers.

I'll remove them after jcranmer likes the solution :)
You can of course disable them locally.

> - I subscribed to two folders which were unsubscribed and crashed when
> hitting the "Subscribe" button.

Should be fixed in the v7.1.

> - I'll add some folders with CJK names to my test server.

Thanks, they display Chinese/Japanese characters so it seems to work.
(In reply to :aceman from comment #32)
> I'll remove them after jcranmer likes the solution :)
You might be disappointed, but I won't wait until likes it. I'll land this pretty much now.
Hmm, how did NS_ASSERT() compile for you, since it's NS_ASSERTION() ??
Attached patch subscribe-servertree.patch v7.3 (obsolete) — Splinter Review
Fixed indentation in one spot.
Attachment #8985476 - Attachment is obsolete: true
Attachment #8985477 - Attachment is obsolete: true
Test dictated to me via IRC by Aceman. Works too now ;-)
Attachment #8985507 - Flags: review+
Comment on attachment 8985479 [details] [diff] [review]
subscribe-servertree.patch v7.3

Let's go with this, thanks for the collaboration :-)
Attachment #8985479 - Flags: review+
Assignee: nobody → acelists
Status: NEW → ASSIGNED
Product: Thunderbird → MailNews Core
I did a try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=93028c153e4b19c5292535dd184bffd466acefc6

Sadly, one test failure:
TEST-UNEXPECTED-FAIL | comm/mailnews/news/test/unit/test_internalUris.js
The log says that it timed out, which is also what I see locally.
Most likely due to the test doing it's own listener:
function* test_grouplist() {
  // This tests nsNntpService::GetListOfGroupsOnServer
  let subserver = localserver.QueryInterface(Ci.nsISubscribableServer);
  let subscribeListener = {
    OnDonePopulating: function () { async_driver(); }
  };
  subserver.subscribeListener = subscribeListener;
Maybe that doesn't get called.

There's also a crash in Mozmill. Look for "Application unexpectedly closed" in
https://taskcluster-artifacts.net/dKc07SbWT4ueytRg6LZ0WQ/0/public/logs/live_backing.log
I've retriggered the job to see whether that's a random failure.
Comment on attachment 8985479 [details] [diff] [review]
subscribe-servertree.patch v7.3

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

::: mailnews/imap/src/nsImapIncomingServer.cpp
@@ -2505,5 @@
> -  nsCOMPtr<nsISubscribeListener> listener;
> -  (void) GetSubscribeListener(getter_AddRefs(listener));
> -
> -  if (listener)
> -    listener->OnDonePopulating();

Why did you remove this?

::: mailnews/news/src/nsNntpIncomingServer.cpp
@@ -1123,5 @@
> -  if (!listener)
> -    return NS_ERROR_FAILURE;
> -
> -  rv = listener->OnDonePopulating();
> -  NS_ENSURE_SUCCESS(rv,rv);

And this?
(In reply to Jorg K (GMT+2) from comment #39)
> There's also a crash in Mozmill. [snip]
> I've retriggered the job to see whether that's a random failure.
That didn't repeat, but the Xpcshell test failure is real :-(

I tried moving mSubscribeListener->OnDonePopulating(); to before the !mTreeRoot check in nsSubscribableServer::StopPopulating(), but that didn't help. I'm not sure why you moved the OnDonePopulating() calls from the IMAP/NNTP code to nsSubscribableServer.cpp.
Yes, I moved those calls OnDonePopulating into nsSubscribableServer.cpp::StopPopulating as a way to merge common code and call it from the object that manages the listener (nothing else in imap/nntpincomingserver touches the listener now). Does it change anything to put them back?
Further code nits:
I changed the assert to:
NS_ASSERTION(mRowMap.Length() == 0, "mRowMap should be empty");
Not "should have been".

Also
-  rv = mInner->AddTo(aName, addAsSubscribed, aSubscribable, changeIfExists);
-  NS_ENSURE_SUCCESS(rv,rv);
-
-  return rv;
+  return mInner->AddTo(aName, addAsSubscribed, aSubscribable, changeIfExists);
is more elegant, but you won't get the benefit of printing an error if AddTo() fails.
Attached patch subscribe-servertree.patch v7.4 (obsolete) — Splinter Review
Shifted the OnDonePopulating() calls back to NNTP and IMAP.

Instead of timing out, the test now fails on
test_internalUris.js:159

  let groups = enumGroups("");
  for (let group in daemon._groups)
159 Assert.ok(groups.indexOf(group) >= 0);

I guess it's the same problem that we needed to address in the Mozmill test, that is, that things may not be ready yet.
Attachment #8985453 - Attachment is obsolete: true
Attachment #8985479 - Attachment is obsolete: true
Attachment #8985453 - Flags: feedback?(Pidgeot18)
I think it's because the test is still broken as it uses the RDF items from the server, which are now cut off (the whole RDF infra in nsSubscribeServer is now ignored, whether it Works or not).
I have this fixed in https://bug457333.bmoattachments.org/attachment.cgi?id=8944167 but as we also removed OnItemDiscovered() I need to find another way how to access the newsgroups found.

> Shifted the OnDonePopulating() calls back to NNTP and IMAP.

As this calls the listener first, it just hides the fact mInner->StopPopulating() is not running correctly. I would be against this change untill we understand what is going on. As I think calling OnDonePopulating from inside StopPopulating should be the same. Even more correct, as we announce being done only after all the work is really done. With your change (and the old code), the listener was run before the nsSubscribable server had all the mRowMap populated. Which may have been fine in the past (as  nsSubscribable was NOT the backend for the tree display), but now it is.
(In reply to :aceman from comment #45)
> > Shifted the OnDonePopulating() calls back to NNTP and IMAP.
> As this calls the listener first, it just hides the fact
> mInner->StopPopulating() is not running correctly. I would be against this
> change until we understand what is going on.
Well, I've just restored the previous state which BTW changes the behaviour. Now the test fails elsewhere and doesn't hang/timeout. But yes, understanding what's going on it best :-)
But reverting only that part may seem to fix this particular call site, but may be logically incorrect with respect to the other changes done to the subscribableServer backend. And will blow up elsewhere.
So please do not land it until I check it out in the evening.
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/ec2cc16327fe
Restore IMAP and NNTP subscribe to use a C++ nsITreeView. r=jorgk,aceman
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/f41fe8a63f2b
Backed out changeset ec2cc16327fe which was pushed accidentally. a=backout DONTBUILD
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch subscribe-servertree.patch v7.3b (obsolete) — Splinter Review
I had a log discussion with Aceman on IRC about the test failure. The test calls GetListOfGroupsOnServer() which runs the URL and finally ends up in nsNntpIncomingServer::OnStopRunningUrl() which calls nsNntpIncomingServer::StopPopulating() and thus nsSubscribableServer::StopPopulating(). The latter returns straight away, since nsSubscribableServer::StartPopulating() was never called.

v7.3b fixes that by adding a call of nsSubscribableServer::StartPopulating() to nsNntpIncomingServer::OnStartRunningUrl().

Like this, the test passes.

v7.3b also fixes the nits from comment #43, so do an interdiff of v7.3b with v7.3. Forget v7.4 ;-)
Attachment #8985564 - Attachment is obsolete: true
Attachment #8985733 - Flags: feedback?(acelists)
Attached patch subscribe-servertree.patch v7.3c (obsolete) — Splinter Review
Oops, Aceman had some white-space correction in his v7.5 from
https://hg.mozilla.org/try-comm-central/rev/f86b331d677d88ec13ad3c9ae22d3602e88a6a95
which I've incorporated here.

Aceman, please check that this added
  nsresult rv = mInner->StartPopulating(nullptr, /* ignored */ false, /* ignored */ false);
doesn't do any damage ;-)
Attachment #8985733 - Attachment is obsolete: true
Attachment #8985733 - Flags: feedback?(acelists)
Attachment #8985736 - Flags: feedback?(acelists)
I noticed another problem. If I click "Subscribe" or "Unsubscribe", the check mark isn't set/removed immediately. I need to hover the cell to see it update.
(In reply to Jorg K (GMT+2) from comment #52)
> Created attachment 8985736 [details] [diff] [review]
> subscribe-servertree.patch v7.3c
> 
> Oops, Aceman had some white-space correction in his v7.5 from
> https://hg.mozilla.org/try-comm-central/rev/
> f86b331d677d88ec13ad3c9ae22d3602e88a6a95
> which I've incorporated here.
> 
> Aceman, please check that this added
>   nsresult rv = mInner->StartPopulating(nullptr, /* ignored */ false, /*
> ignored */ false);
> doesn't do any damage ;-)

Sorry, I can't decide this and we will be drifting far from the intent of this bug.
If you want to change the behaviour of GetListOfGroupsOnServer() or even remove it from public API, I'm fine with that, but we can't do it this quickly as we do not fully understand it. We should do it in a separate bug.
Comment on attachment 8985736 [details] [diff] [review]
subscribe-servertree.patch v7.3c

OK, let's go with 7.5, but please fix comment #53.
Attachment #8985736 - Attachment is obsolete: true
Attachment #8985736 - Flags: feedback?(acelists)
Attached patch subscribe-servertree.patch v7.6 (obsolete) — Splinter Review
Aceman's v7.5 plus fixing the refresh issue from comment #53 as dictated by Aceman via IRC.
Attachment #8985754 - Attachment is obsolete: true
Attached patch subscribe-servertree.patch v7.6b (obsolete) — Splinter Review
Oops, don't call invalidate on negative index.
Attachment #8985776 - Attachment is obsolete: true
Attached patch subscribe-servertree.patch v7.7 (obsolete) — Splinter Review
Aceman's latest version fresh from the try server with some tweaks:
Changed strings a little, Cu.reportError() doesn't need a trailing \n.

Sadly I don't see neither the Cu.reportError() nor the status message :-( - I see "Please wait...".
Attachment #8985777 - Attachment is obsolete: true
(In reply to Jorg K (GMT+2) from comment #60)
> Sadly I don't see neither the Cu.reportError() nor the status message :-( -
> I see "Please wait...".
Actually, when I go offline and do subscribe, I see that message.

So should we go with this now or do we want another iteration?

How do I port the new strings to TB 60:
+offlineState=You are offline. Items could not be retrieved from the server.
+errorPopulating=Error retrieving items from the server.

Aceman suggested to hack the former using:
https://searchfox.org/comm-central/rev/0d312c74b85ff3292247dddb2217f50ab0df45f3/mail/locales/en-US/chrome/messenger/offline.properties#28
and drop the second one which shouldn't show up?
So I'd have to add offline.properties to subscribe.xul to get to the string bundle.
Looking for subscribe bugs currently yields 83 of them: https://mzl.la/2JIKpop

The blank panel when no password is available since no folder was accessed yet is bug 534994 :-(
Final patch \o/:

Removed class nsSubscribeListener since it wasn't used any more and moved Cu.reportError() to the else branch.

Ready to go, but we don't fix all the other 83 bugs here :-(
Attachment #8985790 - Attachment is obsolete: true
Attachment #8985797 - Flags: review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/194e28dafcb5
Restore IMAP and NNTP subscribe to use a C++ nsITreeView. r=jorgk,aceman
https://hg.mozilla.org/comm-central/rev/ce10f3ede4cc
Re-enable test-subscribe-news-filter.js. r=jorgk
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Grrrr, Form History Control posted the previous comment :-(
Attached patch beta.patchSplinter Review
For beta we need to
- s/nsTreeColumn/nsITreeColumn/
- s/mozilla::dom::DataTransfer/nsIDOMDataTransfer/
- hack the offline string.
IMAP subscribe tree works fine in 60.0b7 + rev f87c98a8bb92efc0eb490d9721983315e920a396 backported on top, on OpenBSD/amd64. Thanks!
Glad you like it. As per comment #62, subscribe has some issues :-(
Depends on: 1470892
You need to log in before you can comment on or make changes to this bug.