This diff places the code responsible for local multimedia messages to setMessageStoreMessages action handling, since it is the only place we can do so, that we store messages in SQLite now.
Depends on D4508
Details
Details
- Reviewers
atul tomek • jacek - Commits
- rCOMMd9c67a45b575: Filter local multimedia messages
After this differential app is also expected to work in the same manner as before.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
@atul should definitely take a look here since he was responsible for "flipping the switch", which I think was broken without this change
Comment Actions
Looks good!
I was initially confused when I tried testing this locally because it appeared like local media messages were already removed by the time the setMessageStoreMessages handler was hit. After debugging for a bit (specifically processMessageStoreOperations(...) on C++ side to observe changes to SQLite store), I realized that D4517 wasn't included in the stack when I patched this diff... but everything worked after pulling in that commit.
I also had trouble getting the debugger to "attach" to Hermes in time for this action to be handled in message-reducer. Ended up testing via console.log to make sure values were as expected at each point. I copied the values into a text file as I was working, so it should just take a few minutes to add basic tests for this action. I'll spend like 5-10 minutes to see if I can get basic tests working, otherwise I'll drop it.
| lib/reducers/message-reducer.js | ||
|---|---|---|
| 1292–1294 ↗ | (On Diff #14456) | Thoughts on something like this? |
| 1301 ↗ | (On Diff #14456) | Got confused for a second here between actionPayloadMessages[id] (which is guaranteed to exist) and message.id (which doesn't exist for local messages). Doesn't really matter, but maybe we could rename the id in the loop to be something like clientDBMessageID to differentiate? |
| lib/reducers/message-reducer.js | ||
|---|---|---|
| 1306 ↗ | (On Diff #14456) | Alternatively to check whether a message is local, you could use message-utils.js:messageID(...).startsWith(message-utils.js:localIDPrefix) and use messageID(message) instead of id within the conditional block. |
| lib/reducers/message-reducer.js | ||
|---|---|---|
| 1292–1294 ↗ | (On Diff #14456) | I will introduce it. |
| 1301 ↗ | (On Diff #14456) | I will introduce it |
| 1306 ↗ | (On Diff #14456) | I am open to replace condition in if with the trick above since the code looks cleaner. On the other hand the condition inside if statement makes it clear what exactly do we want to filter. Therefore I would personally leave the condition as it is. @atul what do you think? |
| lib/reducers/message-reducer.js | ||
|---|---|---|
| 1306 ↗ | (On Diff #14456) | I think it's fine to leave the condition as is. I should've clarified it was more of a thought than a suggestion. Whatever you prefer is fine |