IMAP Keywords #2668

Trogel · 2017-08-07T19:49:03Z

This is my attempt at issue #769. It started as a rebase of pull request #558. Many thanks to @igoralmeida and all other contributors! However, to avoid surprises: I assume I have turned almost everything upside down.

As of now I'd call this a working prototype. So I don't expect this go into k-9 without further changes. However, now would be a good time for me to get some feedback and guidance for further development.

Implemented features

Message list: background shading using color of first tag ("first" in terms of the order defined in the keyword editor)
Message view: list of colored tag names; tap for tag choice dialog
Tag choice dialog: de-/select tags for message; changes are written to the IMAP sever; tap preferences icon for keyword editor
Keyword editor: change visibility, display name, color, order; add/remove keyword

New keywords found on emails are automatically added to the database, but by default they are hidden. To make them visible open an email, tap the tag list, tap the preferences icon, tap the visibility icon, and then tap the back button.

Some more things I'd like to add

Make shading in the message list optional.
Make tag names in the message view optinal.
Add optional set of colored tag icons to message list and message view left of the star icon. A tap on any of these icons should also open the tag choice dialog.
Commands to add default keywords of other popular email clients.
Single tag mode for tag choice dialog, so that a single tap already closes the dialog.
Add more commands to context menu in keyword editor, such as hide/show, edit name, etc.
Add "Edit tags" to context menu in message view.
When editing a keyword name, disable the Ok button while the name is empty.

Well, the list is getting long. It probably makes more sense to focus just on the fundamental functionality for now and leave additional fancy features for further pull requests.

Major design changes compared to #558

I tried to align class names and such with the following terminology (see also the comment in Flag.java):
- A "flag" is either used only internally by k-9, or it is a server flag that additionally has an external code used for representation in external storage.
- A "server flag" is either an (IMAP) system flag (whose external code starts with a backslash "\") or an (IMAP) keyword (whose external code does not start with a backslash "\").
- A "keyword" additionally has properties such as a name for display, a color, a visibility flag, and a position in the ordered list of keywords.
- A "tag" is a keyword assigned to a message.
Keyword support has been moved from the Flag class into a derived class named Keyword.
The keyword catalog is now stored in the global preferences database. The old implementation also had just one catalog in memory, but stored it in the account-specific databases.

Open issues

Now for the open issues, where I am hoping for feedback and possibly some guidance:

I roughly tried to follow the coding guidelines. Any feedback is welcome!
Is the UI design okay? What should be changed to be bettern in line with k-9?
Did I possibly break compatibility with old Android versions? Are there special things I should look for? Special checks to perform?
Color shading calculation is currently implemented in the Keyword class. This way it pulls com.android.support:support-v4 into k9mail-library. Is that okay, or should that better go into k9mail (non-library)? Or would is be even preferred to leave out that support lib at all and do the calculation "manually"?
The keyword editor uses @drawable/design_ic_visibility and @drawable/design_ic_visibility_off to indicate visibility of the keywords. This pulls in com.android.support:design. It that okay?
The keyword editor currently uses somewhat awkward up/down buttons for ordering. A list layout with drag support would be nice! But before investing additional time, I'd like to ask if this is considered useful, and if there is a preferred way to go. I briefly looked at the following implementations:
https://github.com/JayH5/drag-sort-listview
https://github.com/justasm/DragLinearLayout
The up/down buttons currently misuse k-9's collapse/expand icons (iconActionCollapse, iconActionExpand). Can this remain until they are replaced with a drag list layout?
I wonder whether the in-memory storage and the on-disk storage of the keyword catalog are already good enough in terms of thread-safety. The former is implemented in the class Keyword, the latter in the class Preferences. The catalog is changed by the keyword editor on one hand, and by the IMAP store (store/imap/ImapFolder.java) on the other hand. So I fear that there is a potential for race conditions.

…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.

per artbristol's recommendation in issue 558

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.

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.

Features: * Message list: background shading using color of first tag * Message view: list of colored tag names; tap for tag choice dialog * Tag choice dialog: de-/select tags for message; changes are written to the IMAP sever; tap preferences icon for keyword editor * Keyword editor: change visibility, display name, color, order; add/remove keyword keywords: Drop TagDialog. keywords: Add new tag choice dialog. keywords: Add new keyword editor activity. keywords: Factor class Keyword out of Flag. keywords: Rename 'tag' to 'keyword' where is it not attached to a mail. keywords: Move keyword storage from account-specific LocalStore to k9-global Preferences. keywords: Move background color shading into Keyword and make it also work for the dark theme. keywords: Pick a random color for a new Keyword. keywords: Save newly created Keywords to preferences. keywords: Factor out parsing of comma-separated list of Flag codes. keywords: Drop tags label from MessageHeader. keywords: Separate font size of tags from cc in MessageHeader. keywords: Clean up MessagingController.syncFlags().

Trogel · 2017-08-07T20:10:06Z

Some screenshots:

And, well, I'll soon take a deeper look at Travis' log.

… query.

igoralmeida · 2017-08-08T19:25:21Z

Thanks a lot for carrying the torch! I used to test this with a separate Application ID alongside my 'official' k-9 install, I'll see if I can dig up the patch I used for that and take a look at what's new.

I'd just like to comment on (6), as a user.

The keyword editor currently uses somewhat awkward up/down buttons for ordering. A list layout with drag support would be nice! But before investing additional time, I'd like to ask if this is considered useful, and if there is a preferred way to go. I briefly looked at the following implementations:
https://github.com/JayH5/drag-sort-listview
https://github.com/justasm/DragLinearLayout

I like AntennaPod's drag-and-drop reordering in the Queue activity, which is only triggered when you use the :: part on the left. However, this is something I would do so rarely that tapping arrows might be acceptable (and, actually, carry a less "temporary" feel to it).

philipwhiuk · 2017-08-08T23:58:28Z

The empahsis has been on reducing k9mail-library's dependency on Android. So put the functionality in k9, possibly by implementing an interface exposed by the library.

philipwhiuk · 2017-08-11T03:10:43Z

k9mail-library/src/main/java/com/fsck/k9/mail/Flag.java

 /**
 * Flags that can be applied to Messages.
+ *
+ * Terminology:


I'd rather see this go on the website's developer documentation than in the source, but I'm a clean-code-ist.

Fine with me! For example, I could add a basic "Flags" or "Flags and Keywords" page to the Development section on https://k9mail.github.io/ or to the Wiki. What do you prefer?

If you could make a PR for the website we can merge that in at the same time we do a release :)

Here we go: See #55 for k9mail.github.io.

philipwhiuk · 2017-08-11T03:12:44Z

k9mail-library/src/main/java/com/fsck/k9/mail/Flag.java

+
+    // Code for use in external storage, e.g. "\\Deleted"
+    // For IMAP: A 'system flag' or a 'keyword' in terms of RFC 3501, 2.3.2.
+    private final String externalCode;


We should think about WebDAV and EWS/EAS here a bit. Ideally we want the same flags in multiple stores.

philipwhiuk · 2017-08-11T03:19:33Z

k9mail-library/src/main/java/com/fsck/k9/mail/Keyword.java

+    }
+
+
+    public static Keyword getKeywordByExternalCode(String externalCode)


I'm not a big fan of static methods where you're storing and loading data, they are hard to test. I'd rather these move to a FlagsManager class which is an application singleton.

I have now created a FlagManager that manages lookup of flags and keywords, as well as dynamic creation, deletion, and order of keywords. I left the creation (and the HashMaps) of flags in the class Flag, since the flags are all hard-coded, and I did not want to make their constructor public. There are still lookup methods in Flag, but they no longer access Keyword.

On the downside, the constructor of Keyword and some other methods are now public, so that FlagManager can access them. I could have made the manager a nested class of Keyword, but since it manages not just keywords, but all kinds of flags, that appeared to be wrong. Also, some do-it-yourself friend construct seems to be over-the-top.

Oh, and I slightly preferred FlagManager over FlagsManager, but will happily change it, if you like. :-)

philipwhiuk · 2017-08-11T03:20:29Z

k9mail-library/src/main/java/com/fsck/k9/mail/store/imap/ImapFolder.java

-            } else if (flag == Flag.FORWARDED
-                    && (canCreateKeywords || store.getPermanentFlagsIndex().contains(Flag.FORWARDED))) {
-                flagNames.add("$Forwarded");
+            // client's can't add the RECENT flag!


philipwhiuk · 2017-08-11T03:24:35Z

k9mail/src/main/java/com/fsck/k9/activity/KeywordEditor.java

+        @Override
+        public View getView(
+            final int position, View convertView, ViewGroup parent)
+        {


This method is way too long. It's a method for a class in a class that and contains multiple anonymous classes.

I agree. Now I refactored it and added a holder class to avoid repetition of one-time tasks. Still, the nested class KeywordEditorListAdapter has ~200 lines, which is almost half of the whole file. If desired, I can move the nested class into its own file.

philipwhiuk · 2017-08-11T03:30:00Z

k9mail/src/main/java/com/fsck/k9/fragment/TagChoiceDialogFragment.java

+
+            Keyword tag = getItem(position);
+            TextView tv = (TextView) convertView.findViewById(
+                android.R.id.text1);


android.R.id.text1 Change this.

android.R.id.text1 comes from the built-in layout android.R.layout.simple_list_item_multiple_choice. So, to change the former (for example to something like keyword_name), I would need to add a local layout for the list item. I will happily change it! Could you please confirm that this approach is preferred over using the built-in layout?

philipwhiuk · 2017-08-11T03:30:16Z

k9mail/src/main/java/com/fsck/k9/fragment/TagChoiceDialogFragment.java

+            }
+
+            Keyword tag = getItem(position);
+            TextView tv = (TextView) convertView.findViewById(


tv Call this something more informative.

philipwhiuk · 2017-08-11T03:30:49Z

k9mail/src/main/java/com/fsck/k9/fragment/TagChoiceDialogFragment.java

+                tv.setTextColor(tag.getTextColor());
+            }
+
+            ListView lv = (ListView) parent;


Call this something more informative.

philipwhiuk · 2017-08-11T03:31:24Z

k9mail/src/main/java/com/fsck/k9/ui/messageview/MessageViewFragment.java

+                Keyword.getVisibleKeywords(mMessage.getFlags()));
+            fragment.show(getFragmentManager(), "tag_choice");
+        } catch (Exception e) {
+            e.printStackTrace();


Use logging instead of this.

philipwhiuk · 2017-08-11T03:31:59Z

k9mail/src/main/java/com/fsck/k9/view/MessageHeader.java

@@ -340,6 +356,32 @@ public void populate(final Message message, final Account account) {
        mForwardedIcon.setVisibility(message.isSet(Flag.FORWARDED) ? View.VISIBLE : View.GONE);
        mFlagged.setChecked(message.isSet(Flag.FLAGGED));

+        SpannableStringBuilder sb = new SpannableStringBuilder();
+        List<Keyword> tags = Keyword.getVisibleKeywords(msgFlags);
+        if (tags.size() != 0) {


Create a helper method for this code.

…m setBackgroundColor()

…support-v4.

Trogel · 2017-08-26T15:34:36Z

The empahsis has been on reducing k9mail-library's dependency on Android. So put the functionality in k9, possibly by implementing an interface exposed by the library.

Done with 5f89a65.

Trogel · 2017-08-26T17:04:53Z

Oh, I hope I did not make things unnecessarily complicated by merging the 'master' into the 'keywords'. I am still new to git and GitHub, and the big "Resolve Conflicts" button was quite tempting... If desired, I'll try to revert. :-/

@philipwhiuk Except for that, I think I have worked on almost all issues you raised. Thinking about keywords in WebDAV and EWS/EAS remains open. I hope I find some time in the next days. Any additional input on this issue is welcome. Please let me know, if I have missed anything else from your feedback!

In addition, I still want to add some options to the global settings to enable/disable tag-based shading in the message list and tag names in the message view. But probably not today.

Valodim · 2017-08-26T23:12:14Z

I rebased this on current master, see https://github.com/k9mail/k-9/tree/Trogel-keywords. I noticed halfway through that you might have wanted to do this yourself, sorry if I've been hasty here :)

Unfortunately 'invalid' currently includes keywords with a comma that are valid according to RFC 3501, 2.3.2, but that k-9 cannot handle.

Trogel · 2017-08-27T05:09:11Z

@Valodim Fine with me! I hope it is okay that I still add a few changes here. You might want to merge them into Trogel-keywords.

Except for my new commit eb25b31, the diff between Trogel-keywords and my branch is currently rather small. I just found two lines that you could drop from Trogel-keywords:

k9mail/src/main/java/com/fsck/k9/view/MessageHeader.java line 148
k9mail/src/main/res/values/strings.xml line 286

Valodim · 2017-08-27T11:10:23Z

It probably makes sense to keep focus on this PR. I'd suggest you just git reset --hard origin/Trogel-keywords and work on top of that (you can also git cherry-pick eb25b3139e13e7f6e4d137b912c61be1c7fce70e that last commit of yours then) :)

Trogel · 2017-08-27T12:20:50Z

@Valodim Did I get it right: Your idea is to, let's say, replace my merge with your rebase? So, the result would be as if I had rebased my branch and the PR. I'll happily do that, if desired, but I remember all these warnings like "Do not rebase commits that exist outside your repository" (from the Git Book) and "BadThingsWillHappen" (from k-9's GitGuide).

Could you please confirm that rebasing the PR is the right thing do now?

I already tried it in a local branch, and it seems I'd better git reset to upstream/Trogel-keywords rather than origin/Trogel-keywords, right? At least, my origin has no Trogel-keywords at the moment, but upstream has. ;-)

Supporting three modes: always, never, if tags are assigned (default).

Trogel · 2017-08-27T15:49:36Z

Options for tags added to global settings. Enjoy! :-)

cketti · 2017-08-27T21:33:37Z

@Trogel, it's nice to see that someone is working on this. Thank you.

I'm sorry for not chiming in earlier. After looking at the issues and previous attempts I feel like this is a feature that needs more consideration before attempting an implementation. The functionality spans a lot of areas and it's not obvious what the user experience should look like. After writing down where we want to go it should be easier to come up with the right abstractions for the code. Otherwise this will be just another instance where a feature is tightly coupled to code that shouldn't have to care about what the app does with the data.

I've added my thought on this feature to issue #769. I'm tempted to close this PR because I believe it'll be easier to start over than to refactor this.

Trogel · 2018-04-02T09:05:02Z

Closing the pull request as suggested by @cketti.

Sander Bogaert and others added 25 commits August 7, 2017 19:36

New Flag object instead of enum.

8eea618

IMAP keyword support.

0dc840c

Reworked the Flag class a bit to be more clear.

d3ccc1c

Requested fixes. Better variable naming mainly.

a8ddf59

Adding an 'equals()' to the new Flag class.

bb2ce59

Fix: IMAP clients can't add the RECENT flag.

b246b3c

More camelCase and ran astyle on Flag.java

0d14397

Fixing code style and unwanted static field.

630aedc

Fixed bug in equals() and added HashCode() also made it immutable. Fi…

765b209

…x to allow all flags to be retrieved by valueOfByRealName() just in case.

User keywords: remove uppercase in mailstore.LocalStore

3b21c59

Also, fix wrong translation of switch statement (must not break when encountering default flags).

User keywords: list them in the message view (the ugly way)

1e48e43

Adds a label+field after "Cc:" listing all user-defined imap keywords in the message.

ImapStore: Enclose if body in braces

c3877ea

Flag: final class to avoid equals/hashcode problems

dcd4011

per artbristol's recommendation in issue 558

Flag: consistent parameter documentation

cdc1cf0

example of using unit test for reflection instead of main code

28b05d9

simplify

0d76e50

Compare and synchronize only custom user keywords

4da0c2e

Proper flag/keyword synchronization

30d01d8

IMAP keywords: add the keyword->tag mapping table to the database

f4d6c4a

IMAP keywords: sync'ed with the local database

7d12ceb

Tag names survive resets. Still barely usable: need somebody with actual android UI skills.

keywords: Bug fix: Use code instead of external code of flag for IMAP…

db3175a

… query.

philipwhiuk suggested changes Aug 11, 2017

View reviewed changes

Trogel added 4 commits August 26, 2017 16:02

keywords: Extract MessageListAdapter.getFirstTag() from bindView().

180c522

keywords: Extract MessageListAdapter.blendBackgroundWithKeyword() fro…

fbc9bb2

…m setBackgroundColor()

keywords: Refactor TagChoiceDialogFragment.onCreateDialog().

aadb64f

keywords: Remove dependency of k9mail-library on com.android.support:…

5f89a65

…support-v4.

philipwhiuk requested review from cketti and rhari991 August 26, 2017 15:33

Trogel and others added 4 commits August 26, 2017 17:50

keywords: Make EditText single-line for external code and name

a4b74a1

keywords: Disallow empty keyword name

3f366b2

keywords: Disallow duplicate creation of keyword

2380bda

Merge branch 'master' into keywords

6fc3701

Trogel added 2 commits August 27, 2017 06:50

keywords: Catch exception and log warning on invalid server keyword

eb25b31

Unfortunately 'invalid' currently includes keywords with a comma that are valid according to RFC 3501, 2.3.2, but that k-9 cannot handle.

keywords: Make flagList final

abc4b8b

Trogel added 7 commits August 27, 2017 14:36

keywords: Add fontSizeMessageViewTags to GlobalSettings

6c7a452

keywords: Also shade background, if !useBackgroundAsUnreadIndicator()

0891fa0

keywords: Don't shade background of selected message

0c11574

keywords: Make background shading optional

6ca04d6

keywords: Make tag names in message view optional

0e7a44b

Supporting three modes: always, never, if tags are assigned (default).

keywords: Put tag name display into separate methods

6b5e650

keywords: Add "Select tags" to menu in message view

4cb4a63

Trogel mentioned this pull request Aug 29, 2017

Add support for IMAP labels #769

Open

Trogel closed this Apr 2, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

IMAP Keywords #2668

IMAP Keywords #2668

Trogel commented Aug 7, 2017

Trogel commented Aug 7, 2017

igoralmeida commented Aug 8, 2017

philipwhiuk commented Aug 8, 2017

philipwhiuk Aug 11, 2017

Trogel Aug 19, 2017

philipwhiuk Aug 20, 2017

Trogel Aug 26, 2017

philipwhiuk Aug 11, 2017

philipwhiuk Aug 11, 2017

Trogel Aug 26, 2017

philipwhiuk Aug 11, 2017

philipwhiuk Aug 11, 2017

Trogel Aug 20, 2017

philipwhiuk Aug 11, 2017

Trogel Aug 19, 2017

philipwhiuk Aug 11, 2017

philipwhiuk Aug 11, 2017

philipwhiuk Aug 11, 2017

philipwhiuk Aug 11, 2017

Trogel commented Aug 26, 2017

Trogel commented Aug 26, 2017

Valodim commented Aug 26, 2017

Trogel commented Aug 27, 2017

Valodim commented Aug 27, 2017

Trogel commented Aug 27, 2017

Trogel commented Aug 27, 2017

cketti commented Aug 27, 2017

Trogel commented Apr 2, 2018

		}


		public static Keyword getKeywordByExternalCode(String externalCode)

IMAP Keywords #2668

IMAP Keywords #2668

Conversation

Trogel commented Aug 7, 2017

Implemented features

Some more things I'd like to add

Major design changes compared to #558

Open issues

Trogel commented Aug 7, 2017

igoralmeida commented Aug 8, 2017

philipwhiuk commented Aug 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Trogel commented Aug 26, 2017

Trogel commented Aug 26, 2017

Valodim commented Aug 26, 2017

Trogel commented Aug 27, 2017

Valodim commented Aug 27, 2017

Trogel commented Aug 27, 2017

Trogel commented Aug 27, 2017

cketti commented Aug 27, 2017

Trogel commented Apr 2, 2018