Notifications for new messages (Issue #162) #184

dsgkirkby · 2014-11-25T19:50:44Z

This addresses issue #162.

In the onCreate function of MainActivity, I added a call to create an Android alarm that triggers every hour.

I created a BroadcastReceiver that receives the alarm and checks for new messages on Reddit. Once it receives them, it parses the received messages and compares the newest message to the newest message from the previous check (which is saved in system preferences storage). If the new one is different, then the user has received a new message and a notification is broadcast to the user.

When the user taps this notification, it starts the app and there is a check in MainActivity of the Intent created by the activity to find out whether it is from a new message notification. If so, the inbox is opened for the user to view the new message they received.

Finally, I added a setting to enable/disable notifications, and I used Google Translate to provide translations of the setting's name.

This reverts commit 442c88b.

This reverts commit 03de931.

Initialization and declaration in one line.

QuantumBadger · 2014-11-25T20:51:34Z

Brilliant stuff, thanks!

A few minor comments before I pull this:

Please could you add a GPL copyright header to NewMessageChecker.java -- you can copy/paste this from another source file
It seems that NewMessageChecker remembers the last received message by serialising the entire message back to JSON (e.g. if (newMessage.toString().equals(oldMessage)) return;). This is really, really inefficient, and may fail to work in some cases. Please modify this to store just the ID of the message/comment. You can do this by doing the conversion from JsonValue to RedditComment/RedditMessage a bit earlier. The ID can be read from the RedditMessage.name and RedditComment.name fields.
The strings here should be moved to strings.xml. No need to translate them yourself :)
It's possible to remove the "project proposal" commits entirely using git rebase -i 8c326a3^ in the terminal and then deleting the two lines corresponding to those commits. I don't particularly mind you leaving them in though.
GitHub is telling me that a merge conflict has occurred, and the merge has to be done manually. I can do this for you if you want.

Generally this is fantastic stuff, and I hope to be able to put this in v1.8.7. Thanks again!

dsgkirkby · 2014-11-25T22:07:34Z

You're very welcome! I'll fix up those issues tonight and commit them.
I'd be happy to do the merge but I don't know how to get the changes made on your end into my branch.
FYI, this was done for a school project (though I'll be contributing more in the future even though the project is now over!) and I get bonus marks if the pull request is accepted before Dec. 3rd - if you could accept it before then that would be greatly appreciated!

QuantumBadger · 2014-11-25T22:47:04Z

if the pull request is accepted before Dec. 3rd

Absolutely -- I'll accept the pull request as soon as the changes are ready.

I'd be happy to do the merge but I don't know how to get the changes made on your end into my branch.

That's fine. I'm not a GitHub expert, but to get the contents of my repo into your local branch, you could add mine as a remote:

git remote add qb https://github.com/QuantumBadger/RedReader
git fetch qb

Then to rebase your changes onto mine, switch to your branch, and do:

git rebase qb/master

If there's a merge conflict, it can be resolved with git mergetool. If you're using Linux, I'd recommend meld as a merge tool (sudo apt-get install meld).

Even better, git rebase -i qb/master would also let you remove the extraneous commits.

I'm more than happy to do this myself, although it'll mean I can't accept the pull request -- I'd have to do the merge locally and close the pull request on the site. Hopefully that should be sufficient to get credit for your project though, as you'd be able to point to my merge commit on the site.

QuantumBadger · 2014-11-25T23:47:28Z

src/main/java/org/quantumbadger/redreader/activities/MainActivity.java

+        if (startInbox) {
+            InboxListingFragment.newInstance().show(this);
+        } else {
+            startMessageChecker();


I think this will actually create an alarm every time MainActivity is created. Over time this could add up to lots of alarms :)

I'd suggest either creating the alarm in the Application class, or ensuring that the alarm is destroyed in onDestroy.

What do you mean by creating the alarm in the application class?

Initialization and declaration in one line.

The code now uses strings.xml values rather than hard-coded stuff and only stores message IDs in system preferences.

…ew-Message-Notification Conflicts: res/values-es/strings.xml

QuantumBadger · 2014-11-26T08:21:17Z

The Application class is here:

https://github.com/QuantumBadger/RedReader/blob/master/src/main/java/org/quantumbadger/redreader/RedReader.java

onCreate should only be called once for the whole application, so the alarm should be created there. The intent should be stored so that the alarm can be cancelled in onDestroy.

I think even this could end up leaking alarms, if the application force closes. I'm not sure there's a better way to do it though (other than starting the alarm at boot).

Dismissed notifications would pop up again.

dsgkirkby · 2014-11-26T18:53:25Z

I think starting it at boot would be the best way, probably. I'll get that set up.

QuantumBadger · 2014-11-27T21:53:15Z

src/main/java/org/quantumbadger/redreader/activities/MainActivity.java

@@ -82,106 +86,111 @@
 	@Override
 	protected void onCreate(final Bundle savedInstanceState) {

-		PrefsUtility.applyTheme(this);


The indentation seems to be broken here -- I think the tabs have been replaced with spaces for the whole onCreate method.

QuantumBadger · 2014-11-27T21:59:55Z

Thanks! I've made a couple of comments about indentation -- as soon as those are fixed I'll accept the pull request.

I'm still not 100% sure about whether the alarm behaviour is correct (e.g. I don't think the alarm will be set until the first reboot after installing), but I'll investigate that myself after pulling these changes into the main repo.

Thanks for all your work -- there are lots of people after this feature, and it'll be good to get it into v1.8.7. Please let me know how you'd like to be credited in the changelog.

dsgkirkby · 2014-11-28T20:08:57Z

Yeah, I'm not fully happy with how it starts yet. You're right in that it requires a restart to function.
Maybe have some sort of static alarm stored somewhere (its own class?) and try to start it both at boot and app start?

dsgkirkby · 2014-11-28T20:09:40Z

Just credit me by my GitHub username.

Notifications for new messages (Issue #162)

QuantumBadger · 2014-11-28T22:31:53Z

Thanks! I'll try this out myself and will look into the best way to schedule the alarm.

If you're looking for another feature to work on, multireddits would probably be a good choice (if you're feeling ambitious!) I'm guessing some of the code could be adapted from the current subreddit subscription logic (see RedditAPIIndividualSubredditListRequester, RedditSubredditSubscriptionManager, etc).

QuantumBadger · 2014-11-28T22:33:23Z

src/main/java/org/quantumbadger/redreader/receivers/NewMessageChecker.java

+					} catch (IndexOutOfBoundsException e) {} // No new messages
+
+				} catch (Throwable t) {
+					notifyFailure(RequestFailureType.PARSE, t, null, "Parse failure");


Quick note for future reference -- IIRC notifyFailure() just calls onFailure(), so this doesn't do anything :)

dsgkirkby · 2014-11-29T23:13:44Z

Ok. I think I'll try to put together a static alarm class myself as well. If I get something I think works well, I'll send it your way.
The code up there was copied straight from InboxListingFragment, do you want me to change it or leave it for consistency?
Multireddits could be fun, probably won't start til after my exams finish though.
Thanks a bunch for the feedback!

QuantumBadger · 2014-11-30T16:48:58Z

The code up there was copied straight from InboxListingFragment, do you want me to change it or leave it for consistency?

That's fine -- feel free to leave it as-is.

Multireddits could be fun, probably won't start til after my exams finish though.

No problem, let me know if you have any questions.

dsgkirkby and others added 23 commits October 30, 2014 11:17

Project Proposal - This should be removed before pushing

8c326a3

Added receiver

69b0784

Create alarm for checking messages [TEST VALUES]

792cb0a

Initial upload

87159a4

Removed alarm and but it in a boot receiver

442c88b

Added boot complete permission

03de931

Initial upload - starts the new message checker on device boot

38459ad

Refactoring yo

b629f56

Revert "Removed alarm and but it in a boot receiver"

7a14758

This reverts commit 442c88b.

REmoved unnecessary import

c83831e

Revert "Added boot complete permission"

d6f5907

This reverts commit 03de931.

Removed Boot receiver

06e0ca3

Added notifications preference

bdde98c

Changed to inexactRepeating for lower battery impact

bae362a

Added notifications preference

05643e7

Added check for notifications preference

efa75db

Put alarm creation in its own function

b99dbc3

Removed debug statement and unnecessary comment

5369073

More descriptive notification messages

034402f

Opens inbox when called from newMessageChecker notification

de2b503

Removed unused imports

1e2d52b

Update MainActivity.java

d977687

Initialization and declaration in one line.

Delete project proposal before pull request

f6fa863

dsgkirkby changed the title ~~Notifications for new messages~~ Notifications for new messages (Issue #162) Nov 25, 2014

QuantumBadger reviewed Nov 25, 2014
View reviewed changes

Added translations for notification text

2fc438c

dsgkirkby and others added 11 commits November 25, 2014 18:36

Opens inbox when called from newMessageChecker notification

9d95269

Removed unused imports

9b89863

Update MainActivity.java

2f05a40

Initialization and declaration in one line.

Delete project proposal before pull request

3067874

Added translations for notification text

53d1b36

Added GPL license and restructured code

e2bc68e

The code now uses strings.xml values rather than hard-coded stuff and only stores message IDs in system preferences.

Merge remote-tracking branch 'origin/New-Message-Notification' into N…

b3dd8b6

…ew-Message-Notification Conflicts: res/values-es/strings.xml

Updated notifications translations

4af7559

Spacing fix for notifications

ef6da81

Start alarm in application rather than MainActivity

c9a7914

Better German translations

1098085

Bugfix

025637f

Dismissed notifications would pop up again.

dsgkirkby added 3 commits November 26, 2014 11:22

Removed alarm start

c1f9737

Added boot receiver that starts message checker

b689ca8

Reduced alarm interval

4397a06

QuantumBadger reviewed Nov 27, 2014
View reviewed changes

Fixed Indentation

7564d0e

QuantumBadger added a commit that referenced this pull request Nov 28, 2014

Merge pull request #184 from NoNeedToBail/New-Message-Notification

a20958f

Notifications for new messages (Issue #162)

QuantumBadger merged commit a20958f into QuantumBadger:master Nov 28, 2014

QuantumBadger reviewed Nov 28, 2014
View reviewed changes

alexanderfast mentioned this pull request Dec 3, 2014

[Feature Request] notifications for messages #162

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Notifications for new messages (Issue #162) #184

Notifications for new messages (Issue #162) #184

dsgkirkby commented Nov 25, 2014

QuantumBadger commented Nov 25, 2014

dsgkirkby commented Nov 25, 2014

QuantumBadger commented Nov 25, 2014

QuantumBadger Nov 25, 2014

dsgkirkby Nov 26, 2014

QuantumBadger commented Nov 26, 2014

dsgkirkby commented Nov 26, 2014

QuantumBadger Nov 27, 2014

QuantumBadger commented Nov 27, 2014

dsgkirkby commented Nov 28, 2014

dsgkirkby commented Nov 28, 2014

QuantumBadger commented Nov 28, 2014

QuantumBadger Nov 28, 2014

dsgkirkby commented Nov 29, 2014

QuantumBadger commented Nov 30, 2014

Notifications for new messages (Issue #162) #184

Notifications for new messages (Issue #162) #184

Conversation

dsgkirkby commented Nov 25, 2014

QuantumBadger commented Nov 25, 2014

dsgkirkby commented Nov 25, 2014

QuantumBadger commented Nov 25, 2014

QuantumBadger Nov 25, 2014

Choose a reason for hiding this comment

dsgkirkby Nov 26, 2014

Choose a reason for hiding this comment

QuantumBadger commented Nov 26, 2014

dsgkirkby commented Nov 26, 2014

QuantumBadger Nov 27, 2014

Choose a reason for hiding this comment

QuantumBadger commented Nov 27, 2014

dsgkirkby commented Nov 28, 2014

dsgkirkby commented Nov 28, 2014

QuantumBadger commented Nov 28, 2014

QuantumBadger Nov 28, 2014

Choose a reason for hiding this comment

dsgkirkby commented Nov 29, 2014

QuantumBadger commented Nov 30, 2014