289208 - user-visible preference for check all IMAP folders

Status: NEW

(Thunderbird :: Preferences, enhancement)

Description

[Mikel Ward]

I am running server-side filtration on my IMAP mail server, so I find the
"mail.check_all_imap_folders_for_new" almost essential.  Without this, messages
that are automatically filtered into a folder other than "INBOX" can go unnoticed.

The only alternative is to manually right click on each of my many folders and
select "Check this folder for new messages".  This method is tedious and I
suspect it will go unnoticed by some users who would either expect all folders
to be checked by default or for there to be a global preference.

Please make this a check box in the preferences dialog.  It's been a hidden
preference for a very long time now, yet the feature is very useful.

I guess it might go under Account Settings->Account Name->Server Settings.

Comment 1

[Inventus]

Hi,

I hope this is the right bug to write this comment in? At least I don't think the following should be considered a new bug:

I too have some trouble with the new mail notification in TB (version 1.5.0.7 / 20060909). This problem have persisted since I started using TB some years back (though the specific nature of the issue may have changed somewhat over time?)

I use IMAP and a single server-side filter (to move server-side filtered spam to my "Junk" folder). My mail.check_all_imap_folders_for_new is FALSE (since I do NOT want to get notified about arriving spam). I have enabled sound-notification (so playing this sound, and/or showing the new-mail icon in the Windows tray is what I mean by "notify" below).

TB will sometimes notify me about messages arriving in the "Junk" folder, most notably this seem to happen every time I browse this folder while there are unread messages in it. The same goes for any other folder, including my "Sent" folder. It would even appear that simply changing folders (ie. viewing "INBOX", then viewing "Junk"), will sometimes (if not always?) cause TB to notify me about mail in other folders (even other than "INBOX" or "Junk", typically the "Sent" folder).

Also, it would seem that the "new mail" icon (in the Windows tray) is removed when viewing a folder with unread messages, even if there are still unread mail in the folder, rather than being shown until all unread mail has been marked as read (not sure if this is intentional, but if so, it would be nice to be able to chose if this should be the case or not).

Aside from the fact that something appear to be messed up with the new mail notification in TB, I also find the notification options quite lacking. As such I should think it ought at least be possible to select which specific folders (and accounts, in case of POP3) one whish to recieve notifications about. Even better would be an option to use costumised notifications (specifically sounds) for each folder/account, so as to allow one to differentiate between arriving mails.

Finally it would be nice if the "new mail" tray-icon (ie. envelope with green arrow) replaced the TB icon in the Windows tray, rather than occuring at the end of the tray-icon line (this is, of course, only relevant when the TB icon is already present in the tray). It seems somewhat superflous to show two seperate icons (sometimes not even beside eachother, if other tray apps. have been launced after TB, but before the notification icon is shown).

Note: I also have a POP3 account (Gmail) which TB also notifies me about, but ideally I'd like to be notified only about messages in my IMAP INBOX (or even better, I would like to use a different notification-sound for the Gmail account, as suggested above).

Thanks in advance for fixing these issues, and for considering my suggestions. Keep up the good work!


Regards,

privacy[dot]lover[at]gmail[dot]com

Comment 2

[Lorenzo Mancini]

Attachment: trunk.patch

I propose a patch for this bug. It:

 * removes the global check_all_imap_folders_for_new preference;
 * adds a local (per-server) check_all_folders_for_new preference;
 * adds a checkbox for enabling/disabling the preference, that only appears for IMAP servers;
 * adds an attribute and the generator for a getter method, to check for the preference at runtime;
 * in the cpp code, retrieves the value for checkAllFolders variabile querying the new preference.

This solves the original issue as described in comment #0. Comment #1 from Inventus IMHO is not related to the purpose of the bug, and a new bug should therefore be opened for it.

Comment 3

[David :Bienvenu]

The problem with this is that all the people who have this hidden pref set will suddenly find it not working - or is there some code to migrate the original pref to the per-server pref?

Comment 4

[Lorenzo Mancini]

(In reply to comment #3)
> The problem with this is that all the people who have this hidden pref set will
> suddenly find it not working - or is there some code to migrate the original
> pref to the per-server pref?

You are right: after applying this patch, people who set the hidden option will have to re-enable it manually.

But, if we want to have automatic migration of it, the problem is that a general infrastructure to migrate preferences doesn't seem to exist (or at least I couldn't find it and neither some folks on #developers and #maildev could).

I could change the accessor to that preference, adding code that *always* checks if the old preference exists and the new one doesn't, and if so creates the latter, but this would then be code to mantain forever just because we didn't want to drop an old preference (not to mention unnecessary overhead on every read of that preference).

Other suggestions?

Comment 5

[Mikel Ward]

I don't care if it's just a relnotes thing, but can't a setting have a set/unset/default (often depicted as checked, blank, gray boxes).

if (server.check == default)
{
  check = check_all;
}


  

Comment 6

[Lorenzo Mancini]

Attachment: checkallfolders.patch

I've reworked the previously submitted patch, adding a method ShouldCheckAllFolders which first checks for the old preference's existence and value. If there is no old preference, or the old preference has its default value, the new preference is used instead.

With profiles created after the inclusion of this patch, only the new preference will be used, because the old preference isn't created anymore in profiles.

To summarize, this patch does everything already stated in comment #2, but keeps in count the legacy preference value, so people with that one set won't suddenly find it not working anymore.

Comment 7

[David :Bienvenu]

Comment on attachment 281091 [details] [diff] [review]
checkallfolders.patch

some minor nits:

should be a space after the ',', here:
+pref("mail.server.default.check_all_folders_for_new",false);

similarly, should be a space after the "if" here:

+  if(checkAllFolders)

here:

+  PRBool checkAllFolders = PR_FALSE;
+  checkAllFolders = ShouldCheckAllFolders(imapServer);


should just be

PRBool checkAllFolders = ShouldCheckAllFolders(imapServer);

Here, you're ignoring rv, should might as well not set it:

+  // This pref might not exist, which is OK. We'll only check inbox and marked ones
+  rv = prefBranch->GetBoolPref("mail.check_all_imap_folders_for_new", &checkAllFolders);

instead, just say

(void) prefBranch->GetBoolPref("mail.check_all_imap_folders_for_new", &checkAllFolders);

Can you submit a patch with those changes addressed? I'm not sure if Scott's going to want to expose the UI for this in account settings, so we'll have to ask him for sr on that part, but the backend part looks OK to me, thx for the patch!

Comment 8

[Lorenzo Mancini]

Attachment: checkallfolders2.patch

David, thanks for your review. I submitted a reworked patch and marked it as waiting for mscott's sr.

Comment 12

[Nikolay Shopik]

David, patch isn't reviewed for half year by mscott can we get this moving into sr by somebody else?

Comment 13

[David :Bienvenu]

Comment on attachment 281231 [details] [diff] [review]
checkallfolders2.patch

I'm going to look at this again, and ask Bryan if he thinks this should be in the UI...

Comment 14

[David :Bienvenu]

cc'ing Bryan for his thoughts about making this a user-visible per-server imap pref.

Comment 15

[[e-mail]]

Comment on attachment 281231 [details] [diff] [review]
checkallfolders2.patch

>Index: mail/locales/en-US/chrome/messenger/am-server-top.dtd
I've no objections, as long as suite's locale is updated too ;-)

>+<!ENTITY checkAllFoldersForNew.label "Check all folders for new messages">
This needs an access key. (I know, there aren't any spare. Bugs are filed.)

>+pref("mail.server.default.check_all_folders_for_new", false);
Oh wow, we've got a mix of _underlined_ and interCaps pref names :-(

>+  <vbox align="start" hidefor="pop3,nntp,movemail">
>+      <checkbox wsm_persist="true" id="imap.checkAllFoldersForNew"
>+              label="&checkAllFoldersForNew.label;"
>+              prefattribute="value"
Nit: the checkbox line is indented two spaces too far.

>+// FIXME: helper function to know whether we should check all IMAP folders
>+// for new mail; this is necessary because of a legacy hidden preference
>+// mail.check_all_imap_folders_for_new (now replaced by per-server preference
>+// mail.server.%serverkey%.check_all_folders_for_new), still present in some
>+// profiles.
Hmm... perhaps we can arrange a one-time migration of mail.c_a_i_f_f_n to mail.server.default.c_a_i_f_f_n somehow (release notes if necessary, although SeaMonkey could do it in its profile migrator.)

>+  PRBool ShouldCheckAllFolders(nsIImapIncomingServer *imapServer);
This could be a static method.

Comment 16

[Lorenzo Mancini]

(In reply to comment #15)

> >+  <vbox align="start" hidefor="pop3,nntp,movemail">
> >+      <checkbox wsm_persist="true" id="imap.checkAllFoldersForNew"
> >+              label="&checkAllFoldersForNew.label;"
> >+              prefattribute="value"
> Nit: the checkbox line is indented two spaces too far.

Fixed.

> >+// FIXME: helper function to know whether we should check all IMAP folders
> >+// for new mail; this is necessary because of a legacy hidden preference
> >+// mail.check_all_imap_folders_for_new (now replaced by per-server preference
> >+// mail.server.%serverkey%.check_all_folders_for_new), still present in some
> >+// profiles.
> Hmm... perhaps we can arrange a one-time migration of mail.c_a_i_f_f_n to
> mail.server.default.c_a_i_f_f_n somehow (release notes if necessary, although
> SeaMonkey could do it in its profile migrator.)

Yes, it would be much better to migrate those preferences once and for all, than having to mantain that legacy-related code forever.  When I initially wrote the patch, there was no known method to perform such a migration (see comment #4).  Can this profile migrator actually help?

The profile migrator code is pretty much new to me however.  Is the profile migrator available from the mailnews app?  Is it already used for tasks similar to the one we just mentioned?  I began to look here, but I'm not sure it's the right starting point:

http://mxr.mozilla.org/thunderbird/source/mail/components/migration/src/nsProfileMigrator.cpp

> >+  PRBool ShouldCheckAllFolders(nsIImapIncomingServer *imapServer);
> This could be a static method.

Yes, it doesn't use any of the instance's state.  Made static.

Comment 17

[Lorenzo Mancini]

Attachment: checkallfolders3.patch

Comment 18

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

This UI seems in conflict with bug 436615 which is making this automatic, but honoring the folder subscription choices.

Comment 19

[Magnus Melin [:mkmelin]]

Well, close but not exactly. This is about check_all_folders_for_new (explicitly checking for new). I think bug 428266 is what you're thinking of.

Comment 20

[Lorenzo Mancini]

(In reply to comment #18)
> This UI seems in conflict with bug 436615 which is making this automatic, but
> honoring the folder subscription choices.

I don't think they're in conflict, but complementary.  This one is about checking all IMAP folders when fetching new mail, and bug 436615 is about using offline mailnews features to fetch new mail.

(In reply to comment #19)
> Well, close but not exactly. This is about check_all_folders_for_new
> (explicitly checking for new). I think bug 428266 is what you're thinking of.

This used to be true, but bug 428266 has taken a whole different direction during discussion; actually its whiteboard suggests to start reading at comment 16.  I'm notifying them of this bug, anyway.

Comment 21

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

(In reply to comment #20)
> I don't think they're in conflict, but complementary.  This one is about
> checking all IMAP folders when fetching new mail, and bug 436615 is about 
> using offline mailnews features to fetch new mail.

Ah, ok.  I didn't really see any discussion about why this needs a preference?  Why can't we just check all folders every time?  If it's a problem about mail notification, that seems like a different bug.

Comment 22

[Magnus Melin [:mkmelin]]

See the discussion in bug 428266. You have to have server side filters to have any use for it -> some power users like it. On the other hand it creates a lot more traffic and adds a lot of strain on the server as the user could easily have 100s of folders, making ISPs/universities reluctant to recommend tb if we had it on by default.

Comment 23

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

> See the discussion in bug 428266. You have to have server side filters to have
> any use for it -> some power users like it.

Right, but even GMail users will require this if they have filters that skip the Inbox.

> On the other hand it creates a lot
> more traffic and adds a lot of strain on the server as the user could easily
> have 100s of folders, making ISPs/universities reluctant to recommend tb if we
> had it on by default.

I think our IMAP logic needs to handle this, having a pref for it seems like a way to escape the responsibility of doing the right thing for our users. 

For systems with lots of folders I would think it'd be better for our IMAP code to attempt to be kind to the servers using some kind of "all folders checked over the course of a couple of Inbox checks" algorithm.

I'm just not sure how to explain to a regular person that even though they can see all their folders we don't check them for new mail because they didn't turn that option on.  Of course I wouldn't want to explain to people why they got their IMAP account shutoff because we were over eager.  But a preference for this just means we're giving people the option to be in either of those situations without really knowing the consequences.

Comment 24

[Magnus Melin [:mkmelin]]

(In reply to comment #23)
> Right, but even GMail users will require this if they have filters that skip
> the Inbox.

You can't skip the Inbox in Gmail. Though sure, for Gmail check all folders, or at least bug 428266 would be wanted.

Comment 25

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

> You can't skip the Inbox in Gmail.

Sure you can, it's the first option in GMail filter actions - Skip the Inbox (Archive it).  (go to: create filter, next step - first option)

Comment 26

[Magnus Melin [:mkmelin]]

Ah sorry, missed that!

Comment 27

[Lorenzo Mancini]

(In reply to comment #23)

> I think our IMAP logic needs to handle this, having a pref for it seems like a
> way to escape the responsibility of doing the right thing for our users.

> I'm just not sure how to explain to a regular person that even though they can
> see all their folders we don't check them for new mail because they didn't turn
> that option on.

Ok, then I hope you're ready to explain that same regular person that achieving the same behaviour he is used to with other MUAs, he will have either to:

 a. hack a hidden preference, using an "Advanced options" panel, or
 b. patiently wait for the mail check implementation you described, that just does The Right Thing (TM), but that nobody has even starting working on.

*This* seems escaping responsibilities to me ;)

This is not about adding random GUI controls here and there, the "check all folders" imap problem is well known and every MUA I've used except Thunderbird has a GUI option for it: Evolution has, Outlook has been having that since version 6, even more obscure ones like Sylpheed-Claws do.  I can't try Eudora but I wouldn't be surprised to find that option there too.


> Of course I wouldn't want to explain to people why they got
> their IMAP account shutoff because we were over eager.  But a preference for
> this just means we're giving people the option to be in either of those
> situations without really knowing the consequences.

Again, I disagree.  This is about not exposing a useful option which imap people will need anyway at some point.  Please check:

http://www.google.com/search?q=check_all_imap_folders_for_new

Comment 28

[Nikolay Shopik]

David are we stuck with that patch for review because of UI inconsistency?

Comment 29

[David :Bienvenu]

It didn't sound like Bryan was in favor of the UI but if you haven't tried requesting a ui-review, you could do so - that's the process.

With imap auto sync, we will automatically update/sync all folders, though not on every check for new mail.

Comment 30

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

perhaps if we went with a simple and easy algorithm for this instead of something complicated.  If the problem is traffic / load for lots of folders why don't we just turn this option off when there are lots of folders; however many lots actually is.  It's not ideal for user experience because there's a chance that some users will go past the threshold and then the system will change inexplicably.  However this would enable the system for most users who would really benefit from this.  Enabling by default would mean that people only had to use the about:config when there is a problem or they go past the threshold and want this behaviour to continue.

But I can't really see approving this kind of pref ui.

Comment 31

[Lorenzo Mancini]

(In reply to comment #30)
> perhaps if we went with a simple and easy algorithm for this instead of
> something complicated.  If the problem is traffic / load for lots of folders

Is it?

> why don't we just turn this option off when there are lots of folders; however
> many lots actually is.  It's not ideal for user experience because there's a
> chance that some users will go past the threshold and then the system will
> change inexplicably.  However this would enable the system for most users who
> would really benefit from this.  Enabling by default would mean that people
> only had to use the about:config when there is a problem or they go past the
> threshold and want this behaviour to continue.

If I understood correctly, after implementing your proposal all IMAP folders will be checked for new mail if 1) you haven't passed a certain hidden arbitrary threshold or 2) a hidden preference is set to true.  You can get the same behaviour exposing the pref in the UI, having it true by default, and let users turn it off if their IMAP admins complain.

Comment 32

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

> > perhaps if we went with a simple and easy algorithm for this instead of
> > something complicated.  If the problem is traffic / load for lots of folders
> 
> Is it?

That's what I understood from magnus in comment #22

> If I understood correctly, after implementing your proposal all IMAP folders
> will be checked for new mail if 1) you haven't passed a certain hidden
> arbitrary threshold or 2) a hidden preference is set to true.  You can get the
> same behaviour exposing the pref in the UI, having it true by default, and let
> users turn it off if their IMAP admins complain.

Or we could not have the pref UI and default to true such that users can turn off the hidden pref when their admins complain.  Even simpler :)

Comment 34

[David :Bienvenu]

Comment on attachment 335792 [details] [diff] [review]
checkallfolders3.patch

this needs UI review first. As long as this defaults to false, I don't mind it so much, though I still think a hidden pref for the few users who need all folders checked is reasonable.

Comment 35

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

Comment on attachment 335792 [details] [diff] [review]
checkallfolders3.patch

(In reply to comment #34)
> Comment on attachment 335792 [details] [diff] [review]
> checkallfolders3.patch
> 
> this needs UI review first. As long as this defaults to false, I don't mind it
> so much, though I still think a hidden pref for the few users who need all
> folders checked is reasonable.

Agreed, this seems fine with just a hidden pref.

We just need some docs about the pref and what it does instead of the pref UI
https://wiki.mozilla.org/Thunderbird:Help_Documentation:Hidden_Preferences

Comment 36

[Matthieu Pupat]

Do I understand correctly that you will not change anything besides some documentation page?

I hardly understand why adding friendly GUI options is such a problem in general in Thunderbird and even less in this particular case where there already is a GUI option which is tedious to activate multiple times and it could be improved by having a central place to activate it.

Comment 37

[Mark Banner (:standard8)]

Comment on attachment 335792 [details] [diff] [review]
checkallfolders3.patch

Cancelling empty review request, as per the ui-review this needs rework to remove the UI elements of the change.

Comment 38

[Mark Banner (:standard8)]

(In reply to comment #36)
> I hardly understand why adding friendly GUI options is such a problem in
> general in Thunderbird and even less in this particular case where there
> already is a GUI option which is tedious to activate multiple times and it
> could be improved by having a central place to activate it.

To try and summarise the previous comments on this bug:

- Checking all folders for new mail seems to be only needed for a small numbers of users where they have server-side filtering of email.
- Turning on checking all folders for new mail can hurt performance and load on the imap server (and as a result selective turning on of checking mail on folders is therefore better if not all folders are necessary).

Both of these indicate that a UI addition would unnecessarily complicate the UI for the majority of users, as well as potentially hurting their performance if they did turn it on without realising the implications.

In these situations we tend not to take the UI change, as shown above, and defer to add-ons - if the add-on community wants to create an add-on to provide an interface to tweak this option then they are quite welcome to, and we'll do our best to provide them with the necessary hooks if they aren't already there.

Comment 39

[Giovanni Bajo]

I have yet to see concrete numbers backing up the "excessive load" on the IMAP server or the performance implications.

I also dispute that there is a small number of users that do server-side filtering. Setting aside the fact that client-side filtering on IMAP is semantically wrong unless you access your e-mail only from a single computer (which is less and less the case with all the mobile devices, pads, etc.), at least ALL users of webmails do server-side filtering; so you should count in all gmail, yahoo, hotmail users for instance.

Comment 41

[Ludovic Hirlimann [:Usul]]

Lorenzo are you still working on making a patch for this bug ?

Comment 42

[TGOS]

Let me throw another e-mail provider: Not just Gmail, Yahoo and Hotmail, also GMX allows sever side filtering and GMX is the biggest E-mail provider of Germany.

The argument that this causes too heavy server load on IMAP servers is bogus, since alone in the USA 14% of all desktop users are Mac users and the vast majority of them uses Apple Mail as their e-mail client, and Apple mail always checks all sub-folder for new mails on IMAP accounts. I never heard anywhere that this is any problem to anyone.

Why can't Thunderdbird have an option to enable a feature that is enabled by default in other mail clients? Those other mail clients may even have a bigger market share than Thunderdbird currently does. Why do the developers insist on nagging users to change this flag on a per folder basis? Most people not aware of the mail.check_all_imap_folders_for_new flag currently manually tag this option for all their IMAP folders anyway.

To add another use case: Our company has shared IMAP folders. Those are shared between users and we have hundreds of those. Updating this flag for each shared folder I have subscribed to is really PITA.

Yes, I can use mail.check_all_imap_folders_for_new, I actually did, but this setting has no UI and is not officially supported, which means if it doesn't work correctly, I cannot even complain, since an unsupported feature doesn't have to work correctly, does it? Further there may be a case where I may not want this behavior for a certain IMAP account; this should definitely be a "per account" setting.

So can we please add this setting for all IMAP accounts, can we please make it checked by default and can we please grey out the per folder setting on all folders of an account if this setting is checked, so that people immediately see that this setting is currently ignored, since the per account setting has been checked? This bug is almost 6 years old, I fail to see why we cannot just add this stupid checkbox and continue with other issues instead of arguing another 6 years. Shall Thunderbird become a real full featured mail client, or shall it continue to be lacking important features that you find pretty much everywhere else? Geez, this bug and the missing "Redirect" feature make me really considering switching to another client once and for all.

Comment 43

[Lorenzo Mancini]

(In reply to comment #41)
> Lorenzo are you still working on making a patch for this bug ?

I somewhat lost interest when I switched to GMail web interface for my personal e-mail.  I'd be happy anyway to update it to apply against the current codebase if it's going to be integrated.

Comment 44

[Lorenzo Mancini]

Attachment: Updated patch to apply against current comm-central hg trunk

Comment 45

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 526221 [details] [diff] [review]
Updated patch to apply against current comm-central hg trunk

Review of attachment 526221 [details] [diff] [review]:

I'm not really happy with this for a couple of reasons.

1) I have mail.check_all_imap_folders_for_new set to true, but somehow mail.server.default.check_all_folders_for_new got set to false.  Now I'm kind of unclear whether or not all my mail folders will be checked.  (It looks like they won't, which seems like the wrong choice to me.)

2) This addition pushes the "Local Directory" field down below the bottom of the dialog.  Sure, I _could_ resize it, but that's kind of a hacky workaround.

What do you think of a UI like http://dl.dropbox.com/u/2301433/CheckAll.png instead?

Thanks,
Blake.

Comment 46

[TGOS]

(In reply to comment #45)
> What do you think of a UI like http://dl.dropbox.com/u/2301433/CheckAll.png
> instead?

1) What sense makes "no folders"? If I want no folders to be checked, I would uncheck the checkbox in front, that's what it exists for, doesn't it?

2) "some folders" is pretty bad wording, since which folders are those? Will Thunderbird randomly pick them or choose whatever folders I read most often? It pretty much tells the user nothing.

3) The dropdown exists only for the first setting, so it would initially check the selected folders at start-up, but I would like it check all my IMAP folders every xxx minutes as well, which means we would have to duplicate that dropdown. However, then we'd also need two settings, since the user might want all folders to be checked at start up, but only INBOX every xxx minutes... and so on. This will only complicate things for most users with little benefit for anyone.

This setting could be added as a third line, like

[_] Check for new messages at startup
[_] Check for new messages every ___ minutes
Check [...] for new messages.

Where [...] would be something like:

- "only the inbox"
- "only selected folders"
- "all existing folders"

However, I still wonder if that is really necessary or if a simple checkbox will already cut it.

Comment 47

[Lorenzo Mancini]

(In reply to comment #45)

Hello Blake, thanks for your review.

> 1) I have mail.check_all_imap_folders_for_new set to true, but somehow
> mail.server.default.check_all_folders_for_new got set to false.  Now I'm kind
> of unclear whether or not all my mail folders will be checked.  (It looks like
> they won't, which seems like the wrong choice to me.)

Unless I ignore some of the nsImapMailFolder class internals, in the scenario you describe all of your mail folders will actually be checked.

In detail, the new function ShouldCheckAllFolders treats the hidden preference mail.check_all_imap_folders_for_new as an override: if it's true, then all folders are checked, if it's false (or it doesn't exist), then the new preference is taken into account.

I chose this behaviour to respect mail.check_all_imap_folders_for_new semantics.


> 2) This addition pushes the "Local Directory" field down below the bottom of
> the dialog.  Sure, I _could_ resize it, but that's kind of a hacky workaround.

Indeed.  Can you give me a pointer about where to fix this?  I'd be glad to solve this one and a similar problem that happens on the "Account Settings" window, which (at least on current comm-central hg trunk) has the "Manage Identities..." button below its bottom.  


> What do you think of a UI like http://dl.dropbox.com/u/2301433/CheckAll.png
> instead?

I understand that you propose this to have a unified UI control; can you develop on what "some folders" means?

Thanks,
Lorenzo

Comment 48

[Mark Banner (:standard8)]

Comment on attachment 526221 [details] [diff] [review]
Updated patch to apply against current comm-central hg trunk

Cancelling review for now as this got ui-review-.

Blake: can you respond to Lorenzo's comment please?

Comment 49

[Blake Winton (:bwinton) (:☕️)]

(In reply to comment #47)
> Hello Blake, thanks for your review.

You're very welcome, and I'm sorry I didn't see your reply earlier, or I would have responded a while ago.

> > 1) I have mail.check_all_imap_folders_for_new set to true, but somehow
> > mail.server.default.check_all_folders_for_new got set to false.  Now I'm kind
> > of unclear whether or not all my mail folders will be checked.  (It looks like
> > they won't, which seems like the wrong choice to me.)
> 
> Unless I ignore some of the nsImapMailFolder class internals, in the scenario
> you describe all of your mail folders will actually be checked.
> 
> I chose this behaviour to respect mail.check_all_imap_folders_for_new
> semantics.

Okay, that's perfect.

> > 2) This addition pushes the "Local Directory" field down below the bottom of
> > the dialog.  Sure, I _could_ resize it, but that's kind of a hacky workaround.
> 
> Indeed.  Can you give me a pointer about where to fix this?  I'd be glad to
> solve this one and a similar problem that happens on the "Account Settings"
> window, which (at least on current comm-central hg trunk) has the "Manage
> Identities..." button below its bottom.  

Well, I would really like us to fix it by re-doing some of the options on that page to get everything to fit.  But if you wanted to just resize it, it looks like we do that in mail/locales/en-US/chrome/messenger/AccountManager.dtd

> > What do you think of a UI like http://dl.dropbox.com/u/2301433/CheckAll.png
> > instead?
> I understand that you propose this to have a unified UI control; can you
> develop on what "some folders" means?

Yeah, "some folders" was a bad choice.  TGOS's suggestion of "selected folders" is more what I meant.

But now that I think about it a little more, and re-read the previous comments, what about just removing the UI for this iteration, leaving it as a hidden pref, and getting it checked in.  Then, after that, we can iterate on what the correct preference UI should be (either here, or in a different bug).

Thanks,
Blake.

Comment 50

[Lorenzo Mancini]

(In reply to comment #49)
> But now that I think about it a little more, and re-read the previous comments,
> what about just removing the UI for this iteration, leaving it as a hidden
> pref, and getting it checked in.  Then, after that, we can iterate on what the
> correct preference UI should be (either here, or in a different bug).

Makes sense to me; I've splitted the patch in two parts, one that adds the new preference and the other that adds the UI for it.

Just to be sure we're in sync, note that the first patch removes the global mail.check_all_imap_folders_for_new; this means that it will not be created anymore for new profiles.  Of course, we still want to handle it for already existing profiles, hence the handling code in nsImapMailFolder::ShouldCheckAllFolders.  The new preference to control this behaviour will be the per-server mail.server.default.check_all_folders_for_new .

I'm attaching the two patches.

Comment 51

[Lorenzo Mancini]

Attachment: Remove global preference, add per-server preference to check all imap folders (preference patch)

Comment 52

[Lorenzo Mancini]

Attachment: Remove hidden global preference, add exposed per-server preference to check all imap folders (UI patch)

Comment 53

[Mark Banner (:standard8)]

Comment on attachment 530152 [details] [diff] [review]
Remove global preference, add per-server preference to check all imap folders (preference patch)

David's already looked at this bug and earlier versions of the patch, so I think he's better to review this patch.

Comment 54

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 530153 [details] [diff] [review]
Remove hidden global preference, add exposed per-server preference to check all imap folders (UI patch)

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

As mentioned before, I think we can get a better UI for this feature than just adding a checkbox, and if we land the hidden pref, we'll have time to work out what that better UI should be.

Thanks,
Blake.

Comment 55

[David :Bienvenu]

Attachment: prev patch, but with new uuid for changed interface

I tweaked your prev patch to add a new uuid for the changed interface. Other than that, it looks ok.

Comment 56

[David :Bienvenu]

I'm not sure I got the revved uid in the prev patch, but I've landed a patch that does have that change - http://hg.mozilla.org/comm-central/rev/00652232e453

Comment 57

[Ludovic Hirlimann [:Usul]]

Why is this still open ?

Comment 58

[David :Bienvenu]

I think only the backend changes for a per-server pref to check all folders on the server for new messages landed, not the UI for that per-server pref.