Closed Bug 1579987 Opened 5 months ago Closed 5 months ago

use the folderURI parameter when loading Account central

Categories

(MailNews Core :: Account Manager, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 71.0

People

(Reporter: aceman, Assigned: aceman)

References

Details

Attachments

(2 files, 2 obsolete files)

In bug 1579575 we added a folderURI argument (containing the selected folder) to the URL used to load msgAccountCentral.xul. But the code in that page ignores it and tried to find the currently selected folder itself.

We can use the argument and simplify the msgAccountCentral.js a bit.

Depends on: 1579575
Product: Thunderbird → MailNews Core
Attached patch 1579987.patch (obsolete) — Splinter Review

This could do it. I also pass the selected folder to the various functions called from the Account central so that again they do not need to find out which folder is selected (using various differing methods). Just pass what we already have displayed to be sure what gets used in those functions (like MsgFilters, MsgSearchMessages).

Attachment #9091540 - Flags: feedback?(jorgk)
Attached patch 1579987-SM.patch (obsolete) — Splinter Review

For this to work, Seamonkey also needs to pass the folderURI similarly to TB (bug 1579575).

Attachment #9091540 - Attachment is obsolete: true
Attachment #9091540 - Flags: feedback?(jorgk)
Attachment #9091541 - Flags: review?(iann_bugzilla)
Attachment #9091540 - Attachment is obsolete: false
Attachment #9091540 - Flags: feedback?(jorgk)

Sorry, more places call ShowAccountCentral().

Attachment #9091541 - Attachment is obsolete: true
Attachment #9091541 - Flags: review?(iann_bugzilla)
Attachment #9091545 - Flags: review?(iann_bugzilla)
Comment on attachment 9091540 [details] [diff] [review]
1579987.patch

I really know nothing about this stuff, sorry. And even with my f+, you'd still get a review somewhere.
Attachment #9091540 - Flags: feedback?(jorgk) → feedback?(alta88)
Comment on attachment 9091540 [details] [diff] [review]
1579987.patch

Using uris like this solves a lot of problems. If there were a back/forward cache or history like in a browser there might be  unexpected consequences, but I think here it's fine.
```
--- a/mailnews/base/content/msgAccountCentral.js
+++ b/mailnews/base/content/msgAccountCentral.js
@@ -4,77 +4,82 @@
 var selectedServer = null;
+var selectedFolder = null;

Didn't these globals used to be gSelectedFolder style? Made it easier. Up to you.
 
-function ArrangeAccountCentralItems(server, msgFolder) {
+function ArrangeAccountCentralItems() {
   let exceptions = [];
   let protocolInfo = null;
+  let server = selectedFolder ? selectedFolder.server : null;

Wasn't selectedServer already set up?
```
Attachment #9091540 - Flags: feedback?(alta88) → feedback+

Good catch, thank you.

Attachment #9091540 - Attachment is obsolete: true
Attachment #9092191 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9091545 [details] [diff] [review]
1579987-SM.patch v1.1

LGTM r=me
Attachment #9091545 - Flags: review?(iann_bugzilla) → review+
Attachment #9092191 - Flags: review?(mkmelin+mozilla) → review+

Thanks.

Keywords: checkin-needed

I'm agonising over the commit message here:
use the passed folderURI argument passed into Account Central
doesn't seem OK. It's not an argument and it's not passed. We get it here:
let folderURI = document.location.search.replace("?folderURI=", "");

Something along the lines of
Use document URL's folderURI query/search(?) part when loading Account Central

But .location is not an nsIURI, it seems to the the href (string). And no idea what .search is, apparently the search/query part of that string. Where are these properties defined?

Flags: needinfo?(acelists)

Well it's called an url parameter

Summary: use the folderURI argument when loading Account central → use the folderURI parameter when loading Account central

(In reply to Jorg K (GMT+2) from comment #9)

But .location is not an nsIURI, it seems to the the href (string). And no idea what .search is, apparently the search/query part of that string. Where are these properties defined?

I looked here: https://developer.mozilla.org/en-US/docs/Web/API/Location

Flags: needinfo?(acelists)

Thanks guys, but no one answered the question:

Well it's called an url parameter

Where?

So how about: Use folderURI from document's location.search when loading Account Central

So is that OK?

They also call it a parameter at the link I provided.
"Use folderURI parameter from document's location.search when loading Account Central" ?

Sold!

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/afad3a7d34eb
Use folderURI parameter from document's location.search when loading Account Central. r=mkmelin
https://hg.mozilla.org/comm-central/rev/dff2395d386f
Use folderURI parameter from document's location.search when loading Account Central - SeaMonkey part. r=IanN

Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 71.0

Hmm, this would have been a beta candidate since bug 1579575 got uplifted.

You need to log in before you can comment on or make changes to this bug.