[PATCH v4] Re: make maxage use UIDs to avoid timezone issues

[Nicolas Sebrecht]

On Fri, Mar 27, 2015 at 02:38:02AM -0400, Janna Martl wrote:

> Thanks to UIDMaps magic, it turns out that this works without
> modification for the case when the local folder has type IMAP. The mess
> I noticed before is because I was using type Gmail, and Gmail-IMAP
> sync isn't supported (yet). In the IMAP case, UIDMaps.py defines a
> MappedIMAPFolder class, so I made a MappedGmailFolder case that inherits
> from GmailFolder and MappedIMAPFolder. I tried to check this in some
> basic cases (copying to/from empty folder, copying/ deleting messages,
> with/without maxsize) but this all seems too good to be true and I can't
> shake the feeling that I missed something.

What about the labels? Won't they hurt at some point?
Other than that, the following patch looks good to me.

> In order to do IMAP-IMAP sync, the local folder needs to inherit from
> MappedIMAPFolder. Make this work for Gmail as the local folder.
> ---
>  offlineimap/folder/Gmail.py        | 6 ++++++
>  offlineimap/folder/UIDMaps.py      | 4 ++--
>  offlineimap/repository/Gmail.py    | 5 +++++
>  offlineimap/repository/__init__.py | 3 ++-
>  4 files changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/offlineimap/folder/Gmail.py b/offlineimap/folder/Gmail.py
> index 93e8eee..10ee45e 100644
> --- a/offlineimap/folder/Gmail.py
> +++ b/offlineimap/folder/Gmail.py
> @@ -23,6 +23,7 @@ from offlineimap import imaputil, OfflineImapError
>  from offlineimap import imaplibutil
>  import offlineimap.accounts
>  from .IMAP import IMAPFolder
> +from offlineimap.folder.UIDMaps import MappedIMAPFolder
>  
>  """Folder implementation to support features of the Gmail IMAP server."""
>  
> @@ -369,3 +370,8 @@ class GmailFolder(IMAPFolder):
>  
>          except NotImplementedError:
>              self.ui.warn("Can't sync labels. You need to configure a local repository of type GmailMaildir")
> +
> +class MappedGmailFolder(MappedIMAPFolder, GmailFolder):
> +    def __init__(self, *args, **kwargs):
> +        MappedIMAPFolder.__init__(self, *args, **kwargs)
> +        GmailFolder.__init__(self, *args, **kwargs)

Arghh... This is very good in the sense you're following the current
design of the code you're touching. This couldn't have been better in
this matter.

Now, if we take a step back, the whole thing looks quite bad and would
require a deep (not hard) refactoring.

First, I think it was really bad for the Gmail case to import the
MappedIMAPFolder and put the mapping MappedGmailFolder outside of
UIDMaps.py.

Also, I'm worried about the lock. It might not be relevant but but in
the IMAP case, the locking comes *after* the initialization of the
IMAPFolder parent. Gmail does it before.

But the whole thing sucks. Sometimes, we are just way too much lazy.
These MappedIMAPFolder and MappedGmailFolder looks poor OOP practices.
Where is the benefit of yet another class in this case? What are we
FACTORIZING? Nothing... It's just about fixing the inheritance
hierarchy. Where is the Proxy or Factory pattern we expect?

Again, you did it right at matching the current practices.

It looks like the coming deep refactoring I aim will actually be fun.
Exciting! :-D

> diff --git a/offlineimap/folder/UIDMaps.py b/offlineimap/folder/UIDMaps.py
> index 04a986b..136b686 100644
> --- a/offlineimap/folder/UIDMaps.py
> +++ b/offlineimap/folder/UIDMaps.py
> @@ -94,8 +94,8 @@ class MappedIMAPFolder(IMAPFolder):
>                  OfflineImapError.ERROR.MESSAGE), None, exc_info()[2]
>  
>      # Interface from BaseFolder
> -    def cachemessagelist(self):
> -        self._mb.cachemessagelist()
> +    def cachemessagelist(self, maxage=None, min_uid=None):
> +        self._mb.cachemessagelist(maxage=maxage)
>          reallist = self._mb.getmessagelist()
>  
>          self.maplock.acquire()
> diff --git a/offlineimap/repository/Gmail.py b/offlineimap/repository/Gmail.py
> index 2e23e62..f85f306 100644
> --- a/offlineimap/repository/Gmail.py
> +++ b/offlineimap/repository/Gmail.py
> @@ -16,6 +16,7 @@
>  #    Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
>  
>  from offlineimap.repository.IMAP import IMAPRepository
> +from offlineimap.folder.Gmail import MappedGmailFolder
>  from offlineimap import folder, OfflineImapError
>  
>  class GmailRepository(IMAPRepository):
> @@ -72,3 +73,7 @@ class GmailRepository(IMAPRepository):
>      def getspamfolder(self):
>          #: Gmail also deletes messages upon EXPUNGE in the Spam folder
>          return  self.getconf('spamfolder','[Gmail]/Spam')
> +
> +class MappedGmailRepository(GmailRepository):
> +    def getfoldertype(self):
> +        return MappedGmailFolder

OMG... We are even faking we are fully built dynamically.

> diff --git a/offlineimap/repository/__init__.py b/offlineimap/repository/__init__.py
> index 0fbbc13..4b9ded4 100644
> --- a/offlineimap/repository/__init__.py
> +++ b/offlineimap/repository/__init__.py
> @@ -23,7 +23,7 @@ except ImportError: #python2
>      from ConfigParser import NoSectionError
>  
>  from offlineimap.repository.IMAP import IMAPRepository, MappedIMAPRepository
> -from offlineimap.repository.Gmail import GmailRepository
> +from offlineimap.repository.Gmail import GmailRepository, MappedGmailRepository
>  from offlineimap.repository.Maildir import MaildirRepository
>  from offlineimap.repository.GmailMaildir import GmailMaildirRepository
>  from offlineimap.repository.LocalStatus import LocalStatusRepository
> @@ -49,6 +49,7 @@ class Repository(object):
>          elif reqtype == 'local':
>              name = account.getconf('localrepository')
>              typemap = {'IMAP': MappedIMAPRepository,
> +                'Gmail': MappedGmailRepository,
>                  'Maildir': MaildirRepository,
>                  'GmailMaildir': GmailMaildirRepository}
-- 
Nicolas Sebrecht