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

Hierarchical facets #164

Closed
wants to merge 3 commits into from

Conversation

EreMaijala
Copy link
Contributor

This is an initial implementation of support for hierarchical facets (bootstrap only). Includes fixes to issues in hierarchy tree in bootstrap theme (encountered when upgrading jsTree).

This is a bit larger than I would have liked due to needing to upgrade jsTree. I also found a few bugs in the hierarchy tree code and tried to make it work properly when I converted it to the new jsTree. Minimal sample records for testing can be found at https://github.com/KDK-Alli/NDL-VuFind/wiki/Hierarchical-Facets-Sample-JSON

The UrlQueryHelper changes were done because getParamArray is so slow that it caused a severe slowdown and I couldn't come up with a better solution. Maybe another solution could be devised for an overall performance improvement.

Changes in Results.php should also improve performance when retrieving coordinates for map display.

…only). Includes fixes to issues in hierarchy tree in bootstrap theme (encountered when upgrading jsTree).
@demiankatz
Copy link
Member

Chris is also in the midst of a jsTree upgrade, so I'll point him toward this code in case there's some way you can meet in the middle. Might take a while to reconcile everything for a merge, but I'll see what I can do. Thanks for sharing!

@EreMaijala
Copy link
Contributor Author

Nice or bad timing, depending on how much Chris has done, but I'm ok with rebasing this to his work if necessary.

@crhallberg
Copy link
Contributor

The jsTree update would be very handy, I actually moved ahead to a different tree library, so having the latest jsTree should fit nicely.

@crhallberg
Copy link
Contributor

The merge was a success! The latest JSTree and the new tree (FancyTree) are both working in Bootstrap.

Some suggestions I did when making the new tree:

  • Sort the children alphabetically for faster finding (Demian may disagree with this)
  • Visually distinguishing between collections and records
  • Clicking a collection should first expand it. I'm working on this problem myself

I'll work on pulling a tree based on config. Excellent work so far.

@EreMaijala
Copy link
Contributor Author

At least in our case sorting the children alphabetically would be wrong, since they already have an order that needs to be maintained. That could be a configuration option, though.

Do you think it would make sense to check if FancyTree would work for the hierarchical facets too? In VuFind 1 I used DynaTree, but converted to JSTree so as to not add another tree library...

@crhallberg
Copy link
Contributor

I agree, I think I'll take your JSTree update over introducing a new library.

However, having finally gotten your facets to work (problems on my end), I'd like to remove the numeric prefixes from the values, I think Animal/Pets/Dog makes more sense than 2/Animal/Pets/Dog.

Another factor is harnessing currently hierarchical data. For example, at Villanova we have our subjects already separated like Animal -- Pets -- Dog. By making the separator configurable, we could immediately harness the power of your facets.

I'm working on code now to make these changes if you'd like to take a look.

@crhallberg
Copy link
Contributor

Could you post a screen shot of how the hierarchical filters work?

@EreMaijala
Copy link
Contributor Author

The numeric prefix is taken directly from https://wiki.apache.org/solr/HierarchicalFaceting. Without the numeric prefix there's no way to fetch a specific hierarchy level. It's perhaps not a big issue if you always fetch the full facet set, but it should be possible to move to a more dynamic approach, if necessary, to make the tree faster. This requires that it be possible to fetch a single level at a time. This is actually what I did in VuFind 1, and you can check it out at https://www.finna.fi/?lng=en-gb. I'll try to get the VuFind 2 version online too, so you'll have something better than a screenshot to work with. I also feel I'm forgetting another reason for the prefixes.

Note that to work properly in all cases you need to have a trailing separator. Otherwise you'll end up matching anything that starts with the term (perhaps not typical but happened to us).

@EreMaijala
Copy link
Contributor Author

I added stripping of the leading number and trailing slash from display values. You can now see it in action at http://finna-dev-fe.csc.fi/test2/Search/Results?lookfor=&type=AllFields.

Note, though, that hierarchical facets don't really work that well with uncontrolled facet values like subject is. At least the current implementation would probably choke with large result sets since the number of facets returned is not limited. It could probably be made to work by applying suitable limits and fetching only the top level by default. We originally meant it for making the format and building facets shorter and easier to use, but I can work on making it more dynamic if you feel there's need for subjects etc. too.

@crhallberg
Copy link
Contributor

I wasn't aware that Solr-based Hierarchical Faceting is what we were aiming for. Are you using "facet.prefix" currently?

@EreMaijala
Copy link
Contributor Author

Yes. And this is of course just a suggestion that this be supported in the core, but if you find it too specialized, I'm ok with other options too.

Btw, I'm away from now on until 21 July so I can't get back to this until then.

@eocarragain
Copy link
Contributor

Ere's test UI looks great. I think it makes sense to use Solr's
hierarchical faceting if possible. Eoghan

On 19 June 2014 10:26, Ere Maijala notifications@github.com wrote:

Yes. And this is of course just a suggestion that this be supported in the
core, but if you find it too specialized, I'm ok with other options too.

Btw, I'm away from now on until 21 July so I can't get back to this until
then.


Reply to this email directly or view it on GitHub
#164 (comment).

@demiankatz
Copy link
Member

Ere,

When you return, do you want to try to rebuild this PR against the latest master? I believe Chris has incorporated some of your jsTree upgrade work into master, and there are now some conflicts that need to be resolved -- hopefully nothing too difficult!

@EreMaijala
Copy link
Contributor Author

Ok, I'll take a look.

@EreMaijala
Copy link
Contributor Author

Closing this PR as I'm switching to bootstrap3.

@EreMaijala EreMaijala closed this Jul 31, 2014
oschihin referenced this pull request in swissbib/vufind Aug 14, 2014
* Feld 'due_hour' entfernt
* use-on-site eigenständig definiert
* Übersetzungen korrigiert und erweitert
* @mschwendener bitte übersetzen nach en/fr/it
* bezieht sich auf #164
oschihin referenced this pull request in swissbib/vufind Aug 14, 2014
* Feld 'due_hour' entfernt
* use-on-site eigenständig definiert
* Übersetzungen korrigiert und erweitert
* @mschwendener bitte übersetzen nach en/fr/it
* bezieht sich auf #164
oschihin referenced this pull request in swissbib/vufind Aug 14, 2014
* Definition Status 'itemlost'
* Übersetzung für 'itemlost' (@mschwendener)
* 'lookonsite' erweitert um Grafik
oschihin referenced this pull request in swissbib/vufind Aug 14, 2014
* Definition Status 'itemlost'
* Übersetzung für 'itemlost' (@mschwendener)
* 'lookonsite' erweitert um Grafik
oschihin referenced this pull request in swissbib/vufind Aug 14, 2014
* Wertprüfung
* Darstellung eines übersetzten Werts
* inkl. Anzahl Vormerkungen
* kann man sicher noch schöner darstellen
oschihin referenced this pull request in swissbib/vufind Aug 14, 2014
* Wertprüfung
* Darstellung eines übersetzten Werts
* inkl. Anzahl Vormerkungen
* kann man sicher noch schöner darstellen
ToVie referenced this pull request in swissbib/vufind Aug 14, 2014
possible fix for @mschwendener comment on issue #164 Ausbau Anzeige Verfügbarkeitsinformationen
ToVie referenced this pull request in swissbib/vufind Aug 14, 2014
possible fix for @mschwendener comment on issue #164 Ausbau Anzeige Verfügbarkeitsinformationen
@EreMaijala EreMaijala deleted the hierarchical-facets branch September 11, 2014 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants