81321 - Offline: Delete/move IMAP folder should be disabled when offline

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Backend, defect, P1)

Description

[laurel]

Using may16 commercial trunk build

Delete Folder is enabled for IMAP folders when in offline state. Able to go
through the motions, Confirm Move to Trash dialog will appear and is able to be
OK'd but nothing happens, folder not moved.

Since delete (IMAP) or move folder is not supported when offline, we should disable.

Affects:
Menu item
Context menu item
Del key
Toolbar button
Drag&Drop should display invalid operation cursor (slashed circle)

Note:  delete POP/local folder works,moves to trash when offline.

Comment 1

[David :Bienvenu]

disabling is frontend (js) work. Reassigning to Diane. Please reassign if neccesary.

Comment 2

[Diane Sun]

Attachment: Attatch fix

Comment 3

[grylchan]

Diane. How about 'rename folders'. In 4.x we disabled this option.
Currently it is available in 2001051704 builds. But when you try
to rename it, nothing happens. You can enter the new name but the ok 
button doesn't work. 

Not sure if your attachment addressed this or not. 
I can log a new bug if you want.

Comment 4

[David :Bienvenu]

this just disables folder delete, not msg delete, right? and yes, rename and
create should also be disabled.

Comment 5

[Diane Sun]

I can disable the rename folder.  In 4.x  we can create folder offline.  Do we 
need to disable create folder while offline,  David?

Comment 6

[David :Bienvenu]

well, it doesn't work right now - it wouldn't be too hard to get it working in
the backend but I haven't done it yet. Why don't we disable it for now and if I
get extra cycles to implement it, we can enable it later.

Comment 7

[Diane Sun]

OK,   I can disable create folder  for now.
I'll also disable "Compact This Folder" and "Compact Folders"  while offline.

Comment 8

[Diane Sun]

Attachment: Disable Delete/Rename/Compact Folder/EmptyTrash for Offline IMAP

Comment 9

[scottputterman]

*** Bug 81316 has been marked as a duplicate of this bug. ***

Comment 10

[racham]

    return IsRenameFolderEnabled() && (CheckOnline() || IsNotImapFolder());
is used in cmd_delete block ..? 

I think this case has to be reworked (remove ISRenameFolderEnabled()here)..move
the existing code under the case button_delete into function (returning a
boolean) and then couple it with (CheckOnline() || IsNotImapFolder()).

Comment 11

[Diane Sun]

Attachment: Modified Patch

Comment 12

[racham]

Please change the name of the var from nospecial to canDeleteThisFolder (or
something similar). nospecial is confusing as we already have another var named
specialFolder.

Also, that particular routine is horribly misaligned even prior to your changes.
Please adjust the indentation so that it's more readable. thanks.

Take care of the above ==> r=bhuvan.

Comment 13

[Diane Sun]

Attachment: changed.

Comment 14

[Diane Sun]

The alignment had been taken care, but it could be not shown in the diff.

Comment 15

[racham]

Thanks for taking care of indentation. 

r=bhuvan.

Comment 16

[Jussi-Pekka Mantere]

PDT+ per 6/12 mtg.

Comment 17

[Diane Sun]

Request SR again

Comment 18

[David :Bienvenu]

you can leave empty trash enabled while offline - I have a fix for that.

Comment 19

[(not reading, please use [e-mail] instead)]

I don't like adding "IsNotImapFolder()"

it seems like what we're really getting at is certain operations are not 
allowed on certain types of folders when offline.

I'd rather see this:

add a js method "IsFolderCommandEnabled(command)".  the bulk of it would be 
just like IsNotImapFolder, but instead of getting the server from the folder 
and then checking the type, you'd call this method on nsIMsgFolder

boolean isCommandEnabled(in string command);

the base implementation in nsMsgDBFolder.cpp would return true and not check 
the offline state.  (an improvement over your current implementation which 
checks the offline state every time, no matter what.

for the imap implementation, we'd override isCommandEnabled() so that 
for "cmd_compactFolder", "cmd_renameFolder", etc
we'd check the offline state and if necessary, the server domain.

the reason we might have to check the server domain is:
do we allow the user to ever compact folders on AOL mail accounts?  do we 
disable the compact folders UI in that case?  If that's an existing bug, we 
could use this new "isCommandEnabled()" to fix that problem.

(see how bhuvan made it so offlineSupportLevel is overriden on a per domain 
basis.)

note, I'm not suggesting moving all the isCommandEnabled() logic into C++.
this is just a way to override it generically on a per folder basis.

at first I was going to suggest we put it in nsMsgDBView, where we already have 
getCommandStatus(), but we might not always have a db view.  (for example, some 
day we'll be able to right click and do context menu actions on folders without 
loading the folder.)

comments?

Comment 20

[Diane Sun]

Seth's comments covered a very broad area. It feels like a complete 
consideration and implementation for menu items Compact Folders/Rename Folder. 
Current AOL mail accounts does not support compact folders,  is that correct?
Since this bugs is related to offline command only.  I would like to suggest to 
consider Seth' suggestions in the future AOL mail/Web mail, or menu item's 
modification.  What do you think?

Comment 21

[(not reading, please use [e-mail] instead)]

again, I don't want to see us adding "IsNotImapFolder()".  this is not the way
to go.

I still suggest we add to the nsIMsgFolder interface a way to check if commands
are enabled.  the base implementation will return true and you'll override the
nsImapMailFolder implemenation to check if we are online or not.

the extra benefit of this is, for pop folders, we will not be checking on line,
we will not be checking if the folder is imap or not.  remember, most users use pop.

also, instead of slapping on the code like this:

-        return IsEmptyTrashEnabled();
+        return IsEmptyTrashEnabled() && (CheckOnline() || IsNotImapFolder());

you should fix IsEmptyTrashEnabled() to get the selected folder and then call
folder.isCommandEnabled(command);

The same goes for IsEmptyTrashEnabled(),IsCompactFolderEnabled(), etc.

please fix this the right way.

Comment 22

[Diane Sun]

Move to .9.3 to use different approach based on Seth's suggestion. 

Comment 23

[Diane Sun]

Attachment: New Approach to diable Delete/Compact/Rename Folder(s) while offline

Comment 24

[Diane Sun]

Attachment: Fix some indent problem in mail3PaneWindowCommands.js

Comment 25

[Jussi-Pekka Mantere]

Removing PDT+, marking nsbranch; should be checked into the 0.9.3 trunk ASAP,
tested & verified, then submitted for limbo builds.

Comment 26

[racham]

Patch looks better than the last one in terms of generalization. But it still
doesn't cover per domain based attribute values. But when in offline mode, how
does the domain matter ? No IMAP server will allow rename, delete or
compact..right ? Correct me if I am wrong. If what I said is correct, 
then the patch is OK in general. Just take care of 
1. Add a new line at the end of nsImapMailFolder.cpp
2. Move (CheckOnLine() || CommandEnabledForOffline(cmd)) into correponding
routines like IsRenameFolderEnabled().

As per as domain based enabling when we are online, we need some work to be done
to make sure we reflect the right choices of the servers (per domain). Following
canFileMessagesOnServer attribute (in nsIMsgIncomingServer.idl couple with
changes in nsImapMailFolder.cpp) model will give us the right results. We have
to override canRename, canComapct and CanDelete folder attributes (just like
canFileMessages) in nsImapMailFolder to achieve that. May be we should all that
in a separate bug..? Seth, what do you think ?

bhuvan

Comment 27

[Diane Sun]

Attachment: New Attachment with Bhuvan's suggestion

Comment 28

[Diane Sun]

I agree with Bhuvan's oppinion regarding per domain based enabling. While 
offline, no domain is related.  For AOL mail and web mail, until we are clear 
about its definition & functionality. We'll know it should be enabled or not and  
it is different from Offline enabling. I agree we handle domain issue in a 
seperate bug. 

Comment 29

[racham]

Can't you do the same thing (like IsRenameFolderEnabled) to
IsCompactFolderEnabled() moving the online/offline checks to into the function
itself ? I think mailwindowoverlay.js does get the visibility of these functions. 

Also, please choose better names than 'rv' in these kind of cases :

+        var rv;
 
	var folderNode = folderList[0];
-
	return(folderNode.getAttribute("CanRename") == "true");
+        rv = (folderNode.getAttribute("CanRename") == "true");
+        return rv && (CheckOnline() ||
isCommandEnabledForOffline("cmd_renameFolder"));

something like folderCanBeRenamed or canRenameFolder or whatever you think is
the best. 

Take care of those. Run some test cases. r=bhuvan.

If you have to rework the patch significantly with any new ideas Seth may have,
please do request for re-review.

Comment 30

[Diane Sun]

Attachment: Minor changes based on Bhuvan's comments.

Comment 31

[(not reading, please use [e-mail] instead)]

1)  move the offline check into the back end (and only do it when necessary)

+                        return canDeleteThisFolder && (CheckOnline() || 
isCommandEnabledForOffline(command)); 

+        return canRename && (CheckOnline() || isCommandEnabledForOffline
("cmd_renameFolder"));

putting your CheckOnline() here means that for the common case (local folders) 
you are doing more work that you have to.   you should be doing as little work 
as possible for the common case.  

I suggest changing it to isCommandEnabled() and push the offline check into the 
back end.   You only need to check the online state in 
nsImapMailFolder::IsCommandEnabled().  The base class will still return true in 
all cases.

2) use NS_ENSURE_ARG_POINTER() instead of NS_ENSURE_ARG(), since result is a 
PRBool *.

+  NS_ENSURE_ARG(result);

this should be

+  NS_ENSURE_ARG_POINTER(result);

3)  why the string copy?

+
+NS_IMETHODIMP nsImapMailFolder::IsCommandEnabledForOffline(const char 
*command, PRBool *result)
+{
+  NS_ENSURE_ARG(result);

this should be 

+ NS_ENSURE_ARG_POINTER(result);
+ NS_ENSURE_ARG_POINTER(command);

+  nsCString  commandStr(command);
+  
+  if((!commandStr.IsEmpty()) && (commandStr.Equals("cmd_renameFolder") ||
+     commandStr.Equals("cmd_compactFolder") || commandStr.Equals
("cmd_delete")))

why did you copy the string to the heap?  There is no need for a string copy.  
you can just use nsCRT::strcmp(command,"cmd_delete");

Comment 32

[(not reading, please use [e-mail] instead)]

more comments:

4) make sure you didn't forget about "button_delete"

5) clean up more code

adding isCommandEnabled() to nsIMsgFolder allows us to clean up more of 
mail3PaneWindowCommands.js

instead of doing: (specialFolder == "Inbox" || specialFolder == "Trash" || 
isServer == "true")

you can add that check to nsMsgDBFolder::IsCommandEnabled() and check if the 
folder is the server and check the folder flags

we can clean up even more by implementing nsNewsFolder::IsCommandEnabled() to 
remove the checks against server type.

this additional clean up is optional.  if you choose not to do it with this 
patch (to miminize risk), let me know.  I'll log a new bug that depends on this 
bug to track the cleanup.  

please get a re-review from racham before requesting a super review.

Comment 33

[Diane Sun]

Attachment: New patch based on Seth's new comments

Comment 34

[Diane Sun]

Attachment: Add button_delete

Comment 35

[Diane Sun]

Seth, I'm going to skip the additional cleanup for comment 5 since it needs more  
investigation and has more risk.  Could you log another bug please.

Comment 36

[racham]

* I guess now that you pushed online/offline check to backend, you can go with
the name Seth suggested i.e., IsCommandEnabled() (which basically checks for
online state and also commands).

* nsCRT::strcmp(command, "button_delete")
==>
  (nsCRT::strcmp(command, "button_delete") == 0)

* use additional set of brackets to cover the command set check

==>
+  if(WeAreOffline() && 
      ((nsCRT::strcmp(command, "cmd_renameFolder") == 0) ||
+      (nsCRT::strcmp(command, "cmd_compactFolder") == 0) ||
+      (nsCRT::strcmp(command, "cmd_delete") == 0) || 
       (nsCRT::strcmp(command, "button_delete") == 0)))
  * covering each command with brackets is optional.

bhuvan

Comment 37

[(not reading, please use [e-mail] instead)]

as I mentioned in my last review:

1)
use IsCommandEnabled(), not IsCommandEnabledForOffline().

2)

+  NS_ENSURE_ARG(result);

should be

+  NS_ENSURE_ARG_POINTER(result);

3)  did you test this?  it looks like it would always return false since you 
forgot a "== 0" (on your button delete test.)

+  if(WeAreOffline() && (nsCRT::strcmp(command, "cmd_renameFolder") == 0 ||
+     nsCRT::strcmp(command, "cmd_compactFolder") == 0 ||
+     nsCRT::strcmp(command, "cmd_delete") == 0) || nsCRT::strcmp
(command, "button_delete") )
+     *result = PR_FALSE;

and, instead of using == 0, you can use !.

it also looks like you forgot a ), but it must be my eyes, since that would 
mean this patch doesn't even compile.

Comment 38

[(not reading, please use [e-mail] instead)]

I spun off the code cleanup to bug #88926

Comment 39

[Diane Sun]

Attachment: New patch to cover the comments

Comment 40

[racham]

Also, how about 

isCommandEnabledForOffline(command)

==>

isCommandEnabled(command)

in js files. No need to post a new patch just for this again.

Certainly run testcases with all these commands after you are done with r & sr.
good work.

r=bhuvan

Comment 41

[Diane Sun]

Attachment: Final patch, get rid of "ForOffline"

Comment 42

[(not reading, please use [e-mail] instead)]

does this code cause JS exceptions when you use the stand alone msg window?

isCommandEnabled(cmd) is defined in mail3PaneWindowCommands.js, but referenced 
in mailWindowOverlay.js

Comment 43

[Diane Sun]

Tested in stand alone messenger,  no JS exception happened.

Comment 44

[(not reading, please use [e-mail] instead)]

ok, thanks for testing.

sr=sspitzer

Comment 45

[Diane Sun]

Fix checked in.

Comment 46

[Jussi-Pekka Mantere]

Adding vtrunk; bugs need to be verified in the trunk for eventual limbo checkins.

Comment 47

[Jussi-Pekka Mantere]

Clearing resolution, nsBranch bugs should be closed only after checkins into
limbo builds.

Comment 48

[grylchan]

Commercial builds
2001-07-05-09-trunk/ win nt 4.0
2001-07-05-08-trunk/ linux 2.2, red hat 7.0
2001-07-05-08-trunk/ mac os 9.0.4

Looking at IMAP accounts/folders only (ie not verifying behavior
on pop, webmail ,local folders, etc..) since this a imap specific bug.

There is a slight problem.

If you Right click and bring up the properties dialog for
any folder, the following are still enabled:
  Rename folder, Compact Folder, Delete Folder
But if you try to rename/delete/compact nothing will work.
For both Rename/Delete a seperate window does pop up. In the
case of Delete you hit ok but nothing happens. For rename, you
can rename but hitting ok doens't get rid of the window. The
user must hit cancel or the x button.

The following are disabled when imap account is offline:
 -From File menu:
   -Rename Folder, Compact Folder
 -From Edit menu:
   -Delete Folder
 -Clicking on the Delete button on the keyboard results in
  no folder being deleted
 -From Messenger toolbar:
   -Delete button is disabled
 -Drag & Drop
  -when you select a folder to drag to the trash, the slashed cirlce
   initially appears. The slashed cirlce dissapears as you physically try 
   to move the folder to say the trash can. But the folder never gets
   physically moved to trash can.
 -tried opening a message in seperate window and going to the file menu.
  Rename/Compact folders is disabled.



Leaving status of bug as is. If Jussi, Diane, etc.. feel the 
right hand properties options on each folder is a seperate
bug, please let me know and I'll put my verified comments
in status white board and in the comments.

I can't tell from the initial bug if we are just to disable
the options on Messanger menu only or via properties dialog.

Comment 49

[grylchan]

Per Diane's email, I will create a new bug to disable 
compact folders/rename folders/disable folders.

Verified on trunk.

Removing Keyword Vtrunk.

adding 'verified on trunk' to status whiteboard.

Leaving status as Assigned per instructions from Jussi.

Comment 50

[Jussi-Pekka Mantere]

Gary, can you copy the contents of the email here? I don't get how this can be 
closed / verified based just on your comments.

Comment 51

[grylchan]

Jussi.

Here is the email:
Diane Sun wrote:
> 
> Gary,
> 
>   Could you mark this as verified and file another bug for the right
> hand pop-up menu?   Thanks,
> 
> Diane

again I wasn't sure whether the right hand click menu should
be part of this bug or not. That's why I deferred to yourself, Diane,
etc..

Comment 52

[Diane Sun]

Attachment: Additional Fix for context menu item

Comment 53

[Diane Sun]

Request addtional R & SR for attachment 41477 [details] [diff] [review]

Comment 54

[racham]

r=bhuvan.

Comment 55

[Håkan Waara]

+ EnableMenuItem("folderPaneContext-remove", isCommandEnabled("cmd_delete") ||
isCommandEnabled("button_delete") );

I'd guess the button listens to the same command as you do, so why check that
too? Is it just me -- or is that redudant?

+<script src="chrome://messenger/content/mail3PaneWindowCommands.js"/>

You are not modifying anything in that file. What does this addition have to do
with the rest of the fix? What is this fixing?

Comment 56

[Diane Sun]

+ EnableMenuItem("folderPaneContext-remove", isCommandEnabled("cmd_delete") ||
isCommandEnabled("button_delete") );
will make isCommandEnabled to use either cmd_delete or button_delete to disable 
offline delete folder in nsImapMailFolder.cpp. 

+<script src="chrome://messenger/content/mail3PaneWindowCommands.js"/>
will make mailContextMenus.js to use isCommandEnabled() defined/implemented in 
mail3PaneWindowCommands.js

Comment 57

[Håkan Waara]

Sounds fine. r=hwaara too then

Comment 58

[(not reading, please use [e-mail] instead)]

nope, this patch needs work.

isCommandEnabled() doesn't contain all the logic that the 
FolderPaneController.isCommandEnabled() does.

there might be some cases where the context menus are enabled, but the menu 
items are disabled.  (or vice versa.)

this is why I logged bug #88926.  If we push the logic into the folder 
implementations of isCommandEnabled() and out of mail3PaneWindowCommand.js, 
then this wouldn't be a problem.

that said, the spirit of your patch makes things better, and when #88926 is 
fixed, any edge cases where the folder context menu and the menu item don't 
match up will go away.

but, there are still problems, as hwaara pointed out.

mailWindowOverlay.xul
+<script src="chrome://messenger/content/mail3PaneWindowCommands.js"/>

adding mailWindowOverlay.xul will pull all the 3 pane code into the stand alone 
message window.  So in addition to making the stand alone message window slower 
to launch, I have a feeling it will break a bunch of stuff.  notice how both 
mail3PaneWindowCommands.js and messageWindow.js define the same functions.

instead, I'd do this:  in mailContextMenus.js, you've got exactly one folder, 
targetFolder.  so calling isCommandEnabled() does more work than you need 
anyways.

instead, go from targetFolder (probably a nsXULElement) to the nsIMsgFolder and 
then call folder.isCommandEnabled().   [use GetMsgFolderFromNode()]

hwaara is also right about the redundancy.  I don't think you need to check for 
both cmd_delete and button_delete.  I think you just want "cmd_delete".

Comment 59

[Diane Sun]

folder.isCommandEnabled()  or targetFolder.isCommandEnabled() does not work here

Comment 60

[(not reading, please use [e-mail] instead)]

> folder.isCommandEnabled()  or targetFolder.isCommandEnabled() does not work here

are you saying you can't go from targetFolder to nsIMsgFolder?  we've got code
that does that.

you can't do targetFolder.isCommandEnabled(), targetFolder is probably an
nsXULElement or something.

when you say "folder.isCommandEnabled()" doesn't work, do you mean that 

var folder = GetMsgFolderFromNode(targetFolder);
folder.isCommandEnabled("cmd_delete");

failed?  why do I feel like I'm writing this patch?

Comment 61

[Diane Sun]

Attachment: New Addtional patch for context menu item

Comment 62

[Diane Sun]

Request addtional  re- R & SR for attachment 41502 [details] [diff] [review]

Comment 63

[(not reading, please use [e-mail] instead)]

there's no reason to enable / disable if we aren't showing the item.

how about something like:

ShowMenuItem("folderPaneContext-remove", showRemove);
if (showRemove) {
 var folder = GetMsgFolderFromNode(targetFolder);
 EnableMenuItem("folderPaneContext-remove",folder.isCommandEnabled("cmd_delete"));
}

Comment 64

[Diane Sun]

Attachment: Additional fix for context menu

Comment 65

[Diane Sun]

Request addtional  re- R & SR for attachment 43012 [details] [diff] [review]

Comment 66

[Diane Sun]

Request R & SR for attachment 43012 [details] [diff] [review] again.

Comment 67

[Blake Ross]

Missed the 0.9.3 train.

Comment 68

[(not reading, please use [e-mail] instead)]

sr=sspitzer

Comment 69

[(not reading, please use [e-mail] instead)]

note, I think the same idea of 

ShowMenuItem(XYZ, foo);
if (foo)
{
  EnableMenuItem(XYZ, bar);
}

could be applied to all three cases here, not just the one.
but that code cleanup can happen later.

Comment 70

[Diane Sun]

Checked in. .9.4

Comment 71

[grylchan]

Commercial builds:
2001-08-15-09-trunk/ - nt 4.0
2001-08-15-14-trunk- linux 2.2, mac 9.0.4
The following are disabled when imap account is offline:
 -From File menu:
   -Rename Folder, Compact Folder
 -From Edit menu:
   -Delete Folder
 -Clicking on the Delete button on the keyboard results in
  no folder being deleted
 -From Messenger toolbar:
   -Delete button is disabled
 -Drag & Drop
  -when you select a folder to drag to the trash, the slashed cirlce
   initially appears. The slashed cirlce dissapears as you physically try 
   to move the folder to say the trash can. But the folder never gets
   physically moved to trash can.
 -tried opening a message in seperate window and going to the file menu.
  Rename/Compact folders is disabled.
 -Using the Context menu
  Rename/Compact/Delete is unavailable

Tested both themes. Marking as verified.