Skip to content
This repository has been archived by the owner on Oct 16, 2020. It is now read-only.

Regarding issue #21: Added preferences flag only_favorite_folders #45

Closed
wants to merge 2 commits into from
Closed

Conversation

Cybso
Copy link

@Cybso Cybso commented Jun 6, 2012

Added preferences flag only_favorite_folders. If enabled, only folders marked as 'favorite' will be regarded when counting new/unread mails.

If enabled, only folders marked as 'favorite' will be regarded.
@foudfou
Copy link
Owner

foudfou commented Jun 6, 2012

Hi, looks interesting. I'll merge it when I get a chance. Looking at your commit, 2 remarks popped up:

  • how should only_favorite_folders relate to included accounts (excludedAccounts in pref mail_accounts) and serverTypes ?
  • how does only_favorite_folders relate to FIRETRAY_MESSAGE_COUNT_TYPE_*

Just personal thoughts for the record.

@Cybso
Copy link
Author

Cybso commented Jun 7, 2012

The handling of only_favorite_folders is done in unreadMsgCountIterate and newMsgCountIterate, so excluded accounts and children of folders with excluded flags won't be included by this parameter.

A different approach, with much more complexity and changes to the current behavior which might lead to regressions, is to remove excludedFoldersFlags and replace it by includedFoldersFlags (with an extra item for "normal" folders). Inboxes and normal folders would be included by default. The check would be moved from countMessages() to unreadMsgCountIterate() and newMsgCountIterate(). In this case, folder.getNumUnread(true) couldn't be used any longer, since the check has to be done for each folder separately (my patch enables this behavior if both recursive and only_favorite are enabled).

But what happens to folders that are flagged, e.g., with INBOX and VIRTUAL? Maybe excludedFoldersFlags might not be removed, but there should be something like an order flag (include first or exclude first). Even more complexity... :-/

The check for excluded_folders_flag was moved from countMessages
to unreadMsgCountIterate and newMsgCountIterate. This should fix
#44
@Cybso
Copy link
Author

Cybso commented Jun 7, 2012

The second commit (906e64d) is a fix for issue #44

@foudfou
Copy link
Owner

foudfou commented Jun 13, 2012

First, thank you for the awesome idea and patch ! I hope to be able to look at it closely soon.

I'm not sure to follow you well, but should we just disable included special folders+accounts when include favorite folder is checked (in the preference window), and vice versa ? Or should we provide the ability to have some favorite folders and some accounts for ex. ?
In other words, do we want something like restrict to favorite folders -> disables Included special folders but keeps Included accounts ?
Or even restrict to favorite folders + checkbox strictly => disable Included special folders and Included accounts ?

@Cybso
Copy link
Author

Cybso commented Jun 14, 2012

I'm not sure if this is a good idea because some people may mark sent or draft or virtual folders as favorite to appear at the top of the folder list (although i disabled this feature for me, but if someone wants to use both this would become a problem).

Maybe it's possible to introduce own folder flags and advance the folder preference pane with a new "tray" page to select this. This would be much easier than displaying and synchronizing the complete folder list in our own preference pane. But I think I must confess that I'm not really an AddOn developer. I had my first look at the source and api, uhm, 8 days ago ;-)

@Cybso
Copy link
Author

Cybso commented Jun 14, 2012

Seems that nsIMsgFolder.setStringProperty(AString, AString) and nsIMsgFolder.getStringProperty(AString) are perfect for this. I couldn't find further documentation, but it seems that these functions do persistant storage of custom key/value pairs, e.g. "extension.firetray.ignored=true". And the AddOn "FolderFlags" has code that shows how to introduce a folder preference pane.

Let me suggest an opt-out implementation like in KMail. Per default, all folders are counted (extension.firetray.ignored = null or "false"), but you can disable this for each folder separatly (extension.firetray.ignored = "true"). This wouldn't break the behavior for upgrading users.

@foudfou
Copy link
Owner

foudfou commented Jul 9, 2012

nsIMsgFolder.setStringProperty look fine to me. And adding a "firetray include" (or exclude) in the context-menu of folders is a nice idea. So do you suggest to drop this patch ?
If not, I think the current patch does not provide in the preference window a clear understanding of the relation between server-type exclusion, folder-type exclusion, and folder inclusion (using favorites):

  1. excluded folder types precede favorite selections, which is not obvious by the name "Only favorite folders"
  2. how about also adding favorites to the "Included accounts" tree ?
  3. how about a radio with "all folders"/"favorite folders" instead of a checkbox ?

I want to emphasize that your implementation is OK. Only the UI would needs enhancements.

Further enhancements, like "firetray include" in the context-menu, could be implemented in a second step.

@Cybso
Copy link
Author

Cybso commented Jul 9, 2012

If I had the time at the moment to write a context menu (or folder property menu) patch I would drop the original patch but the part that fixes issue #44. But as you said, this enhancement could be easily introduced as a second step by importing all favourite folders to the initial "firetray include" list.

UI... well... I may be a skilled programmer, but I've never been an UI expert ;-) I ask myself what others think about this question...

@foudfou
Copy link
Owner

foudfou commented Sep 4, 2012

I merged your patches and further worked on the message counting and UI aspects. I hope everything still works OK, and that the UI is explicit enough. It would be nice if you could test and provide feedback.
I'll close this issue but feel free to re-open it if necessary.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants