509397 - Merge UI to manage Subscriptions and Advanced Offline Settings

Status: UNCONFIRMED

(Thunderbird :: Preferences, enhancement)

Description

[marvin greenberg]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1.2) Gecko/20090729 Firefox/3.5.2 GTB5
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1.1) Gecko/20090715 Thunderbird/3.0b3

From a developer perspective these two functions are totally different, one about communication with the imap server the other about decisions the thunderbird client makes about retrieving messages.  

But from a user perspective, they are closely related.  "Yes, I want this folder visible AND I want its contents available when disconnected."  Merging the UIs would make this continuum more evident.

My initial confusion about these two different settings was an artifact of bug 493455 -- unsubscribed folders do not normally show up in the advanced offline settings (but currently for gmail folders).  Because they show up in Offline Settings even when unsubscribed, it was unclear to me what it meant to sync when not subscribed.  

But even if they do disappear from Advanced Offline Settings when unsubscribed, I believe it is better from a user perspective to merge these related concepts into a single UI.  

I would propose changing the existing subscription dialog to have two checkboxes [subscribe X  sync offline X]  (with behavior that clearing subscribe clears or hides the sync offline checkbox)

Reproducible: Always

Comment 1

[Thomas Schmid]

Hi,

I invested some time to look at the code. Mixing the two dialogs is not as trivial as I expected. 

Some ideas concerning the implementation

The current current subscribe implementation is somehow interesting, realy old and relies on a rdf mechanism, which are exclusively used by the subscribe dialog.
Both Imap as well as NTP Account implement the SubscribableServer Interface and class which is just a rdf wraper interface. It is created by StartPopulating and destroyed by calling StopPopulating. I did not find any spot through out Thunderbird's code where it is reused.

As there's a ongoing effort to get rid of rdf. I would suggest a bigger change. Drop the SubscribableServer, nsISubscribeListener Interface and the corresponding nsSubscribeDataSource.cpp aswell as nsSubscribableServer.cpp and switch to a callback driven Interface, which is more suitable for javascript. 

Concerning the implementation nsNntpIncommingServer.cpp semms to be even worse than nsImapIncommingServer.cpp . I do not fully understand what the code does or is supposed to do. On the one hand Subscribable folders are added to internal treeview representation but they are also added to the rdf. But only the RDF source is used. 
Adding unsubscribed folders to the internal treeview seems to be dead code. Can someone verify this?


In Pseudocode the change would be:

Create a nsISubscibableServer2.idl which supports the following functions (The naming can be definitely improoved, suggestions highly welcome)

nsISubscribaleServer:
  enumerateSubscribableFolder(nsIMsgWindow, nsISubscribeListener2, String uri, int cachsize);
  stopEnumerateSubscribableFolder()
  onSubscribableFolder(const nsACString &aName, bool addAsSubscribed, bool aSubscribable, bool changeIfExists);

nsISubscribaleListener:
  // Returns first all subscribed folders an then all new folders...
  onProgress(folder[URI,PATH[],subsubscribed,subscribable,replaceIfExists], count )
  onDone() //array containing folder uris 
  

In order to avoid tons of callbacks, especially within ntp servers, I though about an internal caching mechanism. 
onSubscibableFolder calls are added to an array. if cachesize is reached or stopEnumerateSubscribableFolder() is called 
onProgress is invoked and the array is passed as parameter. Is it possible to describe this in IDL?

The pseudo code below does not contain this caching mechanism, in order to keep it as simple as possible. 
The new newly created functions would be: 

NS_IMETHODIMP nsImapIncomingServer::enumerateSubscribableFolder(nsIMsgWindow, Subscribelistener, const char *uri /* can be null*/,bufsize/* defaults to 5*/)
{
  nsresult rv;

  if (mDoingSubscribeDialog)
    return NS_ERROR_FAILURE;

  mDoingSubscribeDialog = PR_TRUE;
  mStopped = FALSE;
  
  nsCOMPtr<nsIImapService> imapService = do_GetService(NS_IMAPSERVICE_CONTRACTID, &rv);
  NS_ENSURE_SUCCESS(rv,rv);

  if (!uri)
    return imapService->GetListOfFoldersOnServer(this, aMsgWindow);

  nsCString serverUri;
  rv = GetServerURI(serverUri);
  NS_ENSURE_SUCCESS(rv,rv);

   /*
     if uri = imap://user@host/foo/bar, the serverUri is imap://user@host
     to get path from uri, skip over imap://user@host + 1 (for the /)
   */

   const char *path = uri + serverUri.Length() + 1;
   return imapService->GetListOfFoldersWithPath(this, aMsgWindow, nsDependentCString(path));
}

NS_IMETHODIMP nsImapIncomingServer::StopEnumeratingFolders()
{
  if (mStopped)
    return NS_OK;

  nsCOMPtr<nsISubscribeListener> listener;
  (void) GetSubscribeListener(getter_AddRefs(listener));

  // TODO FLUSH Buffers and clean Buffers...
 
  if (listener)
    listener->OnDone();
 
  mStopped = TRUE;
}

NS_IMETHODIMP
nsImapIncomingServer::AddTo(const nsACString &aName, bool addAsSubscribed, bool aSubscribable, bool changeIfExists)
{
  // RFC 3501 allows UTF-8 in addition to modified UTF-7
  // If it's not UTF-8, it cannot be MUTF7, either. We just ignore it.
  // (otherwise we'll crash. see #63186)

  if (!MsgIsUTF8(aName))
    return NS_OK;

  nsCOMPtr<nsISubscribeListener> listener;
  (void) GetSubscribeListener(getter_AddRefs(listener));

  if (!listener)
    return NS_OK

  if (mStopped)
    return NS_ERROR_FAILURE;

  // TODO Cache Listerner in Buffer, so that not every result ends up in a separate callback
  // Iguess groups of 10 or so should be sufficent
 
  if (NS_IsAscii(aName.BeginReading(), aName.Length()))
    return listener->onProgress(aName, addAsSubscribed, aSubscribable, changeIfExists);

  nsCAutoString name;
  CopyUTF16toMUTF7(NS_ConvertUTF8toUTF16(aName), name);
  return listener->onProgress(name, addAsSubscribed, aSubscribable, changeIfExists);
}

NS_IMETHODIMP nsNntpIncomingServer::enumerateEnumeratingFolder(nsIMsgWindow, Subscribelistener, const char *uri /* can be null*/,bufsize/* defaults to 5*/)
{
  nsresult rv = NS_OK;
 
  nsCOMPtr<nsINntpService> nntpService = do_GetService(NS_NNTPSERVICE_CONTRACTID, &rv);
  NS_ENSURE_SUCCESS(rv,rv);

  nntpService->GetListOfGroupsOnServer(this, aMsgWindow, aGetOnlyNew)
  if (NS_FAILED(rv))
    return rv;
 
  return NS_OK;
}

NS_IMETHODIMP 
nsNntpIncomingServer::AddTo(const nsACString &aName, bool addAsSubscribed,
                            bool aSubscribable, bool changeIfExists)
{
  NS_ASSERTION(MsgIsUTF8(aName), "Non-UTF-8 newsgroup name");
  nsresult rv = EnsureInner();
  NS_ENSURE_SUCCESS(rv,rv);
  
  rv = AddGroupOnServer(aName);
  NS_ENSURE_SUCCESS(rv,rv);

  nsCOMPtr<nsISubscribeListener> listener;
  (void) GetSubscribeListener(getter_AddRefs(listener));
  
  if (!listener)
    return NS_OK

  listener->onProgress(aName, addAsSubscribed, aSubscribable, changeIfExists)
  NS_ENSURE_SUCCESS(rv,rv);
  
  return rv;
}

NS_IMETHODIMP nsNntpIncomingServer::StopEnumeratingEnumeratingFolders()
{
  nsresult rv = NS_OK;

  nsCOMPtr<nsISubscribeListener> listener;
  (void) GetSubscribeListener(getter_AddRefs(listener));
  NS_ENSURE_SUCCESS(rv,rv);

  // TODO FLUSH Buffers and clean Buffers...
 
  if (listener)
    listener->OnDone();

  return NS_OK;
}

NTP implements a mechanism to allow subscribing folders when being offline, 
while the IMAP implementation works only when being online.

Subscribing to folders while being offline is somehow strange, does not too 
much sense and seems to be broken. Thus i would suggest to remove this feature, 
so that subscribing to IMAP and subscribing to NTP is only possible when being online.

Comment 2

[Ludovic Hirlimann [:Usul]]

Thomas do you need help to produce a patch ?

Comment 3

[Thomas Schmid]

I am actually pretty close to a first patch, but currently I am on fullstop because my Laptop was broken got "repaired" and is broken again etc. So the one I am currently using is good enough for normal business but it's way to old to compile Thunderbird. It just takes ages. But this will hopefully change within the two weeks.

Comment 4

[Wayne Mery (:wsmwk)]

Thomas ?

(In reply to Thomas Schmid from comment #3)
> I am actually pretty close to a first patch, but currently I am on fullstop
> because my Laptop was broken got "repaired" and is broken again etc. So the
> one I am currently using is good enough for normal business but it's way to
> old to compile Thunderbird. It just takes ages. But this will hopefully
> change within the two weeks.

Comment 5

[3coma3]

Signed up to the site exactly for suggesting this feature. Would do wonders for end users.

The problem: it's annoying to go to an extra UI to set simple things liks "always check new messages for this folder", or for that matter, any other setting for a suscribed folder.

I suggest the following changes that would improve the situation:

1) Merge the context menus/windows "Suscribe" and "Properties" (Could be named "Manage folders")
2) Have, upfront, the simple view of the current folder subscription UI, but on folders the user chose to subscribe, enable extra columns for simple settings that don't have suboptions (example: "always check messages")
3) For settings with suboptions:
    1) have an advanced settings button per folder, OR
    2) in the respective column have a button with "..." that leads to extra settings, OR
    3) provide an "advanced view" for the "Manage folders UI" that would correspond more closely with the current "Properties" window.

Thank you for thunderbird