pengyuthon/odoo_source
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

[FIX] mail: fix load older/newer not detecting change of isLoaded

Browse Source 
See comment in diff for rationale about the `isLoaded` change.

Also highlighting message before jumping to present can conflict: some
programatic scroll changes are prevented or delayed while there is a
highlight, jumping to present should clear that.

All "previous state" variables need to be reset when re-using the thread
component after making a change that led to reloading the message list
(such as a long jump). They are used to compare value before/after some
changes, but when jumping, the state should be considered clean.

`isJumpingRecent` is obsolete heuristics that has been replaced by
better controlling scroll and visibility check.

Extra `await` in `loadAround` makes no sense.

It's the best guess to fix the following runbot issues. The error
doesn't happen frequently enough to be a guaranteed fix.

runbot-40592
runbot-48557

closes odoo/odoo#145419

Signed-off-by: Alexandre Kühn (aku) <aku@odoo.com>
This commit is contained in:
Sébastien Theys
2023-12-08 11:16:35 +00:00
parent f84fc3c1e8
commit 9723bca3a7
6 changed files with 75 additions and 30 deletions
Download Patch File Download Diff File Expand all files Collapse all files
addons/mail/static/src/core/common/message_model.js
+1 -1
View File
@@ -110,7 +110,6 @@ export class Message extends Record {
/** @type {number[]} */
needaction_partner_ids = [];
/**
* @deprecated
* Still necessary until custom insert()/update() rely on this.
* Fields are computed only at end of update cycle, thus it is not
* computed during custom update
@@ -118,6 +117,7 @@ export class Message extends Record {
get originThread() {
return this._store.Thread.get({ model: this.model, id: this.res_id });
}
/** @deprecated */
originThread2 = Record.one("Thread", {
inverse: "allMessages",
compute() {
addons/mail/static/src/core/common/record.js
+11 -3
View File
@@ -1152,7 +1152,7 @@ export class Record {
}
static onChange(record, name, cb) {
const selfRaw = toRaw(this);
this._onChange(record, name, (observe) => {
return this._onChange(record, name, (observe) => {
const fn = () => {
observe();
cb();
@@ -1174,6 +1174,7 @@ export class Record {
* @param {Record} record
* @param {string|string[]} key
* @param {(observe: Function) => any} callback
* @returns {function} function to call to stop observing changes
*/
static _onChange(record, key, callback) {
let proxy;
@@ -1204,9 +1205,16 @@ export class Record {
}
return;
}
proxy = reactive(record, () => callback(_observe));
let ready = true;
proxy = reactive(record, () => {
if (ready) {
callback(_observe);
}
});
_observe();
return proxy;
return () => {
ready = false;
};
}
/**
* Contains field definitions of the model:
addons/mail/static/src/core/common/thread.js
+49 -17
View File
@@ -2,11 +2,13 @@
import { DateSection } from "@mail/core/common/date_section";
import { Message } from "@mail/core/common/message";
import { Record } from "@mail/core/common/record";
import { useVisible } from "@mail/utils/common/hooks";
import {
Component,
onMounted,
onWillDestroy,
onWillPatch,
onWillUpdateProps,
toRaw,
@@ -84,7 +86,7 @@ export class Thread extends Component {
this.loadOlderState = useVisible(
"load-older",
() => {
if (this.loadOlderState.isVisible && !this.isJumpingRecent) {
if (this.loadOlderState.isVisible) {
this.threadService.fetchMoreMessages(this.props.thread);
}
},
@@ -93,7 +95,7 @@ export class Thread extends Component {
this.loadNewerState = useVisible(
"load-newer",
() => {
if (this.loadNewerState.isVisible && !this.isJumpingRecent) {
if (this.loadNewerState.isVisible) {
this.threadService.fetchMoreMessages(this.props.thread, "newer");
}
},
@@ -112,6 +114,7 @@ export class Thread extends Component {
useEffect(
() => {
if (this.props.jumpPresent !== this.lastJumpPresent) {
this.messageHighlight?.clearHighlight();
if (this.props.thread.loadNewer) {
this.jumpToPresent("instant");
} else {
@@ -152,12 +155,13 @@ export class Thread extends Component {
useEffect(
(isLoaded) => {
this.state.mountedAndLoaded = isLoaded;
if (!isLoaded) {
this.loadOlderState.ready = false;
this.loadNewerState.ready = false;
}
},
() => [this.props.thread.isLoaded]
/**
* Observe `mountedAndLoaded` as well because it might change from
* other parts of the code without `useEffect` detecting any change
* for `isLoaded`, and it should still be reset when patching.
*/
() => [this.props.thread.isLoaded, this.state.mountedAndLoaded]
);
useBus(this.env.bus, "MAIL:RELOAD-THREAD", ({ detail }) => {
const { model, id } = this.props.thread;
@@ -236,6 +240,40 @@ export class Thread extends Component {
* in place (point 1).
*/
let loadNewer;
const reset = () => {
this.state.mountedAndLoaded = false;
this.loadOlderState.ready = false;
this.loadNewerState.ready = false;
lastSetValue = undefined;
snapshot = undefined;
newestPersistentMessage = undefined;
oldestPersistentMessage = undefined;
loadedAndPatched = false;
loadNewer = false;
};
/**
* These states need to be immediately reset when the value changes on
* the record, because the transition is important, not only the final
* value. If resetting is depending on the update cycle, it can happen
* that the value quickly changes and then back again before there is
* any mounting/patching, and the change would therefore be undetected.
*/
let stopOnChange = Record.onChange(this.props.thread, "isLoaded", () => {
if (!this.props.thread.isLoaded || !this.state.mountedAndLoaded) {
reset();
}
});
onWillUpdateProps((nextProps) => {
if (nextProps.thread.notEq(this.props.thread)) {
stopOnChange();
stopOnChange = Record.onChange(nextProps.thread, "isLoaded", () => {
if (!nextProps.thread.isLoaded || !this.state.mountedAndLoaded) {
reset();
}
});
}
});
onWillDestroy(() => stopOnChange());
const saveScroll = () => {
this.props.thread.scrollTop =
ref.el.scrollHeight - ref.el.scrollTop - ref.el.clientHeight < 30
@@ -249,7 +287,7 @@ export class Thread extends Component {
};
const applyScroll = () => {
if (!this.props.thread.isLoaded || !this.state.mountedAndLoaded) {
loadedAndPatched = false;
reset();
return;
}
// Use toRaw() to prevent scroll check from triggering renders.
@@ -264,9 +302,9 @@ export class Thread extends Component {
(this.props.order === "asc" &&
newerMessages &&
(loadNewer || thread.scrollTop !== "bottom"));
if (snapshot && !this.isJumpingRecent && messagesAtTop) {
if (snapshot && messagesAtTop) {
setScroll(snapshot.scrollTop + ref.el.scrollHeight - snapshot.scrollHeight);
} else if (snapshot && !this.isJumpingRecent && messagesAtBottom) {
} else if (snapshot && messagesAtBottom) {
setScroll(snapshot.scrollTop);
} else if (
!this.env.messageHighlight?.highlightedMessageId &&
@@ -331,20 +369,14 @@ export class Thread extends Component {
}
async jumpToPresent(behavior) {
this.isJumpingRecent = true;
this.messageHighlight?.clearHighlight();
await this.threadService.loadAround(this.props.thread);
this.props.thread.loadNewer = false;
this.present.el?.scrollIntoView({
behavior: behavior ?? (this.props.order === "asc" ? "smooth" : "instant"), // FIXME somehow smooth not working in desc mode
block: "center",
});
// Let smooth scroll a bit so load more is not visible
// smooth scrolling starts after 1 animation frame, hence needs to wait 2 animation frames
// for load more becoming not visible
await new Promise((resolve) => setTimeout(() => requestAnimationFrame(resolve)));
await new Promise((resolve) => setTimeout(() => requestAnimationFrame(resolve)));
this.state.showJumpPresent = false;
this.isJumpingRecent = false;
}
/**
addons/mail/static/src/core/common/thread_service.js
+1 -2
View File
@@ -298,6 +298,7 @@ export class ThreadService {
async loadAround(thread, messageId) {
if (!thread.messages.some(({ id }) => id === messageId)) {
thread.isLoaded = false;
thread.scrollTop = undefined;
const { messages } = await this.rpc(this.getFetchRoute(thread), {
...this.getFetchParams(thread),
around: messageId,
@@ -315,8 +316,6 @@ export class ThreadService {
}
}
this._enrichMessagesWithTransient(thread);
// Give some time to the UI to update.
await new Promise((resolve) => setTimeout(() => requestAnimationFrame(resolve)));
}
}
addons/mail/static/src/utils/common/hooks.js
+12 -7
View File
@@ -11,6 +11,7 @@ import {
useState,
} from "@odoo/owl";
import { browser } from "@web/core/browser/browser";
import { useService } from "@web/core/utils/hooks";
export function useLazyExternalListener(target, eventName, handler, eventParams) {
@@ -236,6 +237,7 @@ export function useScrollSnapshot(ref, { onWillPatch: p_onWillPatch, onPatched:
/**
* @typedef {Object} MessageHighlight
* @property {function} clearHighlight
* @property {function} highlightMessage
* @property {number|null} highlightedMessageId
* @returns {MessageHighlight}
@@ -244,6 +246,13 @@ export function useMessageHighlight(duration = 2000) {
let timeout;
const threadService = useService("mail.thread");
const state = useState({
clearHighlight() {
if (this.highlightedMessageId) {
browser.clearTimeout(timeout);
timeout = null;
this.highlightedMessageId = null;
}
},
/**
* @param {import("models").Message} message
* @param {import("models").Thread} thread
@@ -254,21 +263,17 @@ export function useMessageHighlight(duration = 2000) {
}
await threadService.loadAround(thread, message.id);
const lastHighlightedMessageId = state.highlightedMessageId;
clearHighlight();
this.clearHighlight();
if (lastHighlightedMessageId === message.id) {
// Give some time for the state to update.
await new Promise(setTimeout);
}
thread.scrollTop = undefined;
state.highlightedMessageId = message.id;
timeout = setTimeout(clearHighlight, duration);
timeout = browser.setTimeout(() => this.clearHighlight(), duration);
},
highlightedMessageId: null,
});
function clearHighlight() {
clearTimeout(timeout);
timeout = null;
state.highlightedMessageId = null;
}
return state;
}
addons/mail/static/tests/discuss/message_pin/pinned_messages_tests.js
+1
View File
@@ -134,6 +134,7 @@ QUnit.test("Jump to message from notification", async () => {
}
const { openDiscuss } = await start();
openDiscuss(channelId);
await contains(".o-mail-Message", { count: 21 });
await click(":nth-child(1 of .o-mail-Message) [title='Expand']");
await click(".dropdown-item", { text: "Pin" });
await click(".modal-footer button", { text: "Yeah, pin it!" });