Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CLDR-17658 Improve Dashboard performance and UI; work in progress #3752

Merged
merged 5 commits into from
May 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
170 changes: 145 additions & 25 deletions tools/cldr-apps/js/src/esm/cldrDash.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,13 @@ import * as cldrStatus from "./cldrStatus.mjs";
import * as cldrSurvey from "./cldrSurvey.mjs";
import * as XLSX from "xlsx";

/**
* Notifications in these categories are combined so that there is not more than one per page.
* These categories can have well over 10,000 notifications, causing performance problems on
* the front end.
*/
const CATS_ONE_PER_PAGE = ["Abstained" /* , "Missing" */];

class DashData {
/**
* Construct a new DashData object
Expand All @@ -23,6 +30,8 @@ class DashData {
// An object whose keys are xpstrid (xpath hex IDs like "db7b4f2df0427e4"), and whose values are DashEntry objects
this.pathIndex = {};
this.hiddenObject = null;
this.pageCombinedEntries = {}; // map page to array of notification xpstrid on that page
this.updatingPath = false;
}

addEntriesFromJson(notifications) {
Expand All @@ -43,32 +52,107 @@ class DashData {
* @param {Object} e (entry in old format, from json)
*/
addEntry(cat, group, e) {
this.addCategory(cat);
this.catSize[cat]++;
if (!this.catFirst[cat]) {
this.catFirst[cat] = e.xpstrid;
try {
this.addCategory(cat);
this.catSize[cat]++;
if (!this.catFirst[cat]) {
this.catFirst[cat] = e.xpstrid;
}
if (CATS_ONE_PER_PAGE.includes(cat)) {
this.addCombinedEntry(cat, group, e);
} else if (this.pathIndex[e.xpstrid]) {
this.updateEntry(cat, group, e);
} else {
this.addNewEntry(cat, group, e);
}
} catch (err) {
console.error("Error in addEntry: " + err);
}
if (this.pathIndex[e.xpstrid]) {
const dashEntry = this.pathIndex[e.xpstrid];
dashEntry.addCategory(cat);
dashEntry.setWinning(e.winning);
if (e.comment) {
dashEntry.setComment(e.comment);
}

addCombinedEntry(cat, group, e) {
try {
const page = group.page;
if (!this.pageCombinedEntries[cat]) {
this.pageCombinedEntries[cat] = {};
}
if (e.subtype) {
dashEntry.setSubtype(e.subtype);
if (!this.pageCombinedEntries[cat][page]?.length) {
this.pageCombinedEntries[cat][page] = new Array(e.xpstrid);
this.addNewEntry(cat, group, e);
} else {
// TODO: make this work. unshift instead of push may be appropriate
// during an update (following a vote), since the combined notification
// is temporarily removed then added back, and in this case it may belong
// at the start of the array, not the end.
// Reference: https://unicode-org.atlassian.net/browse/CLDR-17658
if (this.updatingPath) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: this.updatingPath is always false here, because here "this" = dashData (as intended), but below when this.updatingPath is set to true, "this" = cldrDash

this.pageCombinedEntries[cat][page].unshift(e.xpstrid);
} else {
this.pageCombinedEntries[cat][page].push(e.xpstrid);
}
// Use the FIRST item in the array as the representative,
// with its comment indicating the size of the array
Comment on lines +93 to +94
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might be able to sort by section+code (if it's currently unsorted?) to get the lowest code. which should correspond to the 'highest' (visually) item. For future

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, there's a ticket already: https://unicode-org.atlassian.net/browse/CLDR-15648

In the meantime, the complete (all-path) dashboard json response is sorted, however, after voting on a path, there's a miniature dash response for that path alone, and then the front end doesn't always know where to insert it...

const xpstrid = this.pageCombinedEntries[cat][page][0];
if (!xpstrid) {
console.error(
"Existing xpstrid not found in addCombinedEntries for cat = " +
cat +
", page = " +
page
);
return;
}
const dashEntry = this.pathIndex[xpstrid];
if (!dashEntry) {
console.error(
"Existing entry not found in addCombinedEntry for cat = " +
cat +
", page = " +
page
);
return;
}
this.setComment(dashEntry, cat, page, null);
}
} else {
const dashEntry = new DashEntry(e.xpstrid, e.code, e.english);
dashEntry.setSectionPageHeader(group.section, group.page, group.header);
dashEntry.addCategory(cat);
dashEntry.setWinning(e.winning);
dashEntry.setPreviousEnglish(e.previousEnglish);
dashEntry.setComment(e.comment);
} catch (err) {
console.error("Error in addCombinedEntry: " + err);
}
}

updateEntry(cat, group, e) {
const dashEntry = this.pathIndex[e.xpstrid];
dashEntry.addCategory(cat);
dashEntry.setWinning(e.winning);
this.setComment(dashEntry, cat, group.page, e.comment);
if (e.subtype) {
dashEntry.setSubtype(e.subtype);
dashEntry.setChecked(this.itemIsChecked(e));
this.entries.push(dashEntry);
this.pathIndex[e.xpstrid] = dashEntry;
}
}

addNewEntry(cat, group, e) {
const dashEntry = new DashEntry(e.xpstrid, e.code, e.english);
dashEntry.setSectionPageHeader(group.section, group.page, group.header);
dashEntry.addCategory(cat);
dashEntry.setWinning(e.winning);
dashEntry.setPreviousEnglish(e.previousEnglish);
this.setComment(dashEntry, cat, group.page, e.comment);
dashEntry.setSubtype(e.subtype);
dashEntry.setChecked(this.itemIsChecked(e));
this.entries.push(dashEntry);
this.pathIndex[e.xpstrid] = dashEntry;
return e.xpstrid;
}

setComment(dashEntry, cat, page, comment) {
if (CATS_ONE_PER_PAGE.includes(cat)) {
dashEntry.setComment(
"Total " +
cat +
" entries on this page: " +
this.pageCombinedEntries[cat][page].length
);
} else {
dashEntry.setComment(comment);
}
}

Expand Down Expand Up @@ -149,9 +233,13 @@ class DashData {
}

removeEntry(dashEntry) {
const xpstrid = dashEntry.xpstrid;
this.removeEntryCats(dashEntry);
const index = this.entries.indexOf(dashEntry);
this.entries.splice(index, 1);
// Changed xpstrid means the entry wasn't really removed but was kept as representative of combined category
if (xpstrid === dashEntry.xpstrid) {
const index = this.entries.indexOf(dashEntry);
this.entries.splice(index, 1);
}
}

removeEntryCats(dashEntry) {
Expand All @@ -161,12 +249,42 @@ class DashData {
this.cats.delete(cat);
delete this.catSize[cat];
}
if (this.catFirst[cat] === dashEntry.xpstrid) {
if (CATS_ONE_PER_PAGE.includes(cat)) {
this.removeCombinedEntryCat(dashEntry, cat);
} else if (this.catFirst[cat] === dashEntry.xpstrid) {
this.findCatFirst(cat);
}
});
}

removeCombinedEntryCat(dashEntry, cat) {
const page = dashEntry.page;
this.pageCombinedEntries[cat][page].shift();
if (this.pageCombinedEntries[cat][page].length > 0) {
if (this.catFirst[cat] === dashEntry.xpstrid) {
const nextXpstrid = this.pageCombinedEntries[cat][page][0];
// If this is the only category for this entry, then we can revise the
// entry to use the next xpstrid for the page -- unless that next xpstrid
// is already present for a different category...
if (dashEntry.cats.size === 1 && !this.pathIndex[nextXpstrid]) {
dashEntry.xpstrid = nextXpstrid;
delete this.pathIndex.xpstrid;
this.pathIndex[nextXpstrid] = dashEntry;
this.catFirst[cat] = nextXpstrid;
this.setComment(dashEntry, cat, page, null);
} else {
// TODO: fix this; work in progress
// Reference: https://unicode-org.atlassian.net/browse/CLDR-17658
// Remove this cat from the existing entry
dashEntry.cats.delete(cat);
// if (dashEntry.cats.size === 0) {
// delete this.pathIndex.xpstrid;
// }
}
}
}
}

findCatFirst(cat) {
for (let dashEntry of this.entries) {
if (dashEntry.cat === cat) {
Expand Down Expand Up @@ -334,6 +452,7 @@ function convertData(json) {
* containing notifications for a single path (old format)
*/
function updatePath(dashData, json) {
this.updatingPath = true;
Copy link
Member Author

@btangmu btangmu May 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug here: "this" = cldrDash NOT dashData

It should be dashData.updatingPath, though it still might not work right; still work in progress

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FIxing that bug still doesn't make update work right; still working on it

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: the "this" bug was fixed in #3777

try {
if (json.xpstrid in dashData.pathIndex) {
// We already have an entry for this path
Expand All @@ -351,6 +470,7 @@ function updatePath(dashData, json) {
} catch (e) {
cldrNotify.exception(e, "updating path for Dashboard");
}
this.updatingPath = false;
return dashData; // for unit test
}

Expand Down
9 changes: 9 additions & 0 deletions tools/cldr-apps/js/src/views/AboutPanel.vue
Original file line number Diff line number Diff line change
Expand Up @@ -42,12 +42,19 @@
</span>
</td>
</tr>
<tr>
<td class="aboutKey">Vue version</td>
<td>
<span class="aboutValue">{{ vueVersion }}</span>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i wonder if there's a way to get antdv versions

</td>
</tr>
</tbody>
</table>
</div>
</template>

<script>
import { version } from "vue";
import * as cldrText from "../esm/cldrText.mjs";
export default {
Expand All @@ -58,10 +65,12 @@ export default {
comparePrefix: null,
compareMain: null,
compareRelease: null,
vueVersion: null,
};
},
created() {
this.vueVersion = " " + version;
fetch("api/about")
.then((r) => r.json())
.then(this.setData);
Expand Down
53 changes: 43 additions & 10 deletions tools/cldr-apps/js/src/views/DashboardWidget.vue
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
type="checkbox"
:title="describeShow(cat)"
:id="'dash-cat-checkbox-' + cat"
:checked="!catIsHidden[cat]"
:checked="!catCheckboxIsUnchecked[cat]"
@change="
(event) => {
catCheckmarkChanged(event, cat);
Expand Down Expand Up @@ -75,7 +75,8 @@
</header>
<section id="DashboardScroller" class="sidebyside-scrollable">
<template v-if="updatingVisibility">
<a-spin delay="1000" size="large" />
<!-- for unknown reason, the a-spin fails to appear on current Chrome/Firefox if any :delay is specified here -->
<a-spin size="large" />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still couldn't repro this. but, the logistics of this update are pretty odd..

</template>
<template v-else>
<template
Expand Down Expand Up @@ -181,6 +182,7 @@ import * as cldrNotify from "../esm/cldrNotify.mjs";
import * as cldrReport from "../esm/cldrReport.mjs";
import * as cldrStatus from "../esm/cldrStatus.mjs";
import * as cldrText from "../esm/cldrText.mjs";
import { nextTick } from "vue";
export default {
props: [],
Expand All @@ -195,12 +197,18 @@ export default {
localeName: null,
level: null,
downloadMessage: null,
catIsHidden: {},
catCheckboxIsUnchecked: {}, // default unchecked = false, checked = true
catIsHidden: {}, // default hidden = false, visible = true
updatingVisibility: false,
};
},
created() {
if (cldrStatus.getPermissions()?.userIsTC) {
this.catIsHidden["Abstained"] = this.catCheckboxIsUnchecked[
"Abstained"
] = true;
}
this.fetchData();
},
Expand Down Expand Up @@ -367,22 +375,47 @@ export default {
},
catCheckmarkChanged(event, category) {
// setTimeout solves a weakness in the Vue implementation: if the number of
// notifications is large, the checkbox in the header can take a second or more
// setTimeout is intended to solve a weakness in the Vue implementation: if the number of
// notifications is large, the checkbox in the header can take a second or even a minute
// to change its visible state in response to the user's click, during which time
// the user may click again thinking the first click wasn't recognized. Postponing
// the DOM update of thousands of rows ensures that the header checkbox updates
// the DOM update of thousands of rows should help ensure that the header checkbox updates
// without delay.
this.updatingVisibility = true;
setTimeout(
() => this.updateVisibility(event.target.checked, category),
0
// Also the booleans catCheckboxIsUnchecked and catIsHidden are distinct in order for
// the checkbox itself to update immediately even if the rows for the corresponding
// category may take a long time to update.
// Unfortunately, neither of these mechanisms seems guaranteed to prevent a very very
// long delay between the time the user clicks the checkbox and the time that the checkbox
// changes its state.
Comment on lines +378 to +389
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't say it's a weakness in Vue itself but a weakness in our usage of the order of 10k DOM objects.

this.catCheckboxIsUnchecked[category] = !event.target.checked; // redundant?
const USE_NEXT_TICK = true;
this.console.log(
"Starting catCheckmarkChanged; USE_NEXT_TICK = " + USE_NEXT_TICK
);
this.updatingVisibility = true;
this.console.log("updatingVisibility = true");
if (USE_NEXT_TICK) {
nextTick().then(() => {
this.updateVisibility(event.target.checked, category);
});
} else {
const DELAY_FOR_VISIBILITY_UPDATE = 100; // milliseconds
this.console.log(
"DELAY_FOR_VISIBILITY_UPDATE = " + DELAY_FOR_VISIBILITY_UPDATE
);
setTimeout(
() => this.updateVisibility(event.target.checked, category),
DELAY_FOR_VISIBILITY_UPDATE
);
}
},
updateVisibility(checked, category) {
this.console.log("Starting updateVisibility");
this.catIsHidden[category] = !checked;
this.updatingVisibility = false;
this.console.log("updatingVisibility = false");
this.console.log("Ending updateVisibility");
},
canBeHidden(cats) {
Expand Down
Loading
Loading