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

Avoid slow HTML parsing in hierarchy sort function #1383

Merged
merged 1 commit into from
Oct 26, 2022

Conversation

osma
Copy link
Member

@osma osma commented Oct 26, 2022

Reasons for creating this PR

The rendering of the concept hierarchy was very slow in some cases, see #1377. The reason is that HTML code is being parsed within the nodeLabelSortKey function and this can be very slow when there are many concepts shown in the hierarchy. This PR changes the way nodeLabelSortKey is implemented to avoid the slow HTML parsing, thus making the hierarchy show up a lot faster.

I couldn't find an easy way to benchmark this but the speedup is quite dramatic for instance in the case of the YSO concept "haircutting" that was used as an example in #1377.

Link to relevant issue(s), if any

Description of the changes in this PR

  • attach the labels to jsTree nodes representing concepts as a separate attribute label (similar to e.g. uri and notation)
  • when sorting nodes, look at the label attribute instead of parsing the HTML code to find it
  • remove extra trailing whitespace from a few lines that are directly adjacent to the above changes

Known problems or uncertainties in this PR

This PR changes the functions createConceptObject, createObjectsFromChildren, vocabRoot and createObjectsFromNarrowers which are also overridden in the footer.inc file which is part of the Finto customizations. If you have installed the customizations as they are currently written, this code will not work properly because the label attribute will be missing from nodes. The changes need to be applied to footer.inc as well. I will do that shortly.

Checklist

  • phpUnit tests pass locally with my changes
  • I have added tests that show that the new code works, or tests are not relevant for this PR (e.g. only HTML/CSS changes)
  • The PR doesn't reduce accessibility of the front-end code (e.g. tab focus, scaling to different resolutions, use of .sr-only class, color contrast)
  • The PR doesn't introduce unintended code changes (e.g. empty lines or useless reindentation)

@osma osma added this to the 2.17 milestone Oct 26, 2022
@sonarcloud
Copy link

sonarcloud bot commented Oct 26, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@codecov
Copy link

codecov bot commented Oct 26, 2022

Codecov Report

Base: 71.17% // Head: 71.17% // No change to project coverage 👍

Coverage data is based on head (61c6d07) compared to base (1d48d64).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff            @@
##             master    #1383   +/-   ##
=========================================
  Coverage     71.17%   71.17%           
  Complexity     1650     1650           
=========================================
  Files            32       32           
  Lines          3788     3788           
=========================================
  Hits           2696     2696           
  Misses         1092     1092           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@osma
Copy link
Member Author

osma commented Oct 26, 2022

Adjusted the footer.inc overrides for dev.finto.fi in this commit. I will merge this now so it will be available in dev.finto.fi.

@osma osma merged commit 0a51dd5 into master Oct 26, 2022
@osma osma deleted the issue1377-slow-hierarchy branch October 26, 2022 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done (verified in test.dev.finto.fi, set Milestone 3.0 for both issue & PR)
Development

Successfully merging this pull request may close these issues.

Opening the hierarchy for a concept takes a long time
2 participants