Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

IMAP Keywords (rebased) #558

Closed
wants to merge 25 commits into from
Closed

Conversation

igoralmeida
Copy link

This is a rebase of #146 on top of current master, also adding the ability to list the tags in the message view.
I have no intention of merging this as-is, but hopefully this is a good starting point to discuss the functionality.

For example: the actual listing should use a mapping so that the user sees tags, not the IMAP keywords themselves ($label1, $label2, some_keyword, ...), but I don't have enough android experience to do this yet.

Please comment.

Sander Bogaert and others added 12 commits February 18, 2015 17:46
…x to allow all flags to be retrieved by valueOfByRealName() just in case.
Changes:
* Current codebase has a new "FORWARDED" flag
* Since Flag is no longer an enum, use another container (HashSet
instead of EnumSet)
* Direct (dumb) translation of ImapStore.search(). Don't quite like it
right now.
* Remove MessagingController.SYNC_FLAGS so that all flags are sync'd
(not entirely sure this works the way I think it does, though).
Also, fix wrong translation of switch statement (must not break when
encountering default flags).
Adds a label+field after "Cc:" listing all user-defined imap keywords in
the message.
public static Flag valueOf(String internalName) throws IllegalArgumentException {
try {
// the argument being 'null' implies we look for a static field
return (Flag)(Flag.class.getField(internalName).get(null));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than reflection here, I would extract the internal flags listed in valueOfByRealName into a method returning a set of Strings and see if it contains internalName.

You can ensure the new method is kept in sync with the actual fields in a unit test.The unit test can use reflection to enumerate Flag's fields without concern about performance.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like k9mail/src/androidTest/java/com/fsck/k9/mailstore/LocalMessageTest.java ?
How would it account for new flags showing up during the imap session, though?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@igoralmeida
Copy link
Author

Guys, thanks for the feedback.

Could you also give me some pointers regarding the tag<->keyword mapping I mentioned? This is how alpine does, for example.
I'm guessing K-9 would have to query some sort of local database to find what tag to show when Flag.mName=='$label1'. How should this work here?

@jberkel
Copy link
Contributor

jberkel commented Mar 3, 2015

another important thing (IMHO) is naming. this PR mentions flag, keyword, label, tag, etc... it's quite confusing. is there a good way to make the concepts more obvious?

@igoralmeida
Copy link
Author

Here's my take:

"flags" are hard-coded IMAP message attributes (SEEN, ANSWERED, RECENT, etc.)

"[user-defined] keywords" live on the IMAP server as well, but can be created by the IMAP client

"tags" are user-facing text that relate to IMAP keywords. I'm thinking a dictionary-like mapping {'$label1':'TODO', 'some_keyw':'Some Keyword'}

"labels" should be avoided here due to Gmail's implementation and UI, which uses IMAP folders instead of keywords. Also, Thunderbird will unfortunately use '$label1...5' for some keywords, but I believe that's understandable given the context.

Thus, the UI should use Tags consistently, while showing the term keyword only on some account configuration activity that maps Keyword to Tag + (possibly) Color.

@kbabioch
Copy link

kbabioch commented Mar 5, 2015

One of the last features that I miss personally. A feature like this is mostly about the UI and how to present tags to the user in a useful way. Does anyone know of mobile mail clients that already support this? I like the way Thunderbird handles this (color and GUI "labels"), but I'm not sure whether this is applicable on mobile devices.

@igoralmeida
Copy link
Author

Latest push: merged @artbristol 's consistency/test code + answer comments here.
I'm still 'losing' keywords, i.e. sometimes the Tags field won't show up because the USER_* keywords get lost from LocalMessage.mFlags. This happens somewhere between MessagingController.syncFlags() and MessageHeader.populate().

@igoralmeida
Copy link
Author

I could not reproduce the problem fully, but I'm almost sure it was due to removing some X_.... flags. If I understand correctly, those are supposed to be local to k-9 only.

Anyway, I believe it's fixed now. Whenever messages have user-defined keywords, they will show up in the header in the message view activity.

@igoralmeida
Copy link
Author

Some ramblings...

I've been looking into the keyword->tag mapping functionality and how things would look like in StoreSchemaDefinition and the fetching code.
Initially, I was considering adding three more tables (imap_keywords, ui_tags and keyword_tag_map), so that any keyword could be mapped to any tag, consistently. However, ensuring the mappings were consistent in the DB would require changing the messages.flags column as well, so I put that aside.

The next idea would be a single keyword_and_tag table, "(keyword TEXT UNIQUE, tag TEXT)", later possibly with a 'color' field too. In this case, I'd add the USER_xxxx keywords in lowercase as they came up during sync, and eventually the user could edit the mappings in a new activity.

Regardless of the design, messages.flags being a multivalued field makes it a bit cumbersome to simply extend the query and have the tags magically appear, so I'd probably have to query keyword_and_tag. So what am I looking for, something inside EmailProvider?

And, finally, another approach that occurred to me was adding the tags to LocalMessage directly, probably in LocalMessage.populateFromGetMessageCursor. In this case, I think I need to create some kind of "TagStore" similar to LocalStore so I can discover the tags as the keywords appear.

If anyone can explain the current architecture and/or comment on the approaches above, I'd be happy to read.

This maps name->Flag, to avoid duplication of objects, and adds a "tag
name" attribute to the Flag class. The UI now uses this field instead of
the name.

The memory mapping is updated at every call to
MessagingController.checkMailForAccount(). Probably not the right place.
This creates a very simple activity (shamelessly copying from FolderList) to
present a listing of imap keywords and their corresponding tags from the memory
map. Read-only, for now.

To access the activity, click the Tags label (or any of the items) in the
message header.

Also, change Flag.REMEMBERED_KEYWORDS to LinkedHashMap to maintain
iteration order.
@igoralmeida
Copy link
Author

This "works" (read-only). Kind of ugly. Contributions, especially to the UI, are very welcome.

@igoralmeida
Copy link
Author

How it works: from the message header, tap "Tags" (or tap any of them):
message_header

The mapping activity shows up:
tagmapping

@cketti cketti mentioned this pull request Jun 10, 2015
User can edit the tag names for imap keywords that k-9 picks up when
reading/syncing local messages.

Usability still sucks and the UI has too many quirks (lost focus, cursor
not updated, saving to memory only when leaving the activity...)
Tag names survive resets. Still barely usable: need somebody with actual android UI skills.
@igoralmeida
Copy link
Author

I think adventurous people can start poking now, despite the annoying usability problems:

  1. Click the "Tags" label or any of the listed tags in the message header (if the message happens to have one) to open the mapping activity.
  2. Click an item in the mapping activity to be able to edit it. Mind the cursor focus (I have no f'ing idea how this works).
    ... then click some other item to commit the change, because the back button will cancel it. Use a blank tagname to hide the tag.
  3. Pressing back after editing the items will take you back to the message view, but the tags won't update right away. Reopen the message to see the new tagname (need a new hook here?).
  4. When opening a message (say, after killing k-9 and restarting) you might see old tagnames instead of the ones you committed. Clicking "check mail" should fix it. That happens because the in-memory mapping is populated (from the keywords themselves) before the database can overwrite the defaults assumed by the UI.

I'm trying to get the most basic functionality working (reading keywords from the server and customizing the tags locally) first. I'd love ideas and patches to fix the most pressing usability problems, but I'm much more interested in making the internals good enough for a merge, possibly with a big "EXPERIMENTAL" sign next to some preference to enable the UI bits.

@PanderMusubi
Copy link

Coming from Thunderbird, I would say you need:

  • tag value (in the message)
  • mapping from tag value to a tag label (display in gui)
  • tag ordering (unique integer per value-label mapping to unambiguously sort messages on tags) (in Thunderbird, this is also they shortkey to toggle a tag on message(s) and is mapped to keyboad 1, 2, .., 9, 0)
  • tag color (font color for tag label)

@equaeghe
Copy link

... in Thunderbird, this is ...

Please take care to not make the implementation so Thunderbird-specific as to conflict with other MUAs' interfaces.

Also, there are a number of registered keywords whose treatment should be special-cased: http://www.iana.org/assignments/imap-keywords/imap-keywords.xhtml

(There are also a lot of non-registered $keywords used by various servers and clients; I'd think special-casing them would not be advised, as their use is not standardized.)

Furthermore: not all IMAP servers support arbitrary keywords (Exchange...), so giving people the option of setting them there must be disabled.

@igoralmeida
Copy link
Author

On 11/13/2015 08:04 AM, Erik Quaeghebeur wrote:

... in Thunderbird, this is ...

Please take care to not make the implementation so Thunderbird-specific
as to conflict with other MUAs' interfaces.

Also, there are a number of registered keywords whose treatment should
be special-cased:
http://www.iana.org/assignments/imap-keywords/imap-keywords.xhtml

(There are also a lot of non-registered $keywords used by various
servers and clients; I'd think special-casing them would not be advised,
as their use is not standardized.)

So far all keywords are equal, and the user decides what tag to assign
to each keyword.

Furthermore: not all IMAP servers support arbitrary keywords
(Exchange...), so giving people the option of setting them there must be
disabled.

Yes. Probably at SELECT time. However, for now, this is read-only and I
hope to get some help with the UI part so this can be merged. We can
then think about how to implement tagging in K-9.


Reply to this email directly or view it on GitHub
#558 (comment).

Igor Almeida

@PanderMusubi
Copy link

Would it be appreciated if we start a BountySource reward on this bug and bug 769?

@richardboegli
Copy link

richardboegli commented Jan 20, 2017

Would it be appreciated if we start a BountySource reward on this bug and bug 769?

@PanderMusubi Yes.
Would it get pushed up into Kaiten though? I use Kaiten because I need rich text composing.

@PanderMusubi
Copy link

I noticed that bounties are not often collected even though there are a lot out there, see https://www.bountysource.com/teams/k9mail

By the way, since this issue is closed, I cannot add to a bounty on this issue...

@philipwhiuk
Copy link
Contributor

Closing - needs further work, rebase and discussion. See #2260

@Trogel Trogel mentioned this pull request Aug 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants