522955 - IMAP ACL edit UI: Allow user to share IMAP folder with other users on the same server

Status: NEW

(Thunderbird :: Mail Window Front End, enhancement)

Description

[Ben Bucksch (:BenB)]

IMAP allows shared folders with flexible access rights. We should allow users to configure that, in a user-friendly manner.

E.g. President Mark (user "Mark") can allow his secretary (user "martha") to modify his folder "INBOX", and his Vice President of Sales (user "fred") to read his folder "Project Zima".

UI:
Folder (right pane), context menu (right click), Properties..., tab "Sharing" or a new tab.
- List other users which currently have access to this folder, with the access rights
- Allow to modify or add others with access
- Access patterns are simplied to let users understand them, i.e. just "No access", "Read" (lrs) and "Write" (lrswipcd)
- Textfield should allow comfortable entry of username based on realname or email address. Either via LDAP (configured by admins in backend prefs) or an extension.

Comment 1

[Ben Bucksch (:BenB)]

Bug 207628 is the older friend of this.

Comment 2

[Nikolay Shopik]

After fixing this bug I think we should close bug 207628 as dupe of this for consistency sake.

Comment 3

[Ben Bucksch (:BenB)]

Attachment: Screenshot current state

Comment 4

[Ben Bucksch (:BenB)]

Attachment: Screenshot - Edit/Add dialog

Comment 5

[Ben Bucksch (:BenB)]

Attachment: Current state as extension

Almost finished. Requires the patch from bug 522954.

Comment 6

[Ben Bucksch (:BenB)]

Attachment: Current state as extension

Comment 7

[Ben Bucksch (:BenB)]

Source code at
https://hg.mozilla.org/users/mozilla.BenB_bucksch.org/mail-acl-ext/

It's currently implemented as extension, but I'd like to get this into core.
It's unobtrusive, fits in naturally with existing ACL read-only UI (folder context menu | Properties... | tab "Sharing") and has a small footprint (a few KB).

Comment 8

[Ben Bucksch (:BenB)]

Documentation:

= Purpose =

This Thunderbird extension allows users to share their IMAP folders with other users of the same server. This is useful to e.g. allow the secretary or college to access your mails when you're on vacation leave or working on the same project.

Given that the user interface is written for end-users, access rights are very simple: Either read-only or full write or no access. It's intentional that you cannot give admin rights to other users, and cannot modify (nor see) your own rights or that of special users like the "cyrus" admin.

= Use =

In the folder list on the left, click on an (IMAP) folder with the right mouse button and click on "Properties..." (the last context menu entry). In the resulting dialog box, click on "Sharing". If the extension is properly installed you see a box on the right, under the heading "Other users' permissions". The list contains all the people (more concretely, the usernames on this IMAP server) which have access rights to this folder.

To modify the access rights of one of them, click on a name and then on "Edit..." and change the radiobox to "Read", "Write" or "No access", depending on how you want it to be. "Write" in this case allows to add, delete or read emails in the folder.
Keep the [x] Incl. Subfolders checked, if you want to modify the access rights of the whole folder hierarchy from here on. If you want to modify only this folder, uncheck it. For example, if you want to share all your folders *apart from* the inbox, you can first share the inbox including all subfolders, and then modify the Inbox, *not* including subfolders, and remove the rights again.

If you want to give new people access to your folder, click the "Add..." button and enter his IMAP username. That's the same name that this person uses to log in to the IMAP server.
Depending on your configuration, you can maybe also enter the person's real name, and you'll get a dropdown with hits, and you select the right person. The username will be extracted automatically (from the part between <>). Select the proper access permission to hand out - read or write - and whether you also want to share the subfolders of this folder. After clicking OK, the new person's username should appear in the list. Your folder should show up automatically in the person's mailbox hierarchy the next time he logs in to his IMAP mail account (he might have to restart Thunderbird, but you don't have to).

If some day you're not sure which folders you have shared with others, you can right-click on the account (on the left, in the folder list, the entry right above your inbox, not the inbox itself), and select "Show all shared folders..." from the context-menu. You should see a list of all shared folders, one entry per folder and user. Be careful with the "Revoke all" button, it will remove all sharing that you have configured. If you want to un-share just one folder, select the folder+user entry and click "Edit..." (you have to repeat that for each user with access to this folder), or alternatively, close the "Show all sharing" dialog and go to the context-menu of the folder, as described in the beginning.

Comment 9

[Ben Bucksch (:BenB)]

Attachment: UI extension hookup point

To install the extension, you need bug 522955 plus this patch.
It merely removes a pointless explanation string (to make space), and slightly changes the dialog XUL to allow the extension to hook in.

Comment 10

[Ben Bucksch (:BenB)]

Attachment: Fix, v1 - in form of extension

Neil, could you also review the source itself, please?

Just pretend that it's not an extension, but a patch for Thunderbird UI. I'll rework it as integrated code later.

Patch is the same as current state of <https://hg.mozilla.org/users/mozilla.BenB_bucksch.org/mail-acl-ext/>.

Comment 11

[Ben Bucksch (:BenB)]

Comment on attachment 414489 [details]
Screenshot current state

Byran, can you UI-review, please? Please see comment 0, comment 7 and 8.

Comment 12

[Ben Bucksch (:BenB)]

Comment on attachment 425840 [details] [diff] [review]
UI extension hookup point

Asking Mnyromyr for UI-review instead (folder properties are still in mailnews/, and he's qualified and willing to review - thanks!)

Comment 13

[Karsten Düsterloh]

Comment on attachment 425840 [details] [diff] [review]
UI extension hookup point

>+++ b/mailnews/base/content/folderProps.js
>     setFolderTypeDescription: function(folderDescription)
>     {
>-      var folderTypeLabel = document.getElementById("folderDescription.text");
>-      if (folderTypeLabel)
>-        folderTypeLabel.setAttribute("value", folderDescription);
>     },

This doesn't make much sense. If the text is indeed useless (with your new interface), you should just kill the respective IDL function and its callers.

r+=me based on the actual XUL changes and the assumption that the above will go away in the non-extensional next patch version.

Comment 14

[Karsten Düsterloh]

Comment on attachment 425843 [details] [diff] [review]
Fix, v1 - in form of extension

I didn't actually run the extension yet, the following comments are solely based upon source code inspection. I usually don't comment each instance of a problem, so assume I mean to fix every instance of it, even if not mentioned explicitly. ;-)

[Basically, I like ;-) <https://wiki.mozilla.org/SeaMonkey:MailNews:CodingStyle>, although I'm aware its slight deviations from the Mozilla standard aren't general consensus.]


>+++ b/src/content/aclAll.js
>+function shallHideUser(folder, username)

Mozilla standard means prefixing function arguments with 'a'.
(Yes, many MailNews files still violate that, but new code usually complies.)

Your code below violates this almost(?) everywhere. :-((

>+{
>+    //  hardcode common server-internal usernames

Mozilla standard indent is two spaces. 
(Yes, many MailNews files still violate that, but new code usually complies.)

>+    // hide configurable list of users, e.g. server admin and groups
>+    try {
>+        var hideUsersPattern = getPref("mail.imap.acl.hideUsersRegexp");
>+        if (new RegExp(hideUsersPattern).test(username))
>+            return true;
>+    } catch (e) {} // if pref doesn't exist

Both SM and TB support Application.prefs.getValue(prefname, defaultvalue), which doesn't throw.
(Here and multiple other instances of getPref.)

>+ * 0 = unknown
>+ * 1 = no access
>+ * 2 = read only
>+ * 3 = write
>+ * 4 = admin

Please define suitable global constants (or make these part of a suitable IDL) and use them where you work with these values!

>+function simplifyIMAPRights(rights)
>+{
>+    if (contains(rights, "a"))
>+        return 4;
>+    if (//contains(rights, "l") &&
>+        contains(rights, "r") &&
>+        contains(rights, "s") &&
>+        contains(rights, "w") &&
>+        contains(rights, "i") &&
>+        (contains(rights, "c") || contains(rights, "k")) &&
>+        (contains(rights, "d") || contains(rights, "t")) &&
>+        contains(rights, "p"))
>+        return 3;
>+    if (//contains(rights, "l") &&
>+        contains(rights, "r") &&
>+        contains(rights, "s"))
>+        return 2;
>+    rights = rights.replace(" ", "");

Yuck. In the worst case, this means parsing the complete 'rights' string at least 12 times! Before parsing it a 13th to delete any blanks... 
Furthermore, I doubt that the JSE will inline all these function call frames for 'contains'.

How about looping over the 'rights' string characters and switch-case-returning on suitable matches?

(I suppose the actual single characters aren't needed anywhere else. If not, define suitably named contants.)

>+function translateIMAPRightsToUser(imapRights)
>+{
>+    var sb = E("delegateStrings");

Ugh. This is just unreadable. Please use "talking" names for identifiers.
And, btw, I very doubt that calling a function to merely call a member function is anywhere from useful. It may save some key strokes when writing the code, but nothing else. It just adds unnecessary complexity and hampers readability, especially if badly named.

(Also: sb isn't used at all; and even if, it probably wouldn't be needed anyway.)

>+    var mode = simplifyIMAPRights(imapRights);
>+    if (mode == 0)

Code like this sucks. Everytime you read this, you need to look up what "mode 0" actually means...

>+function contains(str, lookfor)
>+{
>+    return str.indexOf(lookfor) != -1;
>+}

Most probably calling this function is way more expensive (and way less readable) than using indexOf (or some regex) directly.

>+function setACL(folder, username, rights, withSubfolders,
>+    successCallback, errorCallback)

Probably not worth folding, in the light of readability.

>+    _setACLWithoutSubfolders(folder, username, rights,
>+    function() // succeeded
>+    {
>+        if ( ! withSubfolders)
>+        {
>+            successCallback(); 
>+            return;
>+        }

The bracing philosophy should be consistent to the rest of the file, e.g. fix your try blocks or your if blocks. (Personally, I prefer readability over brevity, iow brace on its own line.)

>+        // List subfolders
>+        var subfolders = Cc["@mozilla.org/supports-array;1"].createInstance(Ci.nsISupportsArray);
>+        folder.QueryInterface(Ci.nsIMsgFolder).ListDescendents(subfolders);

Do you actually expect folder _not_ to be a nsIMsgFolder? That is, do you really need the QI (here, and in several other places as well)?

>+// internal function, don't call
>+function _setACLWithoutSubfolders(folder, username, rights,
>+    successCallback, errorCallback)

Probably not worth folding, in the light of readability.

>+    folder = folder.QueryInterface(Ci.nsIMsgImapMailFolder);

It's better to use 

  if (folder instanceof Ci.nsIMsgImapMailFolder) ... 

If you need the QI at all, that is...

>+    folder.setACL(username, Ci.nsIMsgImapMailFolder.replaceRights, rights,
>+    { // nsIURLListener

Comment goes unto the next line (here and other instances).

>+function getACL(folder, withSubfolders, successCallback, errorCallback)
...
>+        if ( ! withSubfolders)

No spaces around the ! (here and other instances).

>+        // Call setACL for each folder, but sequentially
>+        // (to be easy on the server and our network code).
>+        // Note that setACL is networked and async, so |for| would fire them all in parallel.

Cut'n'waste error? ;-)

>+function fullnameToUsername(fullname)
>+{
>+    var email;
>+    if (contains(fullname, "<"))
>+    {
>+        email = /<([^>]*)>/.exec(fullname)[1];

Strictly speaking RfC 5322, this is wrong. "te<st>"@example.com is a valid email address! ;-)

>+function emailToUsername(email)
...
>+    var sp = email.split("@");

Strictly speaking RfC 5322, this is wrong. "m@d"@example.com is a valid email address! ;-)
(As a sidenote, we probably should have an interface which handles RfC 5322 address handling correctly, given that we do it wrong in so many places since RfC 822... Not part of this bug for sure.)

>+function setupAutoComplete(textboxElement)
...
>+    if (mode == 2) // use address book incl. default LDAP server, like in Composer
>+    {
...
>+    }
>+    else // no autocomplete
>+        ; // nothing

If one if branch has braces, the other should have as well.

>+function createLDAPSession(prefPrefixServer, prefPrefixAutocomplete)
>+{
>+    var LDAPSession= Cc["@mozilla.org/autocompleteSession;1?type=ldap"]
>+            .createInstance(Ci.nsILDAPAutoCompleteSession);

Missing space before '=', the '.' of .create should align with the '['.

>+      LDAPSession.serverURL = Cc["@mozilla.org/network/io-service;1"]
>+            .getService(Ci.nsIIOService)
>+            .newURI(url, null, null)
>+            .QueryInterface(Ci.nsILDAPURL);

Same here.

>+      // Try Gecko 1.8 way
>+      var serverURL = Cc["@mozilla.org/network/ldap-url;1"]
>+            .createInstance(Ci.nsILDAPURL);
>+      serverURL.spec = url; // (error is fatal)
>+      LDAPSession.serverURL =  serverURL;

Shouldn't be needed anymore, since we _are_ 1.9.x?

>+      if (getPref(prefPrefixServer +".protocolVersion") == "2")

Missing space behind '+'.

>+function E(id)
>+{
>+    return document.getElementById(id);
>+}

Evil, useless, pick your derision and kill it.
(I understand the motivation and reason for such functions, but this is nothing we want.)

>+function getPref(aPrefName, isComplex)

Useless, given that Application.prefs.getValue is around.
Plus, we usually don't want this to throw anyway.

>+++ b/src/content/aclEdit.js
>+        var usernameE = E("email");

Any justification for the uppercase E in the variable?! :-|

>+            if (mode == 3)
>+                radiogroup.selectedItem = E("write");

Any special dislike of using 'switch'?

>+function onOK()
>+{
>+    try {

Which exceptions do you expect?

>+++ b/src/content/aclEdit.xul
>+<dialog id="aclEditDialog"
>+         xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
>+         title="&dialog.title;"
>+         onload="onLoad();"
>+         ondialogaccept="onOK();"
>+         >

Wrong indentation here and elsewhere below.

>+    <script type="application/x-javascript" src="chrome://acldelegate/content/aclAll.js"/>
>+    <script type="application/x-javascript" src="chrome://acldelegate/content/aclEdit.js"/>

Use application/javascript (see bug 381467 and relatives).

>+    <vbox>
>+        <hbox align="baseline">

Usual indentation is two spaces for XUL as well.

>+++ b/src/content/aclShowAll.js
>+function listboxBuild2()
>+{
...
>+        var enumerator = folder.getOtherUsersWithAccess();
>+        while (enumerator.hasMore())
>+        {
>+            let username = enumerator.getNext();
>+            let rights = folder.getPermissionsForUser(username);
>+            var rightsLabel = translateIMAPRightsToUser(rights);

Your use of var vs. let is rather inconsistent.
As a general rule, use var only on a function's main level and let on inner scopes. (Or even forget that var ever existed, but Neil will scream then. *g*)

>+function onEdit()
...
>+    window.openDialog("chrome://acldelegate/content/aclEdit.xul",
>+        "aclEdit", "",
>+    { // params
>+        add : false,
>+        username : selectedItem.username,
>+        rights : selectedItem.rights,
>+        folder : selectedItem.folder,
>+        closeOKCallback : editDialogReturned
>+    });

Put "" on its own line, align the ", { and } vertically below that of "chrom..., plus move the comment on its own line. (Several similar instances below.)

>+function editDialogReturned(folder, username, rights, withSubfolders)
>+{
>+    setACL(folder, username, rights, withSubfolders,
>+    function() // success
>+    {
>+        listboxBuild();
>+    },
>+    function(errorMsg) // error
>+    {
>+        error(errorMsg);
>+        listboxBuild();
>+    });

Indent the parameters setACL, so that they don't look like code on the main function level. (Plus several more times below.)

>+++ b/src/content/aclShowAll.xul
>+        <listbox id="aclList" flex="1"
>+                 onselect="listboxSelect();"
>+                 ondblclick="listboxDoubleClick();"
>+                 style="width: 40em;"

That width should probably be localizable.

>+++ b/src/content/aclShowAllMenuOverlay.js
>@@ -0,0 +1,35 @@
>+// Hook up

Missing licence block.

>+/**
>+  <https://developer.mozilla.org/en/XUL/PopupGuide/ContextMenus#Hiding_and_Showing_Menu_Items_based_on_Context>
>+  <https://developer.mozilla.org/en/XUL/PopupGuide/PopupEvents>
>+
>+  TODO doesn't work, prevents context menu from appearing. No idea why.
>+  Therefore, work around using onpopupshowing="myFunc() return originalTBFunction();"
>+  in overlay.
>+
>+document.getElementById("folderPaneContext")
>+    .addEventListener("popupshowing", aclFolderContextMenuShowing, false);
>+*/

O_O

>+++ b/src/content/aclTab.js
>+window.addEventListener("load", function()

I actually dislike anonymous monster functions. Any reason for not defining the function conventionally first and tossing it into addEventListener then?

>+++ b/src/content/aclTab.xul
>+        <listbox id="aclList" flex="1"
>+                onselect="listboxSelect();"
>+                ondblclick="listboxDoubleClick();"
>+                style="height: 10em"

Hardcoded lengths like this are almost never a good idea.

>+++ b/src/content/aclTabSep.xul
>+    <tab
>+         id="aclTab"

Move id onto the preceding line.

>+++ b/src/locale/de-DE/aclAll.properties
>+errorMalformedUsername=Bitte geben Sie einen Benutzernamen oder eine Email-Adresse Ihrer Domain an

Has to be "E-Mail", should probably end with a dot if it is a full sentence.
(Several more of both below.)

>+errorCantChangeYourself=Sie sind anscheinend grade dabei, Ihre eigenen Berechtigungen einzuschränken. Das lassen wir mal besser sein. Oder lassen Sie etwa auch Ihre Schlüssel zuhause? ;-)

A bit too jovial, I'd say. ;-)
(Also, "gerade" and "zu Hause".)

>+errorCantChangeThisUser=Dies ist ein besonderer Benutzeraccount. Sie können seine Berechtigungen nicht ändern.

"Benutzerkonto"?

>+++ b/src/locale/de-DE/aclList.dtd
>+<!ENTITY acl.add.label "Hinzufügen...">

Use an ellipsis character (… 0x2026) instead of ...

>+++ b/src/locale/de-DE/aclShowAll.dtd
>+<!ENTITY acl.showAll.menu.label "Alle Freigaben zeigen...">
>+<!ENTITY acl.showAll.menu.accesskey "f">

Should be an uppercase F (slight optimization).


>+++ b/src/locale/en-US/aclAll.properties
>+errorCantChangeYourself=You seem to try to reduce your own permissions. Do you also leave your keys at home?

This text is not helpful to the user. (The German text is slightly better because it at least tells that we won't do what the user told us to do.)

>+++ b/src/locale/en-US/aclShowAll.dtd
>+<!ENTITY acl.showAll.menu.label "Show all shared folders...">
>+<!ENTITY acl.showAll.menu.accesskey "s">

Should be an uppercase S.

>+<!ENTITY dialog.title "Show all sharing of all folders">
>+<!ENTITY acl.revokeAll.button "Revoke all sharing for all folders">

I don't understand these two texts...

Comment 15

[Ben Bucksch (:BenB)]

Karsten, thanks for taking the time to review. There are some good points in there. The constants for the modes are a good idea, although there are only exactly 2 callers, so I thought they are overkill.
The majority of your points are a matter of pure opinion, like formatting or whether contains(rights, "a") is readable. I didn't want to clutter this IMAP ACL logic with JS details like str.indexOf(substr) != 1.
You had many good points, though, so I'll go through the code and fix those.

Comment 16

[Ben Bucksch (:BenB)]

(And FWIW, it's simply not correct that it's "parsing" the string 12 times. It simply does indexOf(). It's looking 14 times for (different) characters, which can appear in any order, and some of them are either/or. I don't know a simpler/nicer and more readable way to do it. Even sorting the string first would be slower than just searching for 14 characters.)

Comment 17

[Karsten Düsterloh]

Comment on attachment 425843 [details] [diff] [review]
Fix, v1 - in form of extension

BTW:

>+++ b/src/install.rdf
>+<?xml version="1.0" encoding="UTF-8"?>
...
>+    <em:description>Ermöglicht Ihnen, Ihr Postfach für andere Benutzer freizugeben.
...
>+            <em:description>Ermöglicht Ihnen, Ihr Postfach für andere Benutzer freizugeben.</em:description>

My trunk extension manager bitterly complained about an incompatible addon, just because of these wrongly encoded Umlauts... :-|

Comment 18

[Ben Bucksch (:BenB)]

Karsten, why "wrongly encoded"? This is an XML file which says "encoding="UTF-8"". I just double-checked and it's indeed actually encoded in UTF-8 (sometimes I accidently saved as ISO-8859-15, because that was the default in my previous editor, but not so here).

Or are you saying I should use entities, e.g. ü? That would be a good idea, that's even more error-proof, yes. I'll do that.

Comment 19

[Karsten Düsterloh]

(In reply to comment #15)
> The majority of your points are a matter of pure opinion, like formatting 

Coding guide(!) lines are always opinions.
But it doesn't pay if everyone follows his own nose, this just leads to the mess that we already have in mailnews/libmime.
Inconsistent code formatting just hurts code maintenance, especially if you didn't write the code. It's easy to do literal brain dumps, but very hard to clean up other people's mess afterwards. 
Of course, this doesn't apply if one's code is an error free masterpiece. :-|

> whether contains(rights, "a") is readable.

I didn't claim that having a contains function isn't readable. "contains" is even slightly more readable than indexOf, I just doubt the usefulness. And even that wouldn't be an issue which needed fixing if the actual usage wouldn't be that crude.

> I didn't want to clutter this IMAP ACL logic with JS details like
> str.indexOf(substr) != 1.

Quite some overhead (and very inefficient worst case) for minimal gain. 

(In reply to comment #16)
> (And FWIW, it's simply not correct that it's "parsing" the string 12 times.
> It simply does indexOf().

Which means looking at each character, every time, in the failure case.

Anyway, probably not so much of a problem here in this "rarely" called context.

Comment 20

[Karsten Düsterloh]

(In reply to comment #18)
> Karsten, why "wrongly encoded"? This is an XML file which says
> "encoding="UTF-8"". I just double-checked and it's indeed actually encoded in
> UTF-8 (sometimes I accidently saved as ISO-8859-15, because that was the
> default in my previous editor, but not so here).

Saving the diff results in a file encoded as Latin1, with 0xF6 for ü, and I doubt that Bugzilla/SeaMonkey recode on saving it...

Comment 21

[Karsten Düsterloh]

Comment on attachment 425843 [details] [diff] [review]
Fix, v1 - in form of extension

I ran the extension (after some slight manual fixes to install.rdf to make it actually work) in both SeaMonkey 2 trunk and Shredder trunk.

(1) In SeaMonkey, it hardly works only, which is no surprise, since the addon only claims Thunderbird conformance:
- opening the folder context menu complains about the missing gFolderTreeView variable
- opening the Sharing tab says "folder.refreshACL is not a function"
- Sharing tab UI is visible then, but almost non-functional

(2) Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.9pre) Gecko/20100223Shredder/3.0.2pre
- opening the Sharing tab says "folder.refreshACL is not a function"
- Sharing tab UI is visible then, but almost non-functional

</release_sparetime_lock> ;-)

Comment 22

[Ben Bucksch (:BenB)]

> "folder.refreshACL is not a function"

As said (I think), you need the patch from bug 522954, which is the backend for this.

Comment 23

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 414489 [details]
Screenshot current state

Is there a delete/remove option?

I'm wondering what the best way for me to test this would be.  I don't think I have access to an IMAP system with folder sharing so even if I got all this built I'm not sure I'd be able to see anything.

Comment 24

[Ben Bucksch (:BenB)]

> Is there a delete/remove option?

Yes, you select a user from the access list, click "Edit" and select "No access".

Also, the "Show all shared folders..." context menu item on the account (in folder pane), which gives an overview over all sharing, has a button "Revoke all sharing for all folders".

Comment 25

[Ben Bucksch (:BenB)]

I can provide test accounts on my IMAP server.

Comment 26

[Ben Bucksch (:BenB)]

OK, I spoke with my employer, and we don't have the time to go through the review for this. (Esp. with this kind of respectless and insulting review, personal comment.) The review hasn't found any technical problem of any significance, it's all pure opinion.

So, at this point, it's "take it or leave it". It's a pity, because I think this would fit very naturally into the current UI and would be useful for users in general.

One option would be that we check it in as-is and somebody else does the style fixes in the tree.

Comment 27

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 414489 [details]
Screenshot current state

looks fine

Comment 28

[Ben Bucksch (:BenB)]

Comment on attachment 425840 [details] [diff] [review]
UI extension hookup point

ui-r+ on "hookup" per dup bug 553386

Comment 30

[Ben Bucksch (:BenB)]

Comment on attachment 425840 [details] [diff] [review]
UI extension hookup point

neil: ping sr

Comment 31

[[e-mail]]

Sorry for overlooking this.

None of the servers to which I have access seem to support sharing.
So could you do screenshots
a) without attachment 425840 [details] [diff] [review]
b) with attachment 425840 [details] [diff] [review] only
c) with the extension?
Thanks.

Comment 32

[Mark Banner (:standard8)]

Comment on attachment 425840 [details] [diff] [review]
UI extension hookup point

I've looked at this patch in conjunction with the extension, but just from the focus of an extension hook-up point, not actually testing the extension.

>diff --git a/mailnews/base/content/folderProps.js b/mailnews/base/content/folderProps.js
>     setFolderTypeDescription: function(folderDescription)
>     {
>-      var folderTypeLabel = document.getElementById("folderDescription.text");
>-      if (folderTypeLabel)
>-        folderTypeLabel.setAttribute("value", folderDescription);
>     },

Is it actually worth keeping setFolderTypeDescription? If it is pointless text, then maybe we should just remove it and the supporting code. I would be happy for that to happen in a follow-up bug though.

>diff --git a/mailnews/base/content/folderProps.xul b/mailnews/base/content/folderProps.xul

>-        <label value="&permissionsDesc.label;" id="permissionsDescLabel"/>

This string hasn't been removed from the l10n files.

>+          <groupbox id="yourPermissions" align="start" flex="1">
>+            <!--<caption label="&yourPermissions.label;" />-->
>+            <description id="folderPermissions.text" />
>+          </groupbox>

I think this caption needs implementing. Without the extension, you just get a box listing permissions with no label, which makes it pretty hard to understand. With the extension the label is there and makes sense.

Therefore sr- due to the string issues.

Comment 33

[Mark Banner (:standard8)]

(and sorry it took so long, apart from the string issues, the hook-up patch looks fine).

Comment 34

[eichertc]

This feature, (meaning ACL), has the power to retire Outlook forever.

I am certainly not the only one who knows this. It just means that someone didn't want to replace Outlook.

Comment 35

[DaLiV]

sadly even possibility to use IMAP-ACL addon now is fully killed (also as for creating similar new).
that was previously relied on building own connection to server by getting saved user/pass .
but new API not provide "access to account sensitive info" (that behavior possible not so bad, and building own OAUTH link not so good idea)
also none functions gives access to "opened socket to server" for direct sending of commands in already authenticated session (but that is normal)

IMAP ACL still are good for Public/Shared folders . feature mostly is "enterprise-related" and not for home users, therefore not everyone think about this.