GNOME Bugzilla – Bug 767821
Recognize X-GM-EXT-1 as SPECIAL-USE for Gmail IMAP
Last modified: 2016-11-09 13:11:31 UTC
I'm using camel to connect to Gmail. This bug report concerns RFC 6154. First problem, since Gmail's implementation of the standard is broken, they do not advertise the SPECIAL-USE capability. The workaround this is to check is to check for the X-GM-EXT-1 capability, which is Gmail's umbrella capability for all their weird quirks and features. I will be proposing a patch for this soon. With this solved, in the camel_imapx_list_response_add_attribute, it seems to be able to pick up the SPECIAL-USE attributes from the known_attributes array just fine. The lines below show the behavior: [imapx:A] Starting command ( literal) A00011 LIST "" "[Gmail]/Sent Mail*" RETURN (CHILDREN SUBSCRIBED STATUS (MESSAGES UNSEEN UIDVALIDITY UIDNEXT HIGHESTMODSEQ) SPECIAL-USE) Attribute found: \HasNoChildren Attribute found: \Sent <------ this is the SPECIAL-USE attribute Attribute found: \Subscribed The problem seems to be that the flags field of _CamelFolderInfo does not contain this information. So, somewhere between these two, the information is lost. I have tried to understand this process and pin-point what is happening, but I can't seem to wrap my head around it :( I'll gladly work on fixing this issue if I get some pointers as to where I should be looking.
Managed to pinpoint the issue to the implementation of imapx_store_mailbox_attributes_to_flags, which does not check the special use attributes.
I have fixed the issue and will propose a patch. However, from what I can tell, my current solution will require a store refresh to be triggered when this change is installed, as some of the enum bit fields are not compatible anymore (specifically, one of the enum was doing something funny, and I think was non-GObject compliant).
Thanks for a bug report. The evolution itself determines whether certain folder is or is not a Sent folder (you did not mention other folders) by the folder's usage as Sent folder in the account settings (Defaults tab). If none account uses that folder it is not shown as Sent folder in the folder tree (with a special icon). I do not see what version of the evolution-data-server or evolution you use, but consider the changes from bug #725320. I tend to close this as a duplicate of it.
Hello Milan, Yes, eds does have functionality to read SPECIAL-USE mailbox attributes, which is what I tried to show above (I did not include all the folders, although the problem applies to all of them). However, after reading and parsing the SPECIAL-USE attributes, it does not store the the flags anywhere (or at least I cannot seem to be able to access them from the application side). Further, the SPECIAL-USE enums seem to be non-GObject compliant, as they do some sort of bit-packing, which I tried to fix. There is also the issue that SPECIAL-USE doesn't get activated in the case of Gmail, as Gmail does not advertise the capability as "SPECIAL-USE", but as "X-GM-EXT-1". Please have a look at my github commit, I have included annotations explaining why everything is needed: https://github.com/matzipan/evolution-data-server/commit/1d940628172a8661b3a277687ec85ec501dfd193
Okay, I still do not follow what you are exactly trying to achieve and from where. The bug I pointed to adds new CamelStore virtual functions which the respective mail providers can use to advertise these special folders. The IMAPx does that. I do not think the folder types should be stored in the store summary, it's an online state of the folder, not an offline. I also do not agree with those deletions in your github commit, because the folders cannot be Normal, Inbox, Outbox, Trash, Junk,... at once. The part to add the X-GM-EXT-1 looks fine. What is the \Important attribute used for, please?
> Okay, I still do not follow what you are exactly trying to achieve and from where. I am trying write a mail application using EDS. When using the CamelFolderInfoFlags enum to check for the folder types (inbox, normal, junk, outbox, archive, etc), they were simply not getting set by Camel. That was because: 1. Camel didn't know how to interpret X-GM-EXT-1 as SPECIAL-USE. 2. With that fixed, they were still not getting retrieved on the client side. I have made it work by writing the attributes in imapx_store_mailbox_attributes_to_flags. It might not be the best solution, which is why I asked for your opinion about what I should do with this fix to merge it. > The bug I pointed to adds new CamelStore virtual functions which the respective mail providers can use to advertise these special folders. The IMAPx does that. The only changes I could find about what you said are [1]. However, this still seemed to have no affect over the FolderInfoFlags. > I do not think the folder types should be stored in the store summary, it's an online state of the folder, not an offline. I am unsure what offline and online state is in terms of Camel, the documentation was not clear about that. Also, if folder types should not be stored in the store summary, where should they be stored? > I also do not agree with those deletions in your github commit, because the folders cannot be Normal, Inbox, Outbox, Trash, Junk,... at once. I agree, but I suspect this is not GObject compliant. On top of this, there seems to be no way to get this behavior to work in Vala. Not to mention that a client application might get false positives with flags which have overlapping bits. I'd personally prefer extra logic to check for the exclusivity of flags when setting, in Camel, since you have control over it, than have the client user apps get weird results because they are not using the library properly. The Gmail IMAP documentation [2] mentions important \Important as "an additional attribute added for Gmail's Priority Inbox". [1] https://git.gnome.org/browse/evolution-data-server/diff/camel/providers/imapx/camel-imapx-store.c?id=1be0d6f [2] https://developers.google.com/gmail/imap_extensions#special-use_extension_of_the_list_command
I just realized I might have been mistaken, yes, there is a way to get that behavior in Vala. I might have to update the introspection so the vapi picks up the type mask. I also realise I was mistaken about the false positives, it is just the way I was checking was wrong, since I couldn't see the type mask from Vala.
Scratch that, the vapi was fine. I will undo the deletion you mentioned about the types.
No problem, I do not know Vala, thus I cannot help there. The link [1] from comment #6 points to the place I was aiming at. The idea behind it is that you call camel_store_initial_setup_sync(), eventually the asynchronous variant, and get the GHashTable of the save_setup. The 'key' part of the hash table is either one of the predefined CAMEL_STORE_SETUP_ARCHIVE_FOLDER, CAMEL_STORE_SETUP_DRAFTS_FOLDER, CAMEL_STORE_SETUP_SENT_FOLDER, CAMEL_STORE_SETUP_TEMPLATES_FOLDER or a custom name of the option to change (see the link you pointed to for the list of known keys for the IMAPx). The 'value' part of the hash table is the value to write. The documentation describes what is expected for respective keys. The thing is that the server can claim that certain folder is used for certain purpose, but it doesn't mean that the user has setup the account to actually have that folder used as that. For example, it would be confusing to show a user particular folder as Draft (with a special icon in the folder tree), when the folder is not used for Draft messages, thus "Save as Draft" won't add a message into that folder. What might help you a bit (still consider the previous paragraph, though) is to add a guint32 (or an enum) folder_type variable into the CamelStoreInfo, which would serve as "what the server advertises about the folder", but it won't mean that the folder is truly used as that.
I forgot to add: also note of bug #764065, where the API/ABI change can be added together with it.
I will try and digest your message when I get home later this evening, and see where I can get. I will also get in touch with Corentin and make a pull request to his repo.
(In reply to Andrei Zisu from comment #11) > I will also get in touch with Corentin and make a pull request to his repo. Just a note, pull requests do not work, attach patches here, please.
> The thing is that the server can claim that certain folder is used for certain > purpose, but it doesn't mean that the user has setup the account to actually > have that folder used as that. For example, it would be confusing to show a > user particular folder as Draft (with a special icon in the folder tree), when > the folder is not used for Draft messages, thus "Save as Draft" won't add a > message into that folder. Right, I think I have a better understanding of what you mean now. But, is this use-case all that common? I cannot find a justification for it.
Reading through the documentation at [1], it says the following things about "CAMEL_FOLDER_TYPE_": > Then there are additional bits which describe the basic folder type, in addition > to the options above. They are just used to provide appropriate icons currently, > although they could be used to help auto-configure things like Drafts and Sent > folders with some more work. From what I can tell, if your application does not intend to allow for configurable folders, then using these flags directly from the store is alright. [1] https://wiki.gnome.org/Apps/Evolution/Camel.Store
Created attachment 330442 [details] [review] Add check for the Gmail's IMAP extension capability
Created attachment 330443 [details] [review] Add attribute from the Gmail's own IMAP extension
Created attachment 330444 [details] [review] Add missing folder type flags and clean up
I have attached 3 patches. Waiting for your feedback. Please also advise on whether this should go on Corentin's breaking-changes branch.
Review of attachment 330442 [details] [review]: ::: camel/providers/imapx/camel-imapx-utils.c @@ +467,3 @@ { "NOTIFY", IMAPX_CAPABILITY_NOTIFY }, + { "SPECIAL-USE", IMAPX_CAPABILITY_SPECIAL_USE }, + { "X-GM-EXT-1", IMAPX_CAPABILITY_GMAIL } Please follow the name of the extension in the enum, thus IMAPX_CAPABILITY_X_GM_EXT_1
Review of attachment 330443 [details] [review]: ::: camel/providers/imapx/camel-imapx-list-response.h @@ +64,3 @@ #define CAMEL_IMAPX_LIST_ATTR_TRASH "\\Trash" +/* Gmail extension "SPECIAL-USE" Flag */ GMail ... flag
Review of attachment 330444 [details] [review]: ::: camel/camel-enums.h @@ +51,3 @@ } CamelFolderFlags; +#define CAMEL_FOLDER_TYPE_BIT (11) If the change is really necessary, then add there some gap between the last flag and the first type bit. For future expansion. I'd use bit 16. @@ +128,3 @@ CAMEL_FOLDER_SHARED_TO_ME = 1 << 8, CAMEL_FOLDER_SHARED_BY_ME = 1 << 9, + CAMEL_FOLDER_READONLY = 1 << 10, When you are at it, could you add also CAMEL_FOLDER_WRITEONLY, please? @@ +143,3 @@ + CAMEL_FOLDER_TYPE_ARCHIVE = 12 << CAMEL_FOLDER_TYPE_BIT, + CAMEL_FOLDER_TYPE_DRAFTS = 13 << CAMEL_FOLDER_TYPE_BIT, + CAMEL_FOLDER_TYPE_IMPORTANT = 14 << CAMEL_FOLDER_TYPE_BIT, The FLAGGED and the IMPORTANT looks like the same thing for me, thus I'd avoid the IMPORTANT, as it's a special thing for the GMail. @@ +148,3 @@ } CamelFolderInfoFlags; +#define CAMEL_FOLDER_TYPE_MASK (0xF << CAMEL_FOLDER_TYPE_BIT) You are masking 14 bits by 4 bits only. ::: camel/providers/imapx/camel-imapx-store.c @@ +2468,2 @@ static gboolean + this new line shouldn't be there
When updating the patches, please merge them into one. I do not like this splitting, also because clicking 3-times, when I can click only once.
(In reply to Andrei Zisu from comment #13) > Right, I think I have a better understanding of what you mean now. But, is > this use-case all that common? I cannot find a justification for it. I described how the Evolution uses the flags. If your application will use it differently then it's up to you. (In reply to Andrei Zisu from comment #18) > Please also advise on whether this should go on Corentin's breaking-changes > branch. I do not know, I guess he'll rebase his branch once he gets time to work on it again. I added him into the CC, thus he's aware.
> When updating the patches, please merge them into one. I do not like this splitting, also because clicking 3-times, when I can click only once. Apologies, I thought you might find them easier to follow this way. > The FLAGGED and the IMPORTANT looks like the same thing for me, thus I'd avoid the IMPORTANT, as it's a special thing for the GMail. They are not the same thing. Flagging is a user-triggered action, whereas marking a message as important is usually done by a machine learning algorithm when it is recieved (a newsletter you routinely ignore is not marked as important, whereas a mail from your coworker is). > You are masking 14 bits by 4 bits only. I am masking the number 14 by ceil(log2(14)), which is 4. I will integrate the rest of the changes later today.
> I am masking the number 14 by ceil(log2(14)), which is 4. Actually there are 15 different types (1-14 + 0), but same masking value.
(In reply to Andrei Zisu from comment #24) > Apologies, I thought you might find them easier to follow this way. No problem, thanks. > They are not the same thing. Flagging is a user-triggered action, whereas > marking a message as important is usually done by a machine learning > algorithm when it is recieved (a newsletter you routinely ignore is not > marked as important, whereas a mail from your coworker is). The RFC 6154 says: \Flagged This mailbox presents all messages marked in some way as "important". When this special use is supported, it is likely to represent a virtual mailbox collecting messages (from other mailboxes) that are marked with the "\Flagged" message flag. Which is quite close for the Gmail's \Important flag. I'd rather skip it then, as it's a specific thing of the Gmail. > I am masking the number 14 by ceil(log2(14)), which is 4. Eeks, of course, my fault, I'm sorry. I misplaced "shifting number 14" with "shifting by 14 bits". The 0xF mask is fine there, as you said.
(In reply to Milan Crha from comment #20) > Review of attachment 330443 [details] [review] [review]: > > ::: camel/providers/imapx/camel-imapx-list-response.h > @@ +64,3 @@ > #define CAMEL_IMAPX_LIST_ATTR_TRASH "\\Trash" > > +/* Gmail extension "SPECIAL-USE" Flag */ > > GMail ... flag Disregard the "Gmail ~> GMail" part, I see they changed the way they write the 'Gmail' word. I know it from times when it used to be with capital 'M', or I was just used to that way of writing that "word".
> Which is quite close for the Gmail's \Important flag. I'd rather skip it then, as > it's a specific thing of the Gmail. It's true that it clashes with the RFC, but in terms of usage, it's pretty different. It does not add any value to have this as a separate folder anyway, so I agree with you and will remove it. It is, however, pretty important for messages and the order they are displayed in (I have not reached that part yet).
Created attachment 330599 [details] [review] Gmail patch
Review of attachment 330599 [details] [review]: Hmm, the change requires migration of the currently stored data into the new format in the providers (and soname version bump, which is the least thing). The built-in providers are located in eds/camel/providers/, but there are also two major extensions, evolution-ews and evolution-mapi, which should be updated as well. Thus either we provide the migration code too, or we make this in a backward-compatible way, which means adding CAMEL_FOLDER_WRITEONLY as a 1 << 17 (filling the empty gap) and adding new types, without changing the shift bits and other parts. We can rename CAMEL_FOLDER_FLAGGED, the change will be covered by the soname version bump (and it is currently also unused in the evolution/evolution-data-server code). I'm sorry I didn't think of the migration earlier. Otherwise the patch looks fine, with the notes below. ::: camel/camel-enums.h @@ +51,3 @@ } CamelFolderFlags; +#define CAMEL_FOLDER_TYPE_BIT (16) Just thinking of it, the bit structure of this enum after this patch is: ???? ???? GGGG TTTT FFFF FFFF FFFF FFFF 31 15 0 Where the 'F' is for flags, with occupied the first 12 bits and 3 bits for future expansion; the 'T' is for the type and the 'G' is a gap between the type and the last bit. There used to be reserved 6 bits for the type, instead of the proposed 4 bits, which I think can be left as that - again, for the future. The '?' is for unused bits, possibly left for the Camel providers. @@ +129,3 @@ CAMEL_FOLDER_SHARED_BY_ME = 1 << 9, + CAMEL_FOLDER_READONLY = 1 << 10, + CAMEL_FOLDER_WRITEONLY = 1 << 11, there is missing devel-doc comment for this ::: camel/providers/imapx/camel-imapx-store.c @@ +424,3 @@ + + if ((store_info_flags & CAMEL_STORE_INFO_FOLDER_TYPE_MASK) == 0) { + store_info_flags |= CAMEL_STORE_INFO_FOLDER_TYPE_NORMAL; this is not needed, it is a void operation, because CAMEL_STORE_INFO_FOLDER_TYPE_NORMAL evaluates to 0.
> Thus either we provide the migration code too, or we make this in a backward- > compatible way I'll have a look and see what I can do. > this is not needed, it is a void operation, because > CAMEL_STORE_INFO_FOLDER_TYPE_NORMAL evaluates to 0. As a rule, I prefer to not make any assumptions about the values something might take, as that's the whole point of making it a constant. I can remove it, if you insist, but I'd rather not.
(In reply to Andrei Zisu from comment #31) > > Thus either we provide the migration code too, or we make this in a backward- > > compatible way > > I'll have a look and see what I can do. I'm afraid that writing the migration code is too tricky and requires a change in each provider, which is too much work just for these things. Safer bet is to make things backward compatible, for which there are plenty of room (free bits). > > this is not needed, it is a void operation, because > > CAMEL_STORE_INFO_FOLDER_TYPE_NORMAL evaluates to 0. > > As a rule, I prefer to not make any assumptions about the values something > might take, as that's the whole point of making it a constant. I can remove > it, if you insist, but I'd rather not. Please remove it, the "normal" type is just the default, aka if it's nothing else. It's like "none" constants in this regard.
Created attachment 332051 [details] [review] Fixes final comments
Review of attachment 332051 [details] [review]: Thanks for the update. Please see my inline comments. ::: camel/camel-enums.h @@ +51,3 @@ } CamelFolderFlags; +#define CAMEL_FOLDER_TYPE_BIT (19) If you change this, then it's not backward compatible and requires migration code and soname version bump. @@ +101,3 @@ * The folder contains tasks, instead of mail messages. + * @CAMEL_FOLDER_FLAGGED: + * This folder contains flagged messages. Some clients call this "starred. Used by RFC 6154. Missing closing double quote around the 'starred' word. @@ +148,3 @@ } CamelFolderInfoFlags; +#define CAMEL_FOLDER_TYPE_MASK (0xF << CAMEL_FOLDER_TYPE_BIT) Similarly here, it also requires migration code.
Just a note that the changes for bug #764065 are almost ready, I target them for 3.23.2. Reading the backlog of this bug report I think we've got into the state that will avoid the API changes, thus it's rather unrelated. I'll update your patch to not do the API changes and commit it some time around the changes for bug #764065.
As promised, I made the changes and pushed this to master. Created commit d5d7b43 in eds master (3.23.2+)
Hello Milan, Sorry for leaving it like that. Thank you.
(In reply to Andrei Zisu from comment #37) > Sorry for leaving it like that. Thank you. Nah, no problem, those were only simple changes. I see one side-effect, which is not great. I do not have set my [Gmail]/... subfolders any specifically, but they have the icons of Trash, Junk and Drafts folders in the evolution's folder tree. I mentioned that above too. That's not good. Did you want to use those flags specifically for the CamelStore's flags, or for the folder too?
So you're saying you now have two Trash icons? This is how they are used: https://github.com/matzipan/envoyer/blob/master/src/Models/Folder.vala#L13
Yes, evolution's folder tree shows two trashes and two junk folders, because I do not have set real trash/junk folders in the Google Mail account properties. Hrm, okay, so from the folder info structure. That's bad. Could you read it from the store info instead? That what you read is "what the server thinks of this", but not "what the Camel/evolution uses".
I suppose that makes sense, when I wrote this code I wasn't fully aware of the distinction. Does this mean Evolution needs fixing, or that the patch needs to be reverted?
I do not want to revert the patch. We should find some way to express the distinction between "what server advertises" and "what is used". What exactly it'll be I do not know yet.
I might be missing something from the picture (my first mailbox was Yahoo and then Gmail), but when is this really a problem? What is the motivation behind using something else other than what the server advertises?
Call it freedom. Some users prefer virtual trash/junk folders, some prefer real trash/junk. In case of Gmail, you really do not want to save sent messages into Gmail's Sent folder, that way you'll have there the message twice. From the evolution point of view, the UI highlights (shows a folder with a different icon) folders which have special meaning and which are used specifically by the evolution, but right now it's not the case. You can replace 'evolution' with 'Camel library', if you want to.
I made the folder type flags to be ignored, when copying flags from the store's summary, thus the server advertised folder types do not influence the UI. Your change is still useful for the initial setup, where the folders are properly detected and selected. Created commit 42e0124 in eds master (3.23.2+)