272490 - UI inconsistencies for virtual folders (saved searches) for news accounts

Status: NEW

(Thunderbird :: Mail Window Front End, defect)

Description

[sairuh (rarely reading bugmail)]

found using 200411300x-0.9 on linux fc2 and mac os x 10.3.6.

RSS accounts
------------
(a) you can create virtual folders (saved searches) from Views, Quicksearch,
Search Messages dialog, and File > New > Saved Search menu item --just like for
mail accounts. however, the context menu for such a virtual folder additionally
contains "Manage Subscriptions" --shouldn't that be removed?

News accounts
-------------
(b) you can create virtual folders from the same places in (a) *except* that the
File > New > Saved Search menu item is hidden. since it's functional elsewhere,
why not expose it?

(c) the context menu for a News virtual folder is odd:

Open in New Mail Window (that's fine)
Unsubscribe (not applicable --should be Delete Folder)
Get Messages for Account (not applicable?)
Mark Newsgroup Read (not applicable?)
Properties (still works fine)
...Rename Folder is missing, too

(d) the icon for a News virtual folder is the same as the newsgroup folder.
screenshot coming up.

Comment 1

[sairuh (rarely reading bugmail)]

Attachment: virtual folders in both RSS and News accounts

"joey-recent" is the RSS virtual folder, and "news-virt" is the News virtual
folder.

Comment 2

[sairuh (rarely reading bugmail)]

(e) unique to virtual folders in News: bringing up a mail compose window (ctrl+M
or cmd+shift+M) places the name of the virtual folder, which it shouldn't (To:
should be blank). guess tbird somehow thinks it's viewing a real newsgroup
(which it isn't).

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

We're having some progress on this -- are we?

Comment 4

[:aceman]

Attachment: patch

Some of the problems of comment 0 seem already solved in current TB.
So this patch fixes the rest of the cases (does not solve e), that is out of scope).

I remove some of the hardcoded assumptions in the context menu JS code and use the nsIMsgFolder::canRename, deletable, etc. attributes. And then I fix the needed functions in nsNewsFolder.cpp.

I have not yet run tests with this, but is this a good direction?

Comment 5

[Joshua Cranmer [:jcranmer]]

Comment on attachment 715742 [details] [diff] [review]
patch

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

In general, I approve of getting rid of hardcoded type checks wherever possible.

That said, I am mildly scared by the idea that news folders can be virtual. As can be seen from the changes in nsNewsFolder, the NNTP code isn't set up to think that it can ever have a saved folder as a sibling anywhere, and I think NNTP virtual folders are going to suffer from lots of minor nit points that are going to be annoying to keep pruning. AFAICT, it was always meant to be disabled in the UI, but some "accidental bugs" allow people to make it happen. That it works at all is completely surprising to me, and I think I would rather have the UI exclude the possibility of creating virtual folders.

One "crazy" idea I've wanted to experiment with for several years is the idea that a server can have folders of different type underneath it (one useful side effect is to be able to put Sent and Trash folders with the news server). I don't think the results in such a scenario are useful enough to warrant the amount of assumptions that will break, and it would probably be much easier to just have a layer that "tricks" the UI into creating these kinds of ephemeral folders (so a news server wouldn't be able to tell that it had a virtual folder hanging underneath it, e.g.).

Comment 6

[Joshua Cranmer [:jcranmer]]

FWIW, I'm clearing the feedback request because I don't think I'm the right person to ask if this is a good idea or not. You are going to start touching some fundamental assumptions about how folders and servers work if you go down this path, and that needs a wider discussion and decision than just myself (e.g., I'd want to hear rkent's thoughts).

Comment 7

[:aceman]

Attachment: patch v2

Comment 8

[:aceman]

On the other hand, this is removing items from the context menu of the virtual folders that do not apply, so there should be less possibility of problems. So it is not blanket removal of the feature yet but limiting of the problems. See also the other 2 patches I submitted yesterday.
That is why I propose the nsIMsgFolder::canCreateVirtualFolders attribute and hook it up where needed. Then we could decide which types of servers/folders can have virtual folders (e.g. IM can't, it just fortunately is not shown in the folder pane currently). And if we then decide news should not have them, we can disable them easily. I'd do that in bug 515024.

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 715773 [details] [diff] [review]
patch v2

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

Looks ok to me, though you should also take CanRenameDeleteJunkMail into account

Comment 10

[Kent James (:rkent)]

Comment on patch v2:

Since you have reduced function CanDeleteFolder(folder) to a single line, it seems to me that you should just eliminate that function and replace its calls with folder.deletable

"That is why I propose the nsIMsgFolder::canCreateVirtualFolders attribute and hook it up where needed."

Also as a general philosophy, I am all in favor of reducing the size of these interfaces whenever possible. Given that, I wonder if it would be a good idea to just constructions like server.getBoolValue("canCreateVirtualFolders") for new attributes, and make sure that getBoolValue defaults to "false" when the underlying pref has no default, rather than keep expanding the definition of nsIMsgIncomingServer. We could take a similar approach to nsIMsgFolder::canCreateVirtualFolders

As for folder versus server attributes, if there are cases for both then there is always the inheritable folder properties that work for either.

Comment 11

[Joshua Cranmer [:jcranmer]]

(In reply to Kent James (:rkent) from comment #10)
> Also as a general philosophy, I am all in favor of reducing the size of
> these interfaces whenever possible. Given that, I wonder if it would be a
> good idea to just constructions like
> server.getBoolValue("canCreateVirtualFolders") for new attributes, and make
> sure that getBoolValue defaults to "false" when the underlying pref has no
> default, rather than keep expanding the definition of nsIMsgIncomingServer.
> We could take a similar approach to nsIMsgFolder::canCreateVirtualFolders

There are some issues I have with this approach, which amount to the following:
1. This feels like an abuse of preferences (a customization system) to indicate static or pseudo-static properties. In particular, account creation would be forced to add a list of preferences to indicate capabilities, and it's not immediately clear offhand where this creation list would happen.
2. Preference getters I believe throw if they are not present, but any wrapping mechanism would force the default of all of these capabilities to either be affirmative or negative.
3. Requesting a string capability sucks because XPIDL doesn't allow for string constants in interfaces, which means listing them for documentation purposes is error-prone.

As long as we accept the notion that we're going to have all implementations inherit from nsMsgIncomingServer (even JS-implemented ones, via an overriding mechanism), I don't think the approach of several "can I do X?" queries is an unreasonable one (especially if we have lots of [infallible] attributes on IDL to make it easier for C++ consumers).

A feasible middle ground I can think of is a bool hasCapability(in uint32_t capability) method (treating capability as an enum of possible values rather than a 32-bit flags value). Actually, internally to an nsMsgIncomingServer implementation, capabilities could be specified as one of four values: yes, no, yes unless a pref says otherwise, and no unless a pref says otherwise.

Comment 12

[Joshua Cranmer [:jcranmer]]

Comment on attachment 715773 [details] [diff] [review]
patch v2

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

I don't think I'm prepared to jump down the rabbit hole of letting news accounts have virtual folders; judging from the other patches you've asked me for review, we have too many invariants assuming that "folder on server.type == "nntp" is a newsgroup."

I do applaud your attempts to simplify checks in the UI to capability checks, and would like to see more of them, but I haven't gone through the code to verify the correctness of simplifications.

::: mailnews/news/src/nsNewsFolder.cpp
@@ +855,5 @@
> +  bool isServer;
> +  GetIsServer(&isServer);
> +
> +  // Only if this is a virtual folder (saved search) it is deletable.
> +  *deletable = !isServer || (mFlags & nsMsgFolderFlags::Virtual);

This looks wrong: if isServer is false, *deletable becomes unconditionally true.

Comment 13

[:aceman]

(In reply to Joshua Cranmer [:jcranmer] from comment #12)
> I don't think I'm prepared to jump down the rabbit hole of letting news
> accounts have virtual folders; judging from the other patches you've asked
> me for review, we have too many invariants assuming that "folder on
> server.type == "nntp" is a newsgroup."

The fact is, we already allow news have virtual folders. So how do you propose to remove this damage and make existing users migrate away from them?

Comment 14

[:aceman]

(In reply to Joshua Cranmer [:jcranmer] from comment #11)
> A feasible middle ground I can think of is a bool hasCapability(in uint32_t
> capability) method (treating capability as an enum of possible values rather
> than a 32-bit flags value). Actually, internally to an nsMsgIncomingServer
> implementation, capabilities could be specified as one of four values: yes,
> no, yes unless a pref says otherwise, and no unless a pref says otherwise.

I would love this but I do not like the fact that this would add a third way so access capabilities (in addition to server attributes and protocolInfo attributes).

> ::: mailnews/news/src/nsNewsFolder.cpp
> @@ +855,5 @@
> > +  bool isServer;
> > +  GetIsServer(&isServer);
> > +
> > +  // Only if this is a virtual folder (saved search) it is deletable.
> > +  *deletable = !isServer || (mFlags & nsMsgFolderFlags::Virtual);
> 
> This looks wrong: if isServer is false, *deletable becomes unconditionally
> true.

Should have been && .

Comment 15

[Joshua Cranmer [:jcranmer]]

(In reply to :aceman from comment #13)
> The fact is, we already allow news have virtual folders. So how do you
> propose to remove this damage and make existing users migrate away from them?

At this point, my preferred plan of action would be to shut down UI entry points to create these things, and then work on a system that would implement virtual folders in a manner that would be transparent to the actual server implementations.

In the intermediate period, sticking our heads in the sand here sounds like the best course of action--users who are willing to tolerate the current mess would get migrated automatically to a non-broken solution. This does, however, presume that a good general solution to virtual folders would be eventually available.

Comment 16

[Magnus Melin [:mkmelin]]

(In reply to Joshua Cranmer [:jcranmer] from comment #15)
> At this point, my preferred plan of action would be to shut down UI entry
> points to create these things, and then work on a system that would
> implement virtual folders in a manner that would be transparent to the
> actual server implementations.

I'm not sure what you mean by this, but folderdisplay has a few isNews helpers. FWIW, i do think virtual folders work fine for news, and certainly for feeds - i used to use them a lot earlier.

Comment 17

[:aceman]

Thanks, jcranmer, I'll update the patches in this direction.

Magnus, the plan is not to disable virtual folders operating on news. But to disable creation of virtual folders under a news account. The supported solution is to create them e.g. under Local folders but have them operate (search) on news "folders".

Is that fine?

Comment 18

[Joshua Cranmer [:jcranmer]]

(In reply to :aceman from comment #17)
> Thanks, jcranmer, I'll update the patches in this direction.
> 
> Magnus, the plan is not to disable virtual folders operating on news. But to
> disable creation of virtual folders under a news account. The supported
> solution is to create them e.g. under Local folders but have them operate
> (search) on news "folders".
> 
> Is that fine?

To clarify, the situation I most want to avoid is having a virtual folder that is implemented by nsMsgNewsFolder. I have no problems with virtual folders that operate on news folders.

Comment 19

[Magnus Melin [:mkmelin]]

(In reply to :aceman from comment #17)
> Is that fine?

Yeah I suppose there's a certain logic to it.

Comment 20

[alta88]

comment 0, part (a) no longer applies.

Comment 21

[:aceman]

It appears to me that after bug 878805 it is also not possible to save a search under a news account. That would be a welcome change so I want to note it here.