---- Reported by jim@yorba.org 2011-06-03 14:57:00 -0700 ----

Original Redmine bug id: 3704
Original URL: http://redmine.yorba.org/issues/3704
Searchable id: yorba-bug-3704
Original author: Jim Nelson
Original description:

This may only apply to GMail, but it's an important exception.

According to this:

!http://www.lesnikowski.com/blog/index.php/localized-gmail-imap-folders/

GMail returns the special folders (Trash, Spam, etc.) using localized names.
This is a problem since Geary wants to know about these folders without having
to match them against GMail's localized names.

Apple and Google developed the XLIST command to get around this problem. We
should investigate how it's used and add it to Geary.

Related issues:
related to geary - 5443: Display IMAP names for special folders (Open)
related to geary - Feature #3823: internationalization: support different
languages (Fixed)
related to geary - 5217: Incorrect non-ASCII folder names (Fixed)
related to geary - 5253: "Unable to fetch special folder
[Gmail]/something" for Go... (Fixed)
related to geary - Feature #6070: Support SPECIAL-USE extension (Open)



---- Additional Comments From geary-maint@gnome.bugs 2013-05-01 11:50:00 -0700 ----

### History

####

#1

Updated by Christian Dywan over 1 year ago

  * **Description** updated (diff)

I'd like to propose targeting this for 0.1 because currently GMail is by
default broken for non-English accounts and you see only your inbox and
labels.

####

#2

Updated by Adam Dingle over 1 year ago

  * **Target version** set to _0.1_

Sounds reasonable.

####

#3

Updated by Eric Gregory over 1 year ago

  * **Subject** changed from _Use XLIST when possible_ to _Internationalization: Use XLIST when possible_

####

#4

Updated by Adam Dingle over 1 year ago

  * **Target version** deleted (<strike>_0.1_</strike>)

####

#5

Updated by Adam Dingle over 1 year ago

  * **Target version** set to _0.2_

####

#6

Updated by Timo Kluck over 1 year ago

  * **File** _0001-Enable-support-for-XLIST-properties.patch_ added

Here's some initial work to support this. It has very rough edges, because I'd

first like to get some feedback before polishing.

It makes get_special_folder_map an async method which recursively checks the
server

for xlist attributes. For now, this breaks compilation of targets other than
geary.

It includes the patch from issue #4696.

Things that need to be dealt with: * fix compilation of other targets * check
server for xlist support * probably should only use xlist when we need it and
use list otherwise * how does this interact with the local cache of the
mailboxes?

This is my first contribution. I'm looking forward to your feedback.

####

#7

Updated by Adam Dingle over 1 year ago

  * **Status** changed from _Open_ to _Review_

Timo, welcome and thanks for sending this work in progress. Jim, please review
and give feedback as requested!

####

#8

Updated by Jim Nelson over 1 year ago

  * **Assignee** set to _Jim Nelson_

####

#9

Updated by Christian Dywan over 1 year ago

I think this patch should remove the gmail_root hack alotgether. The gmail
folder itself depends on the locale, see issue #5253. And it should fallback
to standard Trash, Sent, Junk, Drafts, Queue folders - if it's not xlisted and
Geary relies on a folder, it still needs to create one.

####

#10

Updated by Christian Dywan over 1 year ago

I wonder, if generic IMAP supports "All mail" - does it mean eg. a Dovecot
plugin could benefit if Geary uses All mail as a performance tweak? Or is it
rather better to move that into the gmail account?

####

#11

Updated by Adam Dingle over 1 year ago

I think generic IMAP has no notion of an "All Mail" account. This means that
if we want to support an Archive command for non-Gmail accounts, we'll need to
let the user specify which folder messages should be archived to.

####

#12

Updated by Christian Dywan over 1 year ago

Ah, sorry, I was NOT thinking of archiving. But Jim mentioned using All mail
as a shortcut for faster notifications.

####

#13

Updated by Timo Kluck over 1 year ago

  * **File** deleted (<strike>_0001-Enable-support-for-XLIST-properties.patch_</strike>)

####

#14

Updated by Timo Kluck over 1 year ago

I've put my changes on gitorious:

https://gitorious.org/geary/geary/commits/xlist_support

It now compiles and runs without warnings, but I've had to comment out an
assertion. It doesn't fix the issue with [Gmail] being a localized name. In
fact, the last commit just removes the logic that ignores the folder, but it
doesn't come up with an alternative.

One issue is that some folders appear multiple times under the Labels tree. I
haven't figured out why.

I'm looking forward to your feedback.

####

#15

Updated by Jim Nelson over 1 year ago

  * **Category** set to _engine_
  * **Status** changed from _Review_ to _Open_

Hi Timo,

Looking over your changes, it looks like you've done a lot of work here to get
some basic support put in. I will say, when I apply your patches Geary hangs
at startup for me; not sure why, but I suspect it has to do with the issue
discussed below.

By making get_special_folders() round-trip to the server, that forces the
client to wait for a connection to be established at startup. That's no good;
Geary should launch immediately and pull up all the data it has in the local
database while the connections are being set up in the background. More
importantly, when we implement offline mode (#3767), Geary should be able to
run without a connection at all. (Not that you need to worry about this
directly now, but for both these reasons we need to find another solution.)

My thinking is that the solution goes something like this:

  * Use XLIST rather than LIST if capabilities indicates it's available. (You're doing this today.)
  * Store the mailbox attributes in the ImapFolderPropertiesTable. (This is already happening in existing code; worth verifying that the new attributes fetched via XLIST are replacing the old attributes fetched via LIST.)
  * The Account classes (including for the various personalities) use the locally-stored list of folders to determine FolderPaths to the special folders. Thus, it's okay for get_special_folders() to become async, but it should only be accessing the database, **not** the remote side.
  * Folders **not** seen by XLIST are removed from the database. This is important to ensure that the localized folder names are presented to the user.

Note that, if I'm doing the arithmetic correctly, this approach will also
solve #5253. Thus, Christian was right in his comment on that ticket.

There is an edge case here where, after logging in, special folders have moved
and so the paths in the database given to the UI are now invalid. I think this
is highly unlikely to happen with the major services and can be dealt with
later if it comes up.

####

#16

Updated by Timo Kluck over 1 year ago

Thank you for your feedback, Jim.

I've now changed it to use the local database, which was straightforward.
However, this means that the special folders will not be available on the
first run, because the `get_special_folder_map` is called before the local
database has been synced with the imap server. There is no mechanism to tell
the `GearyController` that a new special folder has been found. When you close
and restart geary, the special folders are already in the database and
everything works as expected.

Any way that I can think of to change this, asks for rethinking the
`Geary.EngineAccount` interface. Because right now, it assumes that the
SpecialFolderMap is static, whereas it may change dynamically upon connecting
to the server. We could, for example, add signals like `special_folder_added`
and `special_folder_removed`. How do you feel about that? It would need some
thinking and refactoring, though. For example, how do we determine the
ordering of the special folders?

####

#17

Updated by Timo Kluck over 1 year ago

Thinking about it some more: xlist makes being a special folder just an
attribute of a regular folder. I'm actually itching to use that as an internal
representation, as well. Now I'm not too familiar with the code, but as far as
I can see, the main thing that the SpecialFolderMap and SpecialFolder classes
handle, is the ordering of the folders. What do you feel about this
implementation:

  * we do away with the special folder classes
  * each folder gets a `special_folder_type` property, which is possibly `NONE`. For non-xlist accounts, it may be assigned based on e.g. its name.
  * or alternatively, the `Geary.EngineAccount` gets an abstract `get_special_folder_type` method, taking a folder and returning whether it is special
  * the `Geary.EngineAccount` gets an abstract comparison method, taking two (special) folders and returning their relative ordering
  * the UI checks whether a new folder is special using this interface, and adds it accordingly.

####

#18

Updated by Jim Nelson over 1 year ago

I had a similar thought over the weekend, that we should simply do away with
SpecialFolder and make special-ness a property of Folder. This will lead to
some refactoring in the client UI, as currently (to build the sidebar) it
requests all the special folders and then all the regular ones. By breaking up
the work this way, it can easily place the folder in the right place with the
right icon. By removing the SpecialFolder class, the client UI will need to do
more work to place the folder in the right spot in the sidebar. Not
impossible, but that's what will be affected by such a change.

You also asked about the first-run case where the special folders won't be
available. That's fine; first-run should come up quickly, but we can't expect
instantaneous display in this case. Once Geary has initialized the account and
seeded the database, it should startup almost instantly from then on.

Also, you asked about ordering of the special folders. It used to be that we
planned for Geary to mimic the mail service it was displaying, so the folders
would list out like Gmail or Yahoo, etc. displays them. The idea is that the
Personality interface would handle this problem. We've now decided we want a
unified UI that makes all mail services look similar, so the ordering should
be enforced by the UI.

So, I think we should get rid of SpecialFolder.and SpecialFolderMap and make
an enum that lists the various special folders. Folder then offers a getter
that lists what special folder it is (or NONE).

We will have to work out the non-XLIST case in some way. My concern is
something like Dovecot, which seems to be pretty agnostic on these matters. It
may be that the special folders are literally decided by a sysadmin when the
server is first configured. That's something we'll need to tackle at some
point, perhaps, as you suggested, using pattern matching or some-such.

Incidentally, the XLIST extension is not the only extension that offers this
feature (identifying special folders). There's a SPECIAL-USE extension that
does just that:

http://tools.ietf.org/html/rfc6154

However, XLIST also lists folders with internationalized names, the proper
goal of this ticket. It would be nice if whatever work you do here can also be
easily adapted for servers with the SPECIAL-USE extension.

####

#19

Updated by Timo Kluck over 1 year ago

My new work is here:

https://gitorious.org/geary/geary/commits/xlist_support_v2

I've rebased the subsequent commits so that they are coherent changes, but I
don't think every commit compiles. The last one does, though.

The non-xlist case hasn't been addressed yet. Neither have I looked into the
SPECIAL-USE extension.

Looking forward to your feedback, as always :-)

####

#20

Updated by Jim Nelson over 1 year ago

Timo,

This is starting to come together nicely. Here's some issues I see:

  * Instead of XLIST_INBOX, XLIST_ALL_MAIL, et al., let's call them SPECIAL_INBOX, SPECIAL_ALL_MAIL, etc.
  * Yorba's coding guidelines discourage using the var keyword unless there's a really good reason. Please replace its occurrence with static types.
  * With our strategy now moving toward the Geary interface looking the same across all services, the Geary.Personality layer will not have so many user-facing items. One thing we can drop are the strings for the different special folders, i.e. "Spam", "Trash", etc. Ordering also goes out the door, as that's for the UI to determine. I'm thinking the SpecialFolderMap should be nothing more than a mapping of FolderPaths to SpecialFolderTypes. That can be achieved with a simple Gee.HashMap inside of SpecialFolderMap. We can get rid of SpecialFolder (unless you spot some reason to keep it around).
  * With our new paradigm, the client should ignore any special folder it has displayed at the top of the sidebar when its building its Labels tree.
  * Startup time is still a concern here. I have an account with a lot of labels and the UI takes quite a bit of time to startup.

Great work!

-- Jim

####

#21

Updated by Timo Kluck over 1 year ago

> Instead of XLIST_INBOX, XLIST_ALL_MAIL, et al., let's call them
SPECIAL_INBOX, SPECIAL_ALL_MAIL, etc.

Yorba's coding guidelines discourage using the var keyword unless there's a
really good reason. Please replace its occurrence with static types.

Will do.

> With our strategy now moving toward the Geary interface looking the same
across all services, the Geary.Personality layer will not have so many user-
facing items. One thing we can drop are the strings for the different special
folders, i.e. "Spam", "Trash", etc. Ordering also goes out the door, as that's
for the UI to determine. I'm thinking the SpecialFolderMap should be nothing
more than a mapping of FolderPaths to SpecialFolderTypes. That can be achieved
with a simple Gee.HashMap inside of SpecialFolderMap. We can get rid of
SpecialFolder (unless you spot some reason to keep it around).

I'm puzzled. I already dropped the entire SpecialFolderMap class, and replaced
it with a `get_special_folder_type_async` method on the account, taking a
`FolderPath` and returning a `SpecialFolderType`. This essentially comes down
to what you're suggesting. Could you check that you have the right branch
(https://gitorious.org/geary/geary/commits/xlist_support_v2)? I'm also quite
new to git so there is a possibility that I made a mistake with that
somewhere. The commits on the webinterface look right, though.

> With our new paradigm, the client should ignore any special folder it has
displayed at the top of the sidebar when its building its Labels tree.

Will do.

> Startup time is still a concern here. I have an account with a lot of labels
and the UI takes quite a bit of time to startup.

I will have to look into that.

####

#22

Updated by Jim Nelson over 1 year ago

First, apologies for the long delay in response. I took a long weekend and am
still catching up with my email.

You're correct, I was looking at the wrong branch. I now see the work you're
speaking of.

However, what I'm suggesting is still slightly different than what's
implemented on the branch. I admit, I did suggest it in an earlier comment,
but after studying XLIST / SPECIAL-USE and seeing how it's related to #5253,
I'm now thinking of something different. In particular, I'm thinking that
get_special_folder_async() should not be a member of Account, but rather a
method of Folder (and that the enum should include a new value, "LABEL", that
is returned by non-special folders). The XLIST attributes should be saved in
the local database so that they can be fetched immediately at startup rather
than round-tripping to the server. That means the client can request the value
immediately.

Since the value **may** change on the server (although I think this unlikely),
we might need to add a signal of some kind to indicate the change has
occurred. I don't think we need to worry about this right now.

I'll continue to track your branch. Please let us know if you make additions
and please feel free to use this ticket to ask more questions.

####

#23

Updated by Timo Kluck over 1 year ago

Hi Jim,

Thanks for your feedback, but I'm already doing what you're suggesting. The
method `get_special_folder_type_async` is an implementation detail, and is
called when fetching the folder in order to assign the property. Instead of
the value "LABEL", I have added a value "NONE" for the reason you're
suggesting.

However, I have changed this halfway through my branch, so I can imagine that
when you look at all the diffs separately, you missed it. Perhaps it is
helpful to look at a diff of all my commits together.

Timo

####

#24

Updated by Jim Nelson over 1 year ago

That would help a lot Timo. I can't merely diff your branch with our trunk as
(the last time I checked) it had not been merged in a while, so a diff yields
a lot of changes.

####

#25

Updated by Timo Kluck over 1 year ago

Okay, I just merged master for your diff'ing pleasure.

####

#26

Updated by Jim Nelson over 1 year ago

Ok -- this is much better, it allows me to use meld and see exactly what's
going on.

First, yes, this is what I was talking about. However, I now see what my issue
was with the application taking so long to start. If I use a fresh database
with your changes, Geary takes a while on first load to display folders
(understandable), but from then on it starts fast and shows all the special
folders. This is desirable behavior.

But on an existing database, it takes a long time to load initially. We'd like
to avoid this, although it might be we just accept this. More importantly,
once I run this patch on an existing database, close Geary, and re-run, I
don't see any special folders other than Inbox. Additionally, I see this on
the console for the special folders:

    
    [deb] 15:42:23 geary-controller.vala:604: Found folder [Gmail]/Drafts of type GEARY_SPECIAL_FOLDER_TYPE_NONE  
     [deb] 15:42:23 folder-list.vala:156: Could not add folder to folder list: [Gmail]/Drafts

Looking at the database itself, I don't see the XLIST attributes added to the
ImapFolderPropertiesTable other than \Important.

This might be the last piece of the puzzle. I think we can live with a one-
time pause to a user the first time they run after this patch, but from then
on it should be business as usual. But an upgrade path is important to us; we
already have users running out of our daily build PPA and we'd like for them
to keep running without blowing away their database.

####

#27

Updated by Timo Kluck over 1 year ago

I've just added support for updating the folder properties in an existing
database. The update path works for users with an English (US) locale in their
gmail account, but not for users with a different locale. For some reason,
their special folders are each shown several times in the sidebar. For these
users, Geary didn't show anything but the inbox before.

This would possibly be solved when we add support for removing local folders
that don't exist remotely. It's also possible that there should be better
checking for duplicate folders in the local engine. Both things seem to be
beyond the scope of this bug.

I would be inclined to accept that these users will just have to remove their
existing database and start afresh. Especially since geary was hardly usable
for them before.

####

#28

Updated by Adam Dingle over 1 year ago

  * **Status** changed from _Open_ to _Review_

####

#29

Updated by Jim Nelson over 1 year ago

Timo,

This looks good and today I've been integrating this into trunk. I fixed some
of the bugs you mentioned plus a couple more, but all in all pretty minor. I
think this is about ready for committing to trunk. Removing local folders not
found locally will be something we do later. I do think I have a reasonable
solution to the Gmail/Google Mail problem (#5253). However, I need an
international account to test it. I'm concerned about landing this patch until
I've at least tried it out on an international account.

I'll revisit this tomorrow.

####

#30

Updated by Jim Nelson over 1 year ago

  * **Status** changed from _Review_ to _5_
  * **Resolution** set to _fixed_

c98a6b07b2ad785284e39b3d8ae66a5c59559822

Timo, I've committed your patch. I made some modifications because of how I
wanted to deal with folders going forward, and this seemed like an appropriate
time to deal with that.

There's still the issue of showing the special folders' names in native format
(a new ticket, #5443) and dealing with folder name encodings (#5217).

Thanks for all the hard work on this!

####

#31

Updated by Charles Lindsay 7 months ago

  * **Status** changed from _5_ to _Fixed_



--- Bug imported by chaz@yorba.org 2013-11-21 20:25 UTC  ---

This bug was previously known as _bug_ 3704 at http://redmine.yorba.org/show_bug.cgi?id=3704

Unknown milestone "unknown in product geary. 
   Setting to default milestone for this product, "---".
Setting qa contact to the default for this product.
   This bug either had no qa contact or an invalid one.