Issue 619 android wear support #665

Closed
wants to merge 21 commits into
from

Conversation

Projects
None yet
3 participants
@MarcusWolschon
Member

MarcusWolschon commented May 27, 2015

Works well here for some time now in daily usage.
Huge improvement to the notification handling.
Needs more testing on a wider range of devices then what can be do but is definately stable enough to be merged back into trunk.

MarcusWolschon added some commits May 1, 2015

Support for stacked notifications in
#619  "Add android wear support"

No reply with voice yet (as requested in the ticket).
No user-configurable actions yet, just delete+archive+spam
fixed NPE
identified issue with entire stack of notifications when action on a single child-notification is used
added missing setContentTitle.
Trying to fix stacked notifications appearing on phone instead of wear-only. (phone is only supposed to show the summary notification)
#619 "Add android wear support"
added EXTRA_MESSAGE_ID to simplify closing the correct notification and make things more robust.
#619 "Add android wear support"
cleanup, comments and 1 missing intent-extra.
#619 "Add android wear support"
Fixed wrong icon.
Summary Notification is phone-only again because it is often clicked when the user intents to do an action only on a single mail.
@cketti

This comment has been minimized.

Show comment
Hide comment
@cketti

cketti Jun 7, 2015

Member

Great! I'm looking forward to integrating Android Wear support.

On a first glance this seems to contain some unrelated changes. What's with the app name change in the manifest?

We should not need JavaDoc comments inside K-9 Mail. The code style wiki page contains a bit more details on that.

The Git history could have used some cleaning up before submitting the pull request.

I'll try to check out the implementation in more detail in the coming days.

Member

cketti commented Jun 7, 2015

Great! I'm looking forward to integrating Android Wear support.

On a first glance this seems to contain some unrelated changes. What's with the app name change in the manifest?

We should not need JavaDoc comments inside K-9 Mail. The code style wiki page contains a bit more details on that.

The Git history could have used some cleaning up before submitting the pull request.

I'll try to check out the implementation in more detail in the coming days.

+ /**
+ * ID of the notification that triggered an intent.
+ * Used to cancel exactly that one notification because due to
+ * Android Wear there may be multiple notifications per account.

This comment has been minimized.

@maniac103

maniac103 Jun 8, 2015

Contributor

If that's the cade, you'll need to adapt the requestCodes of the PendingIntent creations, as those are what keep the intent extras in place; otherwise you'll still end up with one notification per account.

@maniac103

maniac103 Jun 8, 2015

Contributor

If that's the cade, you'll need to adapt the requestCodes of the PendingIntent creations, as those are what keep the intent extras in place; otherwise you'll still end up with one notification per account.

- if (account.isAnIdentity(message.getFrom()) && !account.isNotifySelfNewMail()) {
- return false;
- }
+ return !(account.isAnIdentity(message.getFrom()) && !account.isNotifySelfNewMail());

This comment has been minimized.

@maniac103

maniac103 Jun 8, 2015

Contributor

Is this really more readable? (IMHO it's not)

@maniac103

maniac103 Jun 8, 2015

Contributor

Is this really more readable? (IMHO it's not)

+ // }
+ // }
+ //}
+

This comment has been minimized.

@maniac103

maniac103 Jun 8, 2015

Contributor

All changes in this method seem pointless.

@maniac103

maniac103 Jun 8, 2015

Contributor

All changes in this method seem pointless.

@@ -158,7 +162,7 @@ public int compare(Message o1, Message o2) {
if (o1 == null || o2 == null || o1.getUid() == null || o2.getUid() == null) {
return 0;
}
- int id1, id2;
+ int id1, id2;

This comment has been minimized.

@maniac103

maniac103 Jun 8, 2015

Contributor

This whitespace change seems wrong.

@maniac103

maniac103 Jun 8, 2015

Contributor

This whitespace change seems wrong.

@MarcusWolschon

This comment has been minimized.

Show comment
Hide comment
@MarcusWolschon

MarcusWolschon Jun 8, 2015

Member

Damn, I got the notification emails into the wrong mail account.
I'll review and fix the identified issues.

The app name was not supposed to be part of the pull request. No idea how to exclude parts of the tree.

We should not need JavaDoc comments inside K-9 Mail.
The code style wiki page contains a bit more details on that.

This is the first time anyone requested NO comments. Usually code gets rejected
if it doesn't meet very strict "comment everything, explain what it is+what it's used for" rules.
I feel explaining why it's needed is required, because on a phone most developers will ever only
see exactly 1 notification per account any may try to remove or ignore that part and thus break it
without noticing that he/she breaks code that is for devices he/she doesn't test with or even know about.

return !(account.isAnIdentity(message.getFrom()) && !account.isNotifySelfNewMail());

Android Studio asked to refactor that and it does indeed look easier to read to me. YMMV

The commented out cancel-code was before I introduced storing the notification-id in an EXTRA.
That's why it's commented out.

Member

MarcusWolschon commented Jun 8, 2015

Damn, I got the notification emails into the wrong mail account.
I'll review and fix the identified issues.

The app name was not supposed to be part of the pull request. No idea how to exclude parts of the tree.

We should not need JavaDoc comments inside K-9 Mail.
The code style wiki page contains a bit more details on that.

This is the first time anyone requested NO comments. Usually code gets rejected
if it doesn't meet very strict "comment everything, explain what it is+what it's used for" rules.
I feel explaining why it's needed is required, because on a phone most developers will ever only
see exactly 1 notification per account any may try to remove or ignore that part and thus break it
without noticing that he/she breaks code that is for devices he/she doesn't test with or even know about.

return !(account.isAnIdentity(message.getFrom()) && !account.isNotifySelfNewMail());

Android Studio asked to refactor that and it does indeed look easier to read to me. YMMV

The commented out cancel-code was before I introduced storing the notification-id in an EXTRA.
That's why it's commented out.

@maniac103

This comment has been minimized.

Show comment
Hide comment
@maniac103

maniac103 Jun 8, 2015

Contributor

The app name was not supposed to be part of the pull request. No idea how to exclude parts of the tree.

Add a commit that reverts the changes and squash it into the one that introduced it using git rebase --interactive.

return !(account.isAnIdentity(message.getFrom()) && !account.isNotifySelfNewMail());

Android Studio asked to refactor that and it does indeed look easier to read to me. YMMV

IMHO, that particular refactor suggestion (which is related to J2ME - thus likely targeted towards code size optimization) does not make any sense to me personally - or can you tell me without thinking whether it'll return false or true if - say - account.isNotifySelfNewMail() returns false? Before, this was possible.
(And BTW, changes to code you didn't touch otherwise without a good reason are usually considered bad style)

The commented out cancel-code was before I introduced storing the notification-id in an EXTRA.
That's why it's commented out.

So is it needed now or not? If yes, why is it commented out? If no, why didn't you just remove it?

Contributor

maniac103 commented Jun 8, 2015

The app name was not supposed to be part of the pull request. No idea how to exclude parts of the tree.

Add a commit that reverts the changes and squash it into the one that introduced it using git rebase --interactive.

return !(account.isAnIdentity(message.getFrom()) && !account.isNotifySelfNewMail());

Android Studio asked to refactor that and it does indeed look easier to read to me. YMMV

IMHO, that particular refactor suggestion (which is related to J2ME - thus likely targeted towards code size optimization) does not make any sense to me personally - or can you tell me without thinking whether it'll return false or true if - say - account.isNotifySelfNewMail() returns false? Before, this was possible.
(And BTW, changes to code you didn't touch otherwise without a good reason are usually considered bad style)

The commented out cancel-code was before I introduced storing the notification-id in an EXTRA.
That's why it's commented out.

So is it needed now or not? If yes, why is it commented out? If no, why didn't you just remove it?

@MarcusWolschon

This comment has been minimized.

Show comment
Hide comment
@MarcusWolschon

MarcusWolschon Jun 8, 2015

Member

I'll clean that up and do another pull request.
It may take a while as I moved and won't get internet before mid- next week.

Member

MarcusWolschon commented Jun 8, 2015

I'll clean that up and do another pull request.
It may take a while as I moved and won't get internet before mid- next week.

@maniac103

This comment has been minimized.

Show comment
Hide comment
@maniac103

maniac103 Jun 8, 2015

Contributor

You don't need to do another PR. Just update the issue-619_AndroidWearSupport branch appropriately (using git rebase --interactive) and git push -f it.

Contributor

maniac103 commented Jun 8, 2015

You don't need to do another PR. Just update the issue-619_AndroidWearSupport branch appropriately (using git rebase --interactive) and git push -f it.

@cketti

This comment has been minimized.

Show comment
Hide comment
@cketti

cketti Jun 10, 2015

Member

I feel explaining why it's needed is required, because on a phone most developers will ever only
see exactly 1 notification per account any may try to remove or ignore that part and thus break it
without noticing that he/she breaks code that is for devices he/she doesn't test with or even know about.

If a comment is required to explain the concept, that's fine. But in that case the Wear-specific code should probably also go into its own class. Add one comment block at the top of the file to explain the high-level concept. Try to stay away from the implementation details as much as possible.

Member

cketti commented Jun 10, 2015

I feel explaining why it's needed is required, because on a phone most developers will ever only
see exactly 1 notification per account any may try to remove or ignore that part and thus break it
without noticing that he/she breaks code that is for devices he/she doesn't test with or even know about.

If a comment is required to explain the concept, that's fine. But in that case the Wear-specific code should probably also go into its own class. Add one comment block at the top of the file to explain the high-level concept. Try to stay away from the implementation details as much as possible.

@MarcusWolschon

This comment has been minimized.

Show comment
Hide comment
@MarcusWolschon

MarcusWolschon Jun 10, 2015

Member

The brance is cleaned up for pulling.
However, I don't understand what I'm supposed to do in the rebase command.
I guess I'll have to read up on that part of git first.
It seems to drop me into a vi with the following lines.
I guess that "noop" was supposed to be a list of commits or something.
I have a train to catch and will likely be offline for 5-8 days.

noop

# Rebase ca08a71..ca08a71 onto ca08a71 (       1 TODO item(s))
#
# Commands:
# p, pick = use commit
# r, reword = use commit, but edit the commit message
# e, edit = use commit, but stop for amending
# s, squash = use commit, but meld into previous commit
# f, fixup = like "squash", but discard this commit's log message
# x, exec = run command (the rest of the line) using shell
#
# These lines can be re-ordered; they are executed from top to bottom.
#
# If you remove a line here THAT COMMIT WILL BE LOST.
#
# However, if you remove everything, the rebase will be aborted.
#
# Note that empty commits are commented out
Member

MarcusWolschon commented Jun 10, 2015

The brance is cleaned up for pulling.
However, I don't understand what I'm supposed to do in the rebase command.
I guess I'll have to read up on that part of git first.
It seems to drop me into a vi with the following lines.
I guess that "noop" was supposed to be a list of commits or something.
I have a train to catch and will likely be offline for 5-8 days.

noop

# Rebase ca08a71..ca08a71 onto ca08a71 (       1 TODO item(s))
#
# Commands:
# p, pick = use commit
# r, reword = use commit, but edit the commit message
# e, edit = use commit, but stop for amending
# s, squash = use commit, but meld into previous commit
# f, fixup = like "squash", but discard this commit's log message
# x, exec = run command (the rest of the line) using shell
#
# These lines can be re-ordered; they are executed from top to bottom.
#
# If you remove a line here THAT COMMIT WILL BE LOST.
#
# However, if you remove everything, the rebase will be aborted.
#
# Note that empty commits are commented out
@maniac103

This comment has been minimized.

Show comment
Hide comment
@maniac103

maniac103 Jun 10, 2015

Contributor

You'll have to give it the amount of commits to rebase. If you want to squash e.g. 10 commits into 1, use git rebase --interactive HEAD~10 - giving it the commit range will cause the commits to appear in that vi, allowing you to reorder, squash etc. them.

Contributor

maniac103 commented Jun 10, 2015

You'll have to give it the amount of commits to rebase. If you want to squash e.g. 10 commits into 1, use git rebase --interactive HEAD~10 - giving it the commit range will cause the commits to appear in that vi, allowing you to reorder, squash etc. them.

@maniac103

This comment has been minimized.

Show comment
Hide comment
@maniac103

maniac103 Jun 10, 2015

Contributor

It looks like there's a return statement missing here.

It looks like there's a return statement missing here.

@MarcusWolschon

This comment has been minimized.

Show comment
Hide comment
@MarcusWolschon

MarcusWolschon Jun 15, 2015

Member

Is everything acceptable now?

Member

MarcusWolschon commented Jun 15, 2015

Is everything acceptable now?

@cketti

This comment has been minimized.

Show comment
Hide comment
@cketti

cketti Jun 19, 2015

Member

I rebased on master, squashed, and added the missing return statement.

The code worked fine in a quick test. I still need to have a closer look.

Member

cketti commented Jun 19, 2015

I rebased on master, squashed, and added the missing return statement.

The code worked fine in a quick test. I still need to have a closer look.

cketti added some commits Jun 25, 2015

Don't reinvent the wheel when creating MessageReferences
Also, we don't care about flags when dealing with MessageReferences for notifications.
@@ -34,37 +35,83 @@
private final static String EXTRA_ACCOUNT = "account";
private final static String EXTRA_MESSAGE = "message";
private final static String EXTRA_MESSAGE_LIST = "messages";
+ private final static String EXTRA_DONTCANCEL = "dontcancel";

This comment has been minimized.

@cketti

cketti Jun 25, 2015

Member

@MarcusWolschon: Did you introduce this to avoid canceling all notifications for an account when the action of a single notification was performed? It looks like EXTRA_NOTIFICATION_ID is now used for that and EXTRA_DONTCANCEL can be removed. Can you confirm?

@cketti

cketti Jun 25, 2015

Member

@MarcusWolschon: Did you introduce this to avoid canceling all notifications for an account when the action of a single notification was performed? It looks like EXTRA_NOTIFICATION_ID is now used for that and EXTRA_DONTCANCEL can be removed. Can you confirm?

This comment has been minimized.

@MarcusWolschon

MarcusWolschon Jun 26, 2015

Member

Yes, EXTRA_DONTCANCEL is no longer needed. I didn't notice it was still in there.

@MarcusWolschon

MarcusWolschon Jun 26, 2015

Member

Yes, EXTRA_DONTCANCEL is no longer needed. I didn't notice it was still in there.

- /* there's no point in keeping the notification after the user clicked on it */
- controller.notifyAccountCancel(this, account);
+ // if this was a stacked notification on Android Wear, update the summary
+ // notification and keep the other stacked notifications

This comment has been minimized.

@cketti

cketti Jun 25, 2015

Member

This comment states that the summary notification is updated. Where does that happen?

@cketti

cketti Jun 25, 2015

Member

This comment states that the summary notification is updated. Where does that happen?

This comment has been minimized.

@MarcusWolschon

MarcusWolschon Jun 26, 2015

Member

We may indeed have a bug here.

@MarcusWolschon

MarcusWolschon Jun 26, 2015

Member

We may indeed have a bug here.

@cketti

This comment has been minimized.

Show comment
Hide comment
@cketti

cketti Jul 2, 2015

Member

Status report:

There are quite a few issues with this approach.

  • discarding notifications on Wear isn't handled at all
  • in some cases the summary notification isn't properly updated
  • the mechanism to create notification IDs isn't working like it should
  • Wear notifications are only created for the latest 5 messages

The existing code already had some problems that got worse with this additional code:

  • NotificationData and the external synchronization is a mess
  • we keep LocalMessage instances in memory just to be able to display notifications
  • we do quite some work to be able to create a notification; and we do that every time a notification is updated
  • we sometimes use different terminology than the official documentation which makes it unnecessarily hard to follow what's going on

I'm currently working on improving improving all of these things.

Member

cketti commented Jul 2, 2015

Status report:

There are quite a few issues with this approach.

  • discarding notifications on Wear isn't handled at all
  • in some cases the summary notification isn't properly updated
  • the mechanism to create notification IDs isn't working like it should
  • Wear notifications are only created for the latest 5 messages

The existing code already had some problems that got worse with this additional code:

  • NotificationData and the external synchronization is a mess
  • we keep LocalMessage instances in memory just to be able to display notifications
  • we do quite some work to be able to create a notification; and we do that every time a notification is updated
  • we sometimes use different terminology than the official documentation which makes it unnecessarily hard to follow what's going on

I'm currently working on improving improving all of these things.

@MarcusWolschon

This comment has been minimized.

Show comment
Hide comment
@MarcusWolschon

MarcusWolschon Jul 2, 2015

Member

Okay.
When I have time again, I'll do an update of the "issue-619_AndroidWearSupport" branch before
trying to adress one or two of these points.

Member

MarcusWolschon commented Jul 2, 2015

Okay.
When I have time again, I'll do an update of the "issue-619_AndroidWearSupport" branch before
trying to adress one or two of these points.

MarcusWolschon added some commits Jul 5, 2015

#619 "Add android wear support"
removed dontCancel
update (stacked) notifications after a notification action was invoked
Merge remote-tracking branch 'origin/issue-619_AndroidWearSupport' in…
…to issue-619_AndroidWearSupport

# Conflicts:
#	k9mail/src/main/java/com/fsck/k9/controller/MessagingController.java
#	k9mail/src/main/java/com/fsck/k9/service/NotificationActionService.java
@MarcusWolschon

This comment has been minimized.

Show comment
Hide comment
@MarcusWolschon

MarcusWolschon Jul 5, 2015

Member

I removed dontCancel and added updating notifications after the NotficationUpdateService is done.
No idea how to refactor the existing NotificationData.
Not enough details to fix "the mechanism to create notification IDs isn't working like it should"
I don't think stacking more then 5 notifications would still be a sensible feature.

Member

MarcusWolschon commented Jul 5, 2015

I removed dontCancel and added updating notifications after the NotficationUpdateService is done.
No idea how to refactor the existing NotificationData.
Not enough details to fix "the mechanism to create notification IDs isn't working like it should"
I don't think stacking more then 5 notifications would still be a sensible feature.

#619 "Add android wear support"
removed dontCancel
update (stacked) notifications after a notification action was invoked
merged upstream changes
@MarcusWolschon

This comment has been minimized.

Show comment
Hide comment
@MarcusWolschon

MarcusWolschon Jul 5, 2015

Member

bug: After deleting a message the renewed notification still includes that message.
Aparently because it hasn't actually been deleted yet.
I'm investigating how to fix this...

The message is supressed instantly via actOnMessages() but NotificationData is not updated.
I see your point where NotificationData is a mess.

Member

MarcusWolschon commented Jul 5, 2015

bug: After deleting a message the renewed notification still includes that message.
Aparently because it hasn't actually been deleted yet.
I'm investigating how to fix this...

The message is supressed instantly via actOnMessages() but NotificationData is not updated.
I see your point where NotificationData is a mess.

@MarcusWolschon

This comment has been minimized.

Show comment
Hide comment
@MarcusWolschon

MarcusWolschon Jul 7, 2015

Member

cketti: How do I add new translation keys to Transifex?
I want "delete all", "spam all",... to make the different of acting on a single and all messages clear.
Otherwise this could lead to people accidentally deleting messages on the watch in stacked notifications.

Member

MarcusWolschon commented Jul 7, 2015

cketti: How do I add new translation keys to Transifex?
I want "delete all", "spam all",... to make the different of acting on a single and all messages clear.
Otherwise this could lead to people accidentally deleting messages on the watch in stacked notifications.

#619 "Add android wear support"
* updating notifications after acting on one of many messages fixed.

* still debugging stacked notifications (deleting second of 2 messages) . CONTAINS DEBUGGING CODE
@MarcusWolschon

This comment has been minimized.

Show comment
Hide comment
@MarcusWolschon

MarcusWolschon Jul 7, 2015

Member

Updating notifications when acting on one of many messages works.
However there is a bug with the delete intent. If deleting the second of 2 messages, the first is deleted instead. Can not figure out why yet. It should be impossible.

Member

MarcusWolschon commented Jul 7, 2015

Updating notifications when acting on one of many messages works.
However there is a bug with the delete intent. If deleting the second of 2 messages, the first is deleted instead. Can not figure out why yet. It should be impossible.

MarcusWolschon added some commits Jul 14, 2015

#619 "Add android wear support"
* debugged stacked notifications (PendingIntents would reference wrong messages due to wrong handling of the PendingIntent mechanism)
* fixed dismissing notifications
#619 "Add android wear support"
cleaned up debugging code
@MarcusWolschon

This comment has been minimized.

Show comment
Hide comment
@MarcusWolschon

MarcusWolschon Jul 14, 2015

Member

FIXED discarding notifications on Wear isn't handled at all
-> be carefull that on the latest Android Wear a dismiss can be un-done for a few seconds. So it's not instantly reflected on the phone but only after this timeout.
FIXED in some cases the summary notification isn't properly updated
FIXED the mechanism to create notification IDs isn't working like it should
WORKS AS DESIGNED Wear notifications are only created for the latest 5 messages
IMPROVED NotificationData and the external synchronization is a mess
OUT OF SCOPE we keep LocalMessage instances in memory just to be able to display notifications
OUT OF SCOPE we do quite some work to be able to create a notification; and we do that every time a notification is updated
OUT OF SCOPE we sometimes use different terminology than the official documentation which makes it unnecessarily hard to follow what's going on

I think we are ready for another review here about merging this with the mainline development in trunk.

Member

MarcusWolschon commented Jul 14, 2015

FIXED discarding notifications on Wear isn't handled at all
-> be carefull that on the latest Android Wear a dismiss can be un-done for a few seconds. So it's not instantly reflected on the phone but only after this timeout.
FIXED in some cases the summary notification isn't properly updated
FIXED the mechanism to create notification IDs isn't working like it should
WORKS AS DESIGNED Wear notifications are only created for the latest 5 messages
IMPROVED NotificationData and the external synchronization is a mess
OUT OF SCOPE we keep LocalMessage instances in memory just to be able to display notifications
OUT OF SCOPE we do quite some work to be able to create a notification; and we do that every time a notification is updated
OUT OF SCOPE we sometimes use different terminology than the official documentation which makes it unnecessarily hard to follow what's going on

I think we are ready for another review here about merging this with the mainline development in trunk.

@MarcusWolschon

This comment has been minimized.

Show comment
Hide comment
@MarcusWolschon

MarcusWolschon Jul 14, 2015

Member

cketti: At least the bugfix in the IntentService (setData()) must get into main trunk beause it means that
if there are multiple notification (e.g. with multiple accounts) the delete action deletes the wrong message!!!

Member

MarcusWolschon commented Jul 14, 2015

cketti: At least the bugfix in the IntentService (setData()) must get into main trunk beause it means that
if there are multiple notification (e.g. with multiple accounts) the delete action deletes the wrong message!!!

@maniac103

This comment has been minimized.

Show comment
Hide comment
@maniac103

maniac103 Jul 14, 2015

Contributor

I don't understand that point. The account number is passed as requestCode parameter precisely to avoid that issue. What does the setData() thing provide that requestCode doesn't?

Contributor

maniac103 commented Jul 14, 2015

I don't understand that point. The account number is passed as requestCode parameter precisely to avoid that issue. What does the setData() thing provide that requestCode doesn't?

@MarcusWolschon

This comment has been minimized.

Show comment
Hide comment
@MarcusWolschon

MarcusWolschon Jul 15, 2015

Member

maniac103: The point is that
a) Extras are ignored by Android in the comparison.
The accountNumber is an extra. It's not Data. It's not action.
b) With Android Wear we have stacked notifications and thus MULTIPLE notifications per account.
One per message that is being notified for.

Member

MarcusWolschon commented Jul 15, 2015

maniac103: The point is that
a) Extras are ignored by Android in the comparison.
The accountNumber is an extra. It's not Data. It's not action.
b) With Android Wear we have stacked notifications and thus MULTIPLE notifications per account.
One per message that is being notified for.

@maniac103

This comment has been minimized.

Show comment
Hide comment
@maniac103

maniac103 Jul 15, 2015

Contributor

Point a) is wrong. The account number is not only used as an extra. See the second parameter of PendingIntent.get[Activity|Service].
For b), I'd say it's better to have a unique request code (e.g. using a hash of the notified message(s)) than to stash random data into the intent.
And BTW, either approach should be put into a method so it can be reused than be copied around 10 times.

Contributor

maniac103 commented Jul 15, 2015

Point a) is wrong. The account number is not only used as an extra. See the second parameter of PendingIntent.get[Activity|Service].
For b), I'd say it's better to have a unique request code (e.g. using a hash of the notified message(s)) than to stash random data into the intent.
And BTW, either approach should be put into a method so it can be reused than be copied around 10 times.

@MarcusWolschon

This comment has been minimized.

Show comment
Hide comment
@MarcusWolschon

MarcusWolschon Jul 15, 2015

Member

Sure, go ahead!
Since this is way too dangerous to get wrong, I didn't want to use the notification-ID to be absolutely sure we never get any hash-collisions that result in the wrong messages to be deleted.

The perfect thing for you to implement would be to have actual URLs that point to our ContentProvider and do the requested action.
I'll be in vacation for the next serveral days and likely offline (deep in the woods).

Member

MarcusWolschon commented Jul 15, 2015

Sure, go ahead!
Since this is way too dangerous to get wrong, I didn't want to use the notification-ID to be absolutely sure we never get any hash-collisions that result in the wrong messages to be deleted.

The perfect thing for you to implement would be to have actual URLs that point to our ContentProvider and do the requested action.
I'll be in vacation for the next serveral days and likely offline (deep in the woods).

@maniac103

This comment has been minimized.

Show comment
Hide comment
@maniac103

maniac103 Jul 15, 2015

Contributor

I don't have time currently to look into this either.
Thinking about it though, it's absolutely safe to use the notification ID for this: if e.g. two notifications are created, and there's a hash collision on it, there won't be two distinct notifications in the first place anyway.

Contributor

maniac103 commented Jul 15, 2015

I don't have time currently to look into this either.
Thinking about it though, it's absolutely safe to use the notification ID for this: if e.g. two notifications are created, and there's a hash collision on it, there won't be two distinct notifications in the first place anyway.

@MarcusWolschon

This comment has been minimized.

Show comment
Hide comment
@MarcusWolschon

MarcusWolschon Jul 15, 2015

Member

I think you're right.
Still a proper URL should include the action and the messages to take an action on.
I guess this is a point we can change at will after merging into trunk.
I don't see these URLs are a major point and replacing one ugly construct (hash) with another one (notification id) isn't much of an improvement.
Considering that this is a precondition to fix the NotificationData class (unless anyone wants to double the workload), I'd accept it.

Member

MarcusWolschon commented Jul 15, 2015

I think you're right.
Still a proper URL should include the action and the messages to take an action on.
I guess this is a point we can change at will after merging into trunk.
I don't see these URLs are a major point and replacing one ugly construct (hash) with another one (notification id) isn't much of an improvement.
Considering that this is a precondition to fix the NotificationData class (unless anyone wants to double the workload), I'd accept it.

@MarcusWolschon

This comment has been minimized.

Show comment
Hide comment
@MarcusWolschon

MarcusWolschon Jul 21, 2015

Member

manuac103: That only is the case if there is just one action of a specific kind on a single notification.
"spam", "archive", "move to trash" are basically the same thing.

Since there's no limitaition to just 3 actions on the phone,
I'm thinking of offering the primary folders (or allowing users to set a flag per folder) as
"move to" targets right on the watch.
For users like me, that have an extensive folder structure, this allows to archive mail that isn't acted on into it's final resting place right away.
(I'm using my "follow up later today" as my archive folder and keep a clean inbox)

Member

MarcusWolschon commented Jul 21, 2015

manuac103: That only is the case if there is just one action of a specific kind on a single notification.
"spam", "archive", "move to trash" are basically the same thing.

Since there's no limitaition to just 3 actions on the phone,
I'm thinking of offering the primary folders (or allowing users to set a flag per folder) as
"move to" targets right on the watch.
For users like me, that have an extensive folder structure, this allows to archive mail that isn't acted on into it's final resting place right away.
(I'm using my "follow up later today" as my archive folder and keep a clean inbox)

@MarcusWolschon

This comment has been minimized.

Show comment
Hide comment
@MarcusWolschon

MarcusWolschon Aug 3, 2015

Member

I suggest to merge this branch and handle the redesign of the notification system in a separate ticket.
This is an often requested feature and the code is stable and performing without issues for month now.

Member

MarcusWolschon commented Aug 3, 2015

I suggest to merge this branch and handle the redesign of the notification system in a separate ticket.
This is an often requested feature and the code is stable and performing without issues for month now.

@cketti cketti referenced this pull request Sep 15, 2015

Merged

Add Android Wear support #795

@cketti

This comment has been minimized.

Show comment
Hide comment
@cketti

cketti Sep 15, 2015

Member

Sorry for the late feedback. I spent quite some time refactoring and cleaning up the notification code and created PR #795. It should contain all the features of this pull request, so I'm closing this one.

Lately I'm hesitant when it comes to merging pull requests that add functionality but do so in a way that decreases the overall code quality (e.g. enormously long methods and classes). I understand that many areas of the current code base encourage such "simple hacks". Because that's what we've been doing for the last couple of years. But every such change makes in only harder to make any changes in the future. In some areas of the code it's already incredibly hard to make changes without inadvertently breaking unrelated functionality.

So cleaning up the surrounding code should be part of adding a new feature. When we consequently do this, adding new features should become much easier and hence faster. Ideally we also increase the test coverage to gain confidence that new features don't break existing functionality.

Member

cketti commented Sep 15, 2015

Sorry for the late feedback. I spent quite some time refactoring and cleaning up the notification code and created PR #795. It should contain all the features of this pull request, so I'm closing this one.

Lately I'm hesitant when it comes to merging pull requests that add functionality but do so in a way that decreases the overall code quality (e.g. enormously long methods and classes). I understand that many areas of the current code base encourage such "simple hacks". Because that's what we've been doing for the last couple of years. But every such change makes in only harder to make any changes in the future. In some areas of the code it's already incredibly hard to make changes without inadvertently breaking unrelated functionality.

So cleaning up the surrounding code should be part of adding a new feature. When we consequently do this, adding new features should become much easier and hence faster. Ideally we also increase the test coverage to gain confidence that new features don't break existing functionality.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment