-
Notifications
You must be signed in to change notification settings - Fork 382
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
Changes from all commits
973c545
582b053
b2a988c
35c724e
2a2f001
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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) { | ||
|
@@ -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) { | ||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
} | ||
} | ||
|
||
|
@@ -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) { | ||
|
@@ -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) { | ||
|
@@ -334,6 +452,7 @@ function convertData(json) { | |
* containing notifications for a single path (old format) | ||
*/ | ||
function updatePath(dashData, json) { | ||
this.updatingPath = true; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -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 | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -42,12 +42,19 @@ | |
</span> | ||
</td> | ||
</tr> | ||
<tr> | ||
<td class="aboutKey">Vue version</td> | ||
<td> | ||
<span class="aboutValue">{{ vueVersion }}</span> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nice. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
|
@@ -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); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
|
@@ -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" /> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -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: [], | ||
|
@@ -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(); | ||
}, | ||
|
||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) { | ||
|
There was a problem hiding this comment.
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