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

Make phrasal matching case-insensitive #304

Open
martindholmes opened this issue May 16, 2024 · 7 comments
Open

Make phrasal matching case-insensitive #304

martindholmes opened this issue May 16, 2024 · 7 comments
Assignees
Labels
enhancement New feature or request fix committed A fix has been made, so if no problems emerge the issue can be closed.
Milestone

Comments

@martindholmes
Copy link
Collaborator

The Moses project team have requested that phrasal matching be made case-insensitive. I believe this could be done with a single change to what is currently line 2032 of StaticSearch.js:

//Make the phrase into a regex for matching.
      let re = new RegExp(strRe);

becomes

//Make the phrase into a regex for matching.
      let re = new RegExp(strRe, "i");

I think this is a good idea, and I see no reason not to do it, although it may involve some fallout for tests, which might perhaps retrieve more hits for specific test phrases. @joeytakeda Do you agree we should just implement this for version 2.0 assuming there are no gotchas when we try it?

@martindholmes martindholmes self-assigned this May 16, 2024
@martindholmes martindholmes added the enhancement New feature or request label May 16, 2024
@martindholmes martindholmes added this to the Release 2.0 milestone May 16, 2024
@martindholmes
Copy link
Collaborator Author

Actually it could be backported to 1.4 easily too. There will probably be documentation updates to do as well, though.

@martindholmes
Copy link
Collaborator Author

We introduced case-sensitivity on purpose in version 1.1, so that people could search for proper names:

"Phrasal searches are now case-sensitive, meaning that you can use a ‘phrasal search’ to search for proper names, by putting quotation marks around them and making the first letter upper-case."

I'd forgotten the history of this, but it's laid out in issue #104. That ticket ended with a comment from @joeytakeda:

I suppose we could make it configurable, but I'm hesitant to add more configuration options to the config. As far as I know, case sensitivity hasn't been an issue for any of the other projects, so I'm happy to just override the method for now unless it becomes clear that more projects want case insensitive phrasal searches.

And now we have one project which does. However, one project may not be enough to justify a new config item.

@martindholmes
Copy link
Collaborator Author

We decided to do an experimental implementation in a branch as follows:

  • If you have phrasal searching turned on in the config, the makeSearchPage.xsl file will create a checkbox control below the search text input box, with the caption "Make phrasal searches case-sensitive", which would be disabled initially, and would be checked (so current behaviour does not change).
  • An onchange event on the search text box detects when you have a phrasal search in the text box, and enables the checkbox as soon as one appears. It would be disabled again if you erase the phrasal search.
  • The StaticSearch object has a property controlling when phrasal search is case-sensitive, and that property is set at search time, based on the state of the checkbox.

@martindholmes
Copy link
Collaborator Author

This work will happen in the iss304-make-phrasal-search-case-sensitivity branch. (Screwed up the name, but meh.)

@martindholmes
Copy link
Collaborator Author

Basic working implementation now in iss304-make-phrasal-search-case-sensitivity branch. What remains to be done is tracking the checkbox's state in search history URLs. @joeytakeda I don't know if you were convinced of the usefulness of this, so you might want to try it out. One thing that's bugging me right now is that the auto-complete suggestions from the browser appear right where the checkbox and its label will appear and disappear, which means you don't necessarily see it right away when it appears.

@martindholmes
Copy link
Collaborator Author

Added the URL tracking bit. This is now functionally complete, but without any tests as yet.

@martindholmes martindholmes added the fix committed A fix has been made, so if no problems emerge the issue can be closed. label Jun 9, 2024
@martindholmes
Copy link
Collaborator Author

@joeytakeda If you try this out and you think it's a good idea, I'll add tests and documentation before creating a PR. I purposely used the CSS visibility property rather than display because any end-user who doesn't want it to appear can keep it hidden with display: none.

@joeytakeda joeytakeda modified the milestones: Release 2.0, Release 2.1 Sep 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request fix committed A fix has been made, so if no problems emerge the issue can be closed.
Projects
None yet
Development

No branches or pull requests

2 participants