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

WIP: initial version of search #847

Merged
merged 2 commits into from
Oct 28, 2021
Merged

WIP: initial version of search #847

merged 2 commits into from
Oct 28, 2021

Conversation

kepper
Copy link
Member

@kepper kepper commented Jul 29, 2021

This is the first version of a new search based on Fuse.JS. It allows fuzzy searching, which is generally helpful. It may require some more finetuning, though, to get the best possible results. The current options I use for Fuse are found here. The index itself is generated here. In summary, I have three levels of things I'm searching in:

  • ident
  • desc
  • remarks

For most parts of the Specs, it's fairly easy to see where these are coming from. The first field has a weight of 10, the second a weight of 2, the last a weight of 1. For Guidelines Chapters, ident is the title of the chapter (including the chapter number), desc is a list of all elements etc. referenced explicitly, and remarks is the full text of the chapter, including examples. Only for guidelines chapters I render a preview from the remarks, as everything else is either quite obvious or hard to render at all. All those parameters can be adjusted as necessary.

The search index is generated alongside the web version of the guidelines and should work out of the box. Compilation times haven't changed for me, so this should be fairly efficient. The searchIndex.js that is generated weighs in at around 900kb, and is already quite compressed. We could think about doing this on the server instead, but I'm not sure this is really preferable.

Of course I'd like to get feedback from others as well (@musicEnfanthen, @rettinghaus, …) – I just failed to set you up as reviewers ;-)

I believe this is ready in general. That said, there is no rush to merge it – after tomorrow, I'll be offline for the coming two weeks ;-)

If merged, this fixes #749.

@kepper kepper added Component: Guidelines & Documentation changes to source: docs, examples or web (assigned automatically) Priority: Medium Type: Feature Request indicates that new features, that do not break backward compatibility, have been proposed Status: Needs Review Component: Workflows changes to GitHub Actions workflows (assigned automatically) labels Jul 29, 2021
@kepper kepper requested review from lpugin and bwbohl July 29, 2021 17:19
@github-actions github-actions bot added Component: Utils changes to utils/**/* (assigned automatically) and removed Component: Guidelines & Documentation changes to source: docs, examples or web (assigned automatically) Component: Workflows changes to GitHub Actions workflows (assigned automatically) labels Jul 29, 2021
Copy link
Member

@rettinghaus rettinghaus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

var tipuesearch = <xsl:value-of select="$json.string"/>;
</xsl:result-document>-->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if this is broken anyway, why not remove it completely?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't had the time to do a proper cleanup – that's one of the reasons why I said there's no rush to merge this.

</div>
</form>

<form><div class="tipue_search_group">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If tipue is gone, why keep the classes? I would prefer to have them renamed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, because of time constraints, I haven't done this yet. This still relies on a (potentially large) number of CSS rules from tipue, and cleaning this up may take some time. I agree it should be done, though.

@ahankinson ahankinson changed the title initial version of search WIP: initial version of search Jul 30, 2021
@ahankinson ahankinson marked this pull request as draft July 30, 2021 07:51
@ahankinson
Copy link
Member

I just converted this to a draft to indicate that it's still a WIP.

@kepper kepper added this to 2021-08-26: ODD Thursday in ODD Meetings Jul 30, 2021
@kepper
Copy link
Member Author

kepper commented Jul 30, 2021

Thanks @ahankinson – draft is surely more appropriate at this point. I will put it on the agenda for the next ODD meeting on August 26. I can't promise that I'll have the CSS cleaned up by then, but maybe some more people will have had an opportunity to look at this.

@kepper
Copy link
Member Author

kepper commented Aug 26, 2021

reviewed at ODD Thursday. Question (to myself): Is this going to work for MEI v4 as well?

@kepper kepper moved this from 2021-08-26: ODD Thursday to 2021-09-24: ODD Friday in ODD Meetings Aug 26, 2021
@bwbohl
Copy link
Member

bwbohl commented Sep 24, 2021

ODD meting notes:

  • code cleanup still needed
  • from the discussion I assume testing is also needed
  • Proposal is: get code cleanup done, merge and let users test ;-)

@bwbohl bwbohl moved this from 2021-09-24: ODD Friday to next in ODD Meetings Sep 24, 2021
should be clean now
@kepper
Copy link
Member Author

kepper commented Oct 28, 2021

the code for the v4 guidelines is static (see https://github.com/music-encoding/guidelines/tree/master/v4), so this won't give search on older versions. While it's doable to adopt this, I suggest to test it on the dev guidelines in the wild first…

@bwbohl
Copy link
Member

bwbohl commented Oct 28, 2021

2021-10-28 ODD meeeting:

  • functional again
  • old CSS stuff is removed now
  • only working on current dev
    ** if we want it on v4 and earlier we would need to generate indices once and add them to the corresponding files in the schema repo

@bwbohl bwbohl added Status: Ready To Merge indicates that a pull request is ready for merging and removed Status: Needs Work labels Oct 28, 2021
@kepper kepper marked this pull request as ready for review October 28, 2021 14:22
@bwbohl bwbohl merged commit 34429b1 into develop Oct 28, 2021
@rettinghaus
Copy link
Member

@kepper do you want to keep the search-guidelines branch for additional things? Otherwise I'd recommend to delete it.

@kepper
Copy link
Member Author

kepper commented Jan 25, 2022

let me see what I can accomplish this week. Would be nice to keep it as reference over there, but then remove it afterwards…

@musicEnfanthen musicEnfanthen removed the Status: Ready To Merge indicates that a pull request is ready for merging label Jan 28, 2022
@rettinghaus
Copy link
Member

@kepper The search isn't working because fuse.js is missing. There are several possible ways to fix that, but I would leave this to you. If you like I'll open a new ticket for this.

@kepper
Copy link
Member Author

kepper commented Jan 31, 2022

IIRC, this is an issue with moving things in the correct folder, so this is mostly an issue about the GitHub Actions workflows. Seems easiest to me to do this during a video call with someone more involved in this, maybe @musicEnfanthen, @bwbohl or you, @rettinghaus?

@bwbohl
Copy link
Member

bwbohl commented Feb 1, 2022

that was what I tried to address in #871 but if I understand correctly I was mistaken in removing fuse.js, right? So we need the build.xml patch and KEEP fuse.js, correct?

@bwbohl bwbohl deleted the search-guidelines branch May 2, 2022 23:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Utils changes to utils/**/* (assigned automatically) Priority: Medium Type: Feature Request indicates that new features, that do not break backward compatibility, have been proposed
Projects
No open projects
ODD Meetings
  
2021-10-28: ODD Thursday
Development

Successfully merging this pull request may close these issues.

search is broken
5 participants