493016 - nsIMsgFolder::isCommandEnabled should die

Status: NEW

(MailNews Core :: Backend, defect)

Description

[Magnus Melin [:mkmelin]]

Bug 473642 comment 9 -->

> (From update of attachment 377221 [details] [diff] [review])
> +        return folders.length == 1 && folders[0].canRename &&
> +               folders[0].isCommandEnabled("cmd_renameFolder");
>
> +            (folder.server.type != "imap" ||
> folder.server.canCompactFoldersOnServer) &&
> +            folder.isCommandEnabled("button_compact");
> I just had a long irc discussion with dmose about the absurdity of (1)
> splitting this logic between the front and back ends, with particular checks > in each and (2) even worse, having a canRename attribute that doesn't tell you
> everything, but also requires you to check
> isCommandEnabled("cmd_renameFolder"). (The same applies for the compact stuff)

> The tentative conclusion we came to was that nsIMsgFolder::isCommandEnabled
> should die. Whether its current logic should move to the front-end, or should
> move to the appropriate nsIMsgFolder attributes remains a trickier question.

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

To give a little more context here, I think we need to do a bit of thinking about encapsulation.  In general, commands are a front-end concept, and the backend having special knowledge of them is not such a great idea, because it then entrains backend and front-end semantics together very closely.  This is a problem when there are impedence mismatches between front-end and back-end concepts (eg think about how the semantics of Google IMAP's delete command has historically, and maybe still, differed from the generic IMAP delete command).
 
Entrainment also makes it hard for folks new to the code base to think about small chunks of the code without already having an overview of much more.

I do think it's entirely possible that the backend does need something like what isCommandEnabled does, but the current assumption that it shares a conceptual namespace with the front-end is likely to be confusing and lead to trouble over the long haul.

Comment 2

[Magnus Melin [:mkmelin]]

We should revive this. From skimming it, there's not necessarily too much to do

https://searchfox.org/comm-central/search?q=folder.*.isCommandEnabled&case=false&regexp=true&path=

• removing https://searchfox.org/comm-central/rev/72694e3d97e48704952acf28871e0b64a38081f9/mailnews/imap/src/nsImapMailFolder.cpp#8147 and checking the places those few commands are used. Apparently all it's really doing is checking if we're offline.

aceman, any interest?

Comment 3

[:aceman]

It seems to be an interesting concept that the folder itself knows whether it can be renamed or deleted right now (e.g. due to being offline). It is useful for IMAP, but may also be useful for some future folder types.
What should we replace it with? Should the front-end know about the folder type peculiarities? I'm not sure about that. We have enough hard-coding of account types in the front-end that should be pushed down to the server implementation in some way and this bug here seems to me to go in the opposite direction.

Comment 4

[Magnus Melin [:mkmelin]]

I think it's very hard to avoid the hard coding of account types in the front-end. Anyway, we already have canRename and canCompact (which would/should!) give you this knowledge to begin with. Checking if a named front-end command is enabled by querying the back-end for that is simply wrong. We don't have a folder.canDelete, but that could be added.