kitodo-production
kitodo-production copied to clipboard
Improve "correction K" tooltip
The tooltip is now rendered using p:tooltip
and also lists all normal comments in addition to the correction comments.
Correction comments in the tooltip are marked with a K
/C
in red (not yet resolved) or grey (already resolved).
The column is renamed from "Correction message" to "Comments". It can now display three different symbols:
- a grey speech bubble for normal comments
- a red
K
/C
for an unresolved correction comment - a grey
K
/C
for at least one resolved correction comment
Resolves #4994.
This will create substantial merge conflicts with #5360, which stores the comment status in ElasticSearch to be sortable.
#5360 has been merged. Please resolve resulting conflicts.
Please rebase against master to fix broken Github CI builds!
@oliver-stoehr I tested your pull request. Functionally, it seems to work just fine and resolves the conflicts mentioned earlier. From this perspective, the pull request is acceptable.
However, I do have some performance concerns. When rendering a process list with 100 processes, the performance of the backend call that retrieves the DataTable contents (inspected via the F12 browser network tab) degrades from ~0.8 seconds (current master) to ~1.3 seconds. It is not a huge deal, but noticable. Still, in my database I only have in total ~8 comments. I could imagine, if there are more comments in your database, performance might be affected even more.
I suspect the performance degradation is caused by multiple calls to ProcessForm.getComments(process)
in processList.xhtml
, which each will trigger additional 100 database queries to find all comments of all listed processes. Unfortunately, theses database queries are also not cached in Kitodo. So, two calls to ProcessForm.getComments(process)
will trigger 200 database queries. Maybe it is possible to improve the code to save one or two calls or even write a custom database query that retrieves all comments with a single database query? For example, you could create a RequestScoped bean that issues the custom database query and caches the comments during the lifetime of the request (and potentially multiple calls to the bean getter method when rendering the jsf).
Ideally, since the comments are not even shown to the user without hovering over the comment symbol, the tooltip contents could be loaded only when hovering occurs, instead of pre-loading all comments of all listed processes. Unfortunately, the Primefaces Tooltip component doesn't seem to support this.
Thank you for pointing out! I moved the getComments
methods to a separate bean which caches the comments per request. So there should now be only one database query per process.
The comment icon's rendered
attribute now relies on index information and does not query the database. Same applies to processes without comments.
@thomaslow could you finish your review for this pull request and check whether your prior remarks have been incorporated sufficiently?
Sorry. As I said in November, I really do not have any more time left for Kitodo. The review above was already made as a favor to the community.
@thomaslow sure, I understand, no problem. Thanks for the initial review, though!