Rewrite MessageList store logic and refactor chat store #1161
Conversation
Jikstra
force-pushed
the
feat_fix_messagelist2
branch
from
ed964b4 to
00a63c1
Compare
Jikstra
changed the title
Jikstra
changed the title
- Fix message functions
overwrite the state as soon as we have fetched the new chat infos, but not before.
Jikstra
force-pushed
the
feat_fix_messagelist2
branch
from
79676e3 to
bd1801a
Compare
src/renderer/stores/chat.js
Outdated
| const { chatId, messages, totalMessages } = payload | ||
| if (chatId === chatStore.getState().id) { | ||
| chatStore.setState({ ...state, totalMessages, messages: [...messages, ...state.messages] }) | ||
| const messageIndex = state.messageIds.findIndex(mId => mId === msgId) |
| ...state.messageIds.slice(0, messageIndex), | ||
| ...state.messageIds.slice(messageIndex + 1) | ||
| ] | ||
| const messages = { ...state.messages, [msgId]: null } |
There was a problem hiding this comment.
Can you please add some comments here, what are you doing etc. all this index juggling is hard to read without comments
| } else if (type === 'MESSAGE_CHANGED') { | ||
| return { ...state, messages: { ...state.messages, ...payload.messagesChanged } } | ||
| } else if (type === 'SENT_MESSAGE') { | ||
| const [messageId, message] = payload |
There was a problem hiding this comment.
if SENT_MESSAGE means message is sent and MESSAGE_DELIVERED means message is delivered, please rename the first MESSAGE_SENT, one could easily misread SEND_MESSAGE...
src/renderer/stores/chat.js
Outdated
| const { msgId } = payload | ||
| callDcMethod('messageList.deleteMessage', [msgId]) | ||
| } else if (type === 'FETCH_MORE_MESSAGES') { | ||
| const oldestFetchedMessageIndex = Math.max(state.oldestFetchedMessageIndex - 30, 0) |
There was a problem hiding this comment.
Shouldn't we have a "buffer" variable instead of hard coded number? Would be more explicit...
src/renderer/stores/chat.js
Outdated
| }) | ||
| }) | ||
|
|
||
| ipcBackend.on('DC_EVENT_INCOMING_MSG', async (_, id, messageIdIncoming) => { |
There was a problem hiding this comment.
messageIdIncoming is never used...?
|
|
||
| function convert (message) { | ||
| export function RenderMessage (props) { | ||
| const chatStoreDispatch = useChatStore()[1] |
There was a problem hiding this comment.
this is a bit hard to read, what about [state, chatStoreDispatch] = useChatStore() ?
then it looks more similar to useState...
|
|
||
| const scrollPrepare = () => { previousScrollHeightMinusTop.current = doc.scrollHeight - doc.scrollTop } | ||
| useEffect(() => { |
There was a problem hiding this comment.
I really would like some comments in all these useEffect handler, what the related use case or scenario is. Very hard to understand by reading...
src/renderer/stores/store.js
Outdated
| return this._log | ||
| } | ||
|
|
||
| getName () { |
| const { deaddrop, onClose } = props | ||
| const chatStoreDispatch = useChatStore()[1] | ||
| const yes = async () => { | ||
| const messageId = deaddrop.msg.id |
There was a problem hiding this comment.
according to https://github.com/deltachat/deltachat-desktop/blob/feat_fix_messagelist2/src/main/deltachat/chatlist.js#L179 deaddrop has no msg property but just an id
| const oldestFetchedMessageIndex = messageIds.length - PAGE_SIZE | ||
| const newestFetchedMessageIndex = messageIds.length | ||
| const messageIdsToFetch = messageIds.slice(oldestFetchedMessageIndex, newestFetchedMessageIndex) | ||
| const messages = await callDcMethodAsync('messageList.getMessages', [messageIdsToFetch]) |
There was a problem hiding this comment.
Why do we need 3 times to call callDcMethodAsync?
Why is messageIDs not fetched together with chatList.selectChat
Why do we need to calculate messageIdsToFetch here? PAGE_SIZE could be a shared variable, so the calculation could be done also together with chatList.selectChat
Renaming Bugfix accept contact request Adapt Test dom helper
Missing:
Cannot convert undefined or null to object at Function.keys (<anonymous>) at module.exports../src/renderer/stores/MessageList.js.MessageListStore.effects.push (MessageList.js:123)Fixes #1166
Fixes #1158
Fixes #477
Fixes #1180
Fixes #1122 (comment)
Fixes #712