rrbrambley / messagebeast-android Goto Github PK

When the DisplayLocations are assigned upon loading messages from the database, a value of false is passed for the persist flag in lookupLocation(). This is because we assume that the display location was already assigned once and persisted when the retrieveMessages() method was called, so we optimize by avoiding a unnecessary insert. This could technically fail for all kinds of display locations, but geolocations are more susceptible to this failure because reverse geocoding can fail easily (e.g. when performing a ton of them upon full sync).

One solution might be to just assign display locations at the DB level - i.e. to rely on the display location instances table to populate the display locations when they are being pulled from sqlite. Then, only if a message does not have a display location we will attempt location lookup and always persist one if we find it.

It is currently depending on the message existing, which... will not work! crash

We should add a flag to the MessageManagerConfiguration so that a user can specify whether they want this feature on or not.

I'm not sure what exactly would happen if you give a bogus factual id, but since we provide caching of Places via sqlite, we should handle the case where you try to use a factual id that no longer exists.

When calling sendPendingDeletions(), we should send a broadcast or execute a callback after the operation is complete.

sendUnsentMessages() uses broadcasts, but sendPendingDeletions may be better suited for a handler since there are no long-running operations (e.g. file upload)

After the sendPendingDeletions() fix is added, it should be incorporated into sendAllUnsent()

Perhaps offer a “reconcile” method that refetches persisted messages to see if they are still NOT deleted? Hm.

This seems like a pretty tricky problem to solve.

On the other hand, MessageManager has this same problem. Is there really any clean way of doing this?

See #39.

Basically, now that a client app should not send unsent messages for an Action Channel, we should find ways to do this automatically, and better. Currently, after an action message's target message is sent successfully, ActionMessageManager gets a chance to then update its action message specs and then send unsent action messages. For this reason, this should work MOST of the time, but there will be scenarios when the action messages do not get sent (e.g. 429 too many requests), and will have to be sent later.

It seems like as more features are added, it's going to get harder and harder to make sense of this flag. It might be best to just remove the flag and always persist messages?

Currently, the MessageManager and FileManager both listen for the FileUploadService.INTENT_ACTION_FILE_UPLOAD_COMPLETE broadcast to perform their own tasks.

The MessageManager modifies the annotations of all messages dependent on the file and deletes pending file attachments from the db.

The FileManager removes the pending file from the db.

It's possible that the pending file could be removed from the db right before a crash/termination, leaving the Messages in an unrecoverable state (i.e. with pending message attachments pointing to a pending file id that doesn't exist).

Fix this.

Specifically, for pending deletions

When calling MessageManager.loadPersistedMessages(), it would be nice if there were some way to filter certain messages that have had an Action Channel action applied.

There may be other use cases too...

To support using custom places from Ohai, store them on sync and offer methods for getting custom places.

Currently, when loading messages from the sqlite db, the min Id of the currently loaded messages are used to determine the "before date" – the date that must come after the display date of all messages to be loaded.

However, when the displayDate != createdAt on a message (e.g. an ohai date is used), the min id may correspond to a message with a display date that is after a message with a greater id. I think.

Perhaps the solution is to always keep the min display date in memory and query that way?

Currently, upon fetching new messages from the server, the message manager (pretty much) always assumes it is safe to keep the messages in the channel's message map and incorporate their ids and dates into the MinMaxPair for that channel. However, because an app could create a new message with an old display date (e.g. ohai display date annotation), it is not safe to incorporate all new messages' dates/ids into the MinMaxPair – doing so means that subsequent calls to load persisted messages will use the min max pair and potentially leave gaps of messages out. This is bad.

In the first step of this method, we rely on the location instances search table to return place names. The values that are returned are sometimes totally different (DO NOT MATCH) the query string, and are also null sometimes. This is very strange because I explicitly check for a null location value before attempting to insert searchable display location instances in insertSearchableDisplayLocation().

This would require that a "beforeDate" can be accepted on methods for getting messages by annotation, display location, hashtag, etc.

Use case:

1. Create unsent message with file attachment, attempt send
2. Meanwhile, app shows the unsent message in the app.
3. The user chooses to delete the message as the upload is executing

This should presumably cancel the upload request, etc.

as described here: https://github.com/appdotnet/object-metadata/blob/master/channel-types/net.app.ohai.journal.md

Although action messages could lay around and have no effect when a their target messages are deleted, it'd be nice to have the action messages deleted automatically. A couple ideas:

1. ActionMessageManager listens for a broadcast that is sent whenever a delete happens, looks for action messages associated with the deleted message, deletes them.
2. ChannelSyncManager has a method for deleting a message and does the same work as in 1) above.

The OrderedMessageBatch contains a field of type TreeMap<Long, MessagePlus>, whose keys are message display dates. So, technically, if two messages are created at the same millisecond, then one of them will not be seen. Is this a problem? I don't think so. Just never create two messages with the same date. Multiple apps could post messages simultaneously though. Will they have the same exact date? Probably not. sigh

Currently, reverse geolocation will be performed rapidly when processing messages that need it (so, especially when a full sync is being performed).

However, I don't think Google wants this (too many rapid fire requests) and I think it causes failures sometimes. Perhaps a queue should be maintained and they should be fired off gradually? I'm actually not sure what the best solution for this is.

This is crappy when hashtags and links are needed for displaying the message, etc.

Currently, unsubscribe is available, but deactivate would be more appropriate: http://developers.app.net/docs/resources/channel/lifecycle/#deactivate-a-channel

In the event that a file has already been deleted, we should not try to re-delete in the future.

The configuration object currently defines the maximum message length as 2048 characters. It'd be great if we could add way off automatically gluing together multiple messages (via annotations or something) so that they could appear as one message – for those messages that need to exceed the max.

I think the "temporarily" suffix on these load methods is awkward. It should be more clear what these methods do – that the messages are not kept in MessageManager memory after loading.

This wouldn't be appropriate for all db requests, but would be a nice-to-have.

Since message ids are unique across channels, there isn't technically a reason why a single action channel cannot be used for multiple target channels.

We could either:

1. Not require a channel_id in the com.alwaysallthetime.action.metadata channel annotation, or
2. Have an alternate key – channel_ids that is an array of channel ids to be used with the action channel.

When a pending file is no longer available (the user deleted it?), we need a way to notify the client app and delete the unsent message.

https://github.com/appdotnet/object-metadata/blob/master/annotations/net.app.core.attachments.md

This should be handled very similarly to how pending OEmbeds work. Perhaps just a modification of that.
Also, when deleting messages and sending pending message deletions, file attachements should be deleted.

Because the message id changes after a message is sent, action messages that are created for unsent messages no longer apply after the unsent message gets sent. After a message is sent, we should somehow, automatically determine which actions have been applied to it and create new action messages for that message.

Many apps could benefit from being able to easily access messages that have specific annotations. There is already support for retrieving OEmbed instances from the sqlite database, but maybe the configuration should enable the extraction of any annotation types.

It seems like it might be related to pending deletions, but not sure.

i.e. if, while retrieving the new messages, an action channel cannot refresh because unsent messages exist, then the ChannelSyncManager doesn’t really do anything to notify the client

It'd be great to have a draft feature the MessageManager/sqlite DB. This is different than unsent messages because:

• Unsent messages will be sent when a connection is available. So they're like drafts that have already been committed to be sent.
• Drafts are typically in-progress work that don't show up in the same place in the UI as the rest of things. Therefore, it makes sense to have a different sqlite table and such.

Currently we rely on the Message Id for both of these tables. This is incorrect because:

• Offline message ids are created by examining the greatest message id in its target channel
• If an offline message is created in a channel, it is possible that the message id that is used already exists for a message in a different channel

The solution is most likely to change the primary key to be a messageId+channelId combo, but I will have to look into the performance implications for this.

For example, a DisplayLocationInstances may have two message ids for a location that only has one existing Message associated with it. My guess is that the display location instances are not properly being deleted when the unsent message is being deleted.

Perhaps this should be optional, or an array of annotations should be optional.

Since message ids are unique across channels, there isn't technically a reason why a single action channel cannot be used for multiple target channels.

We could either:

1. Not require a channel_id in the com.alwaysallthetime.action.metadata channel annotation, or
2. Have an alternate key – channel_ids that is an array of channel ids to be used with the action channel.

Currently, it can often take 5-10 seconds to obtain geocoordinates and then perform an ADN Place lookup. Because this is so slow (and because offline posting is important), it might be worth caching Places in a table and supporting lookup via geocoordinates.