Refactor hierarchy tree to an HTML-based one. #3277
Conversation
- Removes dependency on JSTree and remnants of it from the codebase. - Removes the defunct XML file support. - Adds all expected methods to the abstract base classes. - Retains the pluggable system for now.
There was a problem hiding this comment.
@EreMaijala, I just started this up so I could resolve conflicts on the branch, and I'm seeing some extra bullets in the "View Context" pop-ups in the sandal theme that I'm pretty sure are not supposed to be there:
I didn't take the time to test any other themes or look deeper into this, but hopefully this is helpful. If you want a more robust analysis of the state of things, I can ask @sturkel89 to help... but I thought you might want to do one more round of revisions first. Please let me know!
demiankatz
added
improvement
architecture
|
@demiankatz I can't reproduce that. Your screenshot looks like the current dev branch. This is how it should look (note the different icons):
Nevertheless, it would be really helpful if @sturkel89 could check things out. I've now merged the latest dev here. |
|
@EreMaijala, I double-checked that I was running the correct branch when I made my screenshot. It looks like your screenshot is of the collection tree tab, whereas my screenshot is of the "view context" lightbox that pops up from the search results when "showTree" is set to true in config.ini. I just rechecked this morning using both Firefox and Chrome to be sure it wasn't a browser-specific issue, but I'm seeing the same result in both places. |
|
@demiankatz Odd... I'll investigate. |
|
...and for those in the audience, the problem turned out to be that I was looking at the wrong branch, even AFTER double-checking. :-) |
|
@sturkel89, could you take a look at this now that my confusion has been identified? ;-) There are a few configuration changes to make, and then you can compare functionality here against functionality in dev, and also across the three themes. Based on the discussion above, I think you'll find that there are actually problems in the dev branch that are fixed here (like extra bullets showing up in appropriately), but if you see anything different, please let us know! Key configuration: in config.ini, set After making this change, if you filter to the "collections.mrc" records, you should see "In collection:" on all of the records, and if you click on the collection name, you'll show up in a collection record. Collection records should have a "Collection Items" tab showing the records inside the collection. All records that belong to a collection (subcollections and plain content records) should have a "Context" tab showing the tree where they live. The Context tab behaves a little differently depending on whether or not you're on a collection record. On collection records, clicking links loads summary information about the clicked items into the current page; on non-collection records, links take you away to the full page representing the clicked record. Other things to test: in the If you want to test with a bigger collection, let me know -- I plan to figure out a way to test this new code against the Digital Library to be sure that some of the more complex things still work right... but I think we might as well start by testing with the basic test data to see if we can catch anything obvious before we go after the subtleties. :-) |
|
I am in the process of looking at the test branch in all three themes. I also need to compare with dev. Before going forward and putting in a full review with screenshots, I wanted to mention my first observation: When I set collections = true in the [Collections] section, there was no Context tab visible at the Collection or Record view after I limited an open search to collections.mrc. It seemed that I needed to update the [Hierarchy] section with showTree = true in order to get the Context tab to show. Is this what’s expected? Demian's instructions above gave me the idea that the Context tab should have been available right away. |
|
Also, I will definitely want to test this branch and functionality against a larger set of records with more meaningful titles. That will help a lot in making the content I'm looking at more understandable to me. |
|
@sturkel89, sorry, you're right, the tree tabs also only show up when showTree is true. I'm pretty sure that's expected behavior. |
There was a problem hiding this comment.
Overall look and feel
With showTree = true set in the [Hierarchy] section, item records show a Context tab. All seems to work fine, and is formatted much more neatly than on dev. Here are screenshots of how that formatting appears on the test branch.
Sandal
Bootstrap3
Bootprint3
Main title formatting suggestion
On a landing page for a collection, the title of the collection is currently given in unstyled text that is the same size as the rest of the normal text on the page. It is not bolded or emphasized in any way. I think that the collection name should be bolded in a larger font, as it is in the preview you see on the right in the context tab. If you open the Show/hide more info feature, it's even more apparent that this title needs to stand out visually more than it does. I've put an orange box around the text I'm talking about.
Search within hierarchy tree
I have been testing searching within the hierarchy tree, and in most cases I cannot get it to work properly.
A couple of times, I have been able to enter a search term and then see the relevant items in the tree turn bold and italicized to tell me that there is a hit in that area.
However, most of the time either the entire tree closes back up (if I am in a top collection and have manually opened the tree), or I enter a term and nothing really happens Even though I know that there are items in the tree that contain my search term.
I am having a hard time replicating the circumstances under which I was able to successfully get searching to work.
When I set search = false in the [Hierarchy] section, the search box disappears. So that particular feature is working properly.
Here's an example of search working properly. I am in Top Collection 1, have clicked within the Context tab on Collection item 1. Then I search for the word Indiana. The record containing that word is highlighted; however my view has not changed from the item I already have selected. This [http://localhost/vufind_test/Collection/topcollection1/HierarchyTree#tabnav] is the URL I was at when things worked properly; however, there is no guarantee that this URL will work as I've seen it not work in other circumstances as described below.
When I switch to the Context menu of an individual item, I see the search box. But when I enter a term and click Search, my search term disappears and nothing is highlighted. You can see that the search button itself is now dark blue instead of a neutral color as in the screenshots above.
At this point, when I go back to the Context menu at the top of Top Collection 1 and try to search (from the same URL I was at before), the search does not work. My search term disappears and sometimes the opened tree closes back up when I click Search.
|
@sturkel89 The heading change has now been done in #3297. I'm having trouble reproducing the search issue. I've tried with both Firefox and Chrome. Regardless, I made a couple of small changes to ensure that any error messages are displayed properly. Could you try once more with the latest version? The tree closing is to be expected when the top-level item is the selected one, and an empty search is done, but I can't see why or how the field would get emptied. Do you see any errors or such in browser's console? Or network requests? |
|
Thanks for the progress, @EreMaijala! I also tried to reproduce the search problem and could not, so I'll be interested to hear if @sturkel89 is still seeing it after these latest changes. |
|
The fix to the title formatting looks good! Upon further testing I see that I have no problem with search within the hierarchy tree in Firefox. I'm continuing to see the behavior I reported when I use Chrome. I'm seeing the problem in dev as well as in the test branch. But!! I think what I'm seeing may be user error, combined with a difference in the way the two browsers deal with the voice input tool in Windows. I broke my elbow on Sunday and I have been using Windows voice typing to input my search terms. I'm doing a close comparison between my input and search behavior in the two browsers, and I see that the term appears in the box after I speak it, but sometimes IN CHROME when I click elsewhere on the screen or in the search text box, the term disappears. In those cases, I'm actually doing an empty search, not a search for my term. Firefox seems to be more sticky with keeping my search term present after I input it. So... I will keep testing, but I think we can disregard this particular observation of mine. |
There was a problem hiding this comment.
@EreMaijala, I spent a little time experimenting with the util/createHierarchyTrees.php command in this branch and discovered that it still contains a bunch of XML-specific logic that's causing it to fail unless I set the --skip-xml flag. I think we'll need to rework that utility to reflect the removal of the XML logic.
|
@EreMaijala, another interesting problem that may or may not have a solution... I connected this code to my live Digital Library Solr index. When I try to view the context tab for some of our very large collections, it's taking MINUTES to load the tree... and while the page is loading, none of the styles are applied, so it looks very strange. I know that it is working, because if I scroll to the bottom of the page, I can see data coming in gradually, and the page keeps getting longer and longer. Presumably it will finish eventually, but it's been going for a LONG time. Admittedly, this test is a very extreme case, because the real live system uses field collapsing to group records together, and in my quick-and-dirty test, I'm not doing that, which is resulting in many tens (or possibly hundreds) of thousands of tree nodes beyond what would normally be expected. I'm about out of time for today, but I can do more testing next week with more moderate cases if that would help... but in any case, I thought it might be useful to know about this as a potential problem. (I also have not compared the behavior here against the dev branch yet to see if it's notably different in any way... but that's also something I can do next week if it would be helpful to do so). I'm helping @sturkel89 get set up with the same type of test environment, so she should also be able to try some things soon. |
|
Update on my "huge tree" situation: after about 20 minutes of downloading HTML, the browser finally just gave up and stopped. Styles were never applied and Javascript never ran. As I say, this is probably not a realistic scenario and thus may not be cause for concern, but it definitely proves this code has some limits. :-) |
|
I have done a bit of testing using this much larger data set. At this point everything is working as expected. Please let me know if there are specific things you would like me to test. I have done a bunch of searching within the tree, both at the collection level and the collection item level, and as long as I am sure that the text in the search box is really present, the searches are coming up with accurate results. |
|
@sturkel89 Sorry to hear about your elbow. And thanks for finding out what the problem was, phew! |
|
@demiankatz It would be interesting to know how dev version behaves. I assume it will load and apply page styles properly since the tree is loaded asynchronously, but it'd be interesting to find out if it actually works. Perhaps the tree should be loaded asynchronously here as well, but there's certainly still a limit to how large a tree can be loaded to the DOM regardless of the way it's done. |
|
@demiankatz I tested with the largest hierarchy I have at hand, and it seems that there's a clear performance difference (like 10 times) in favor of the old code. I'll need to look at what's causing it. |
There was a problem hiding this comment.
Thanks for the progress, @EreMaijala. I did a bit more testing and have good news and bad news.
Good news: the updated command line utility appears to be working correctly for me.
Bad news: I'm seeing some problems with the hierarchy navigation in the context tab.
Problem 1: when viewing the context of a collection, clicking on records in the left panel tree does not update the description in the right panel. I just get this in my console:
Uncaught DOMException: Document.querySelector: '' is not a valid selector
showRecord http://localhost/vufind_test/themes/bootstrap3/js/hierarchy_tree.js?_=1704811924:22
setupTree http://localhost/vufind_test/themes/bootstrap3/js/hierarchy_tree.js?_=1704811924:178
setupTree http://localhost/vufind_test/themes/bootstrap3/js/hierarchy_tree.js?_=1704811924:176
setupTree http://localhost/vufind_test/themes/bootstrap3/js/hierarchy_tree.js?_=1704811924:176
initTree http://localhost/vufind_test/themes/bootstrap3/js/hierarchy_tree.js?_=1704811924:244
promise callback*initTree http://localhost/vufind_test/themes/bootstrap3/js/hierarchy_tree.js?_=1704811924:239
<anonymous> http://localhost/vufind_test/Collection/vudl:292799/HierarchyTree?recordID=vudl:292799#tabnav:369
<anonymous> http://localhost/vufind_test/Collection/vudl:292799/HierarchyTree?recordID=vudl:292799#tabnav:369
hierarchy_tree.js:22
Problem 2: when I click on an item link in the tree pop-up in search results and it takes me to the context tab on a record page, the anchor isn't working quite right. Once everything is finished loading, I scroll to a position close to the highlighted item, but that item is slightly "below the fold" and I have to scroll down to see it.
I'm testing with Firefox in case that makes a difference; are you seeing either or both of these problems on your end?
|
@demiankatz |
|
@EreMaijala, thanks for the fixes! Regarding problem 1, I confirmed that Mink tests were failing prior to its correction, and they pass afterwards. Good to know that case is covered (though I didn't bother running tests at the time that I discovered it). Regarding problem 2, the tree should scroll to the selected item whether it's an item or a collection. I confirmed the functionality in the dev branch. I think the problem was caused by an interaction between the unnecessary anchor tag and the scrolling Javascript code. I adjusted the Javascript to scroll unconditionally in commit 006ab00, and now the behavior seems acceptable to me. (The selected node is still at the bottom of the screen, but at least it's in view!). Please let me know if you have any concerns about this change. At this point, I think this is ready for another round of testing from @sturkel89. |
There was a problem hiding this comment.
Thanks, @EreMaijala. This looks good to me now, and all tests are passing!
The new HTMLTree renderer is based on the JSTree renderer, but outputs an HTML-based tree similar to hierarchical facets.
TODO