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
feature: Implement Search Settings Ability #416
Conversation
Looks good, but specs would be great for such a large new feature. |
Yeah you make a good point. I'll go ahead and add some here after looking at the existing ones to see how they are testing this section |
So I do definitely plan on adding some tests here, but wanted to add some quick stats I was able to generate about how it runs. With the below being the
So not sure why such a huge difference between |
break; | ||
default: | ||
// The handling for any packages name | ||
atom.workspace.open(`atom://config/packages/${settingLocation}`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a difference between builtin packages and installed packages that makes installed packages not work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Surprisingly the URI for the settings page of all packages is exactly the same. Made sure this functioned for either the core packages or community packages.
Only thing not explicitly tested for is Git installed packages, but I'd imagine they should work as well, especially since the settings-view
showPackage()
function is written identically.
But of course anyone with Git installed packages on their system could double check by navigating to this URI format in the browser and letting it open up Pulsar
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been using the package-settings package for ages in order to jump quickly to a given package's settings, and this is the exact method that package uses. Never observed it not to work, no matter how a package is installed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I got confused, that's relieving. I think for clarity, the packages caveat means:
- Does not search for the names of any packages
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I got confused, that's relieving. I think for clarity, the packages caveat means:
- Does not search for the names of any packages
Ahh I see how that can be confusing. Yeah just means it's not the search that's available on the "Packages" pane, as in it won't accomplish the same thing.
(Although technically you could search by package name to help bring your package up, but it will only show results for packages that have settings. As the name of the package is considered when doing a search. But this makes the whole thing a little confusing, so though it was easier to say it doesn't search for packages)
I'm for the concept of this, it seems like a really good idea! ✅ Haven't looked over all the code yet or anything, but I'm generally in favor! Thanks for doing this! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very cool feature!
I am testing this now, and it does let you jump to the package where the setting it finds is! (The basic premise of the feature is working, which is not at all a given in reviewing PRs! EDIT: More helpful thing to say: works on my machine too, not just the author's machine. Which it should, JS is cross-platform, but yeah!)
A couple of thoughts:
- Would be amazing if we could jump to the specific setting within the package's settings page. Since the search results are so specific, but they can only jump to [the top of] the package's settings page.
- But I can see why this would not be easy -- we would have to add stuff like
id=
attributes onto the settings page of the different packages, and I'm not sure if anchors like#setting-name
would work in theatom://
URIs this is using???
- But I can see why this would not be easy -- we would have to add stuff like
- Would be amazing if there was some "back" button to go back to search.
- Counter-point: the search button in the sidebar is effectively exactly this, since it jumps to the search exactly as it was before the user visited the given package. Just not obvious, slightly clunky UX going back and forth a lot. (But again, the fact this works at all is cool, this is just nit-picking.)
- It's a little unintuitive what's going on if you type something with no truly relevant results. Searches such as "hello" and "onomatopoeia" have several results that seem unrelated. I don't know if it's a problem to show results in this case, but it does feel unexpected at least.
This feature existing at all is frankly really cool. So consider the above to just be any areas I could think of for improvement after some brief testing. Not expecting these things to be implemented necessarily.
Thanks! ✅
@DeeDeeG yeah the fact it doesn't jump to search results on the page is a bummer. Doesn't seem that the URI parses anything after the direct path to the config, and iirc in my testing the URI wouldn't accept it even, as the regex used wouldn't validate it. One thing I am currently working on, is trying to highlight the portion of search results that caused a match, which could help in determining why you sometimes get bunk results. But otherwise you could try increasing the new setting within Or as we can now say, just paste that into settings search and click the results to go ahead and change it! |
Alright, so @DeeDeeG I may have lied. That new feature I wanted is seemingly a lot more complicated than I hoped to add. So I may leave that to another day if we plan to have this in anytime soon. But otherwise I've added some small tests here. Frontend testing is not my strong suite, but there are some tests to help prevent any regressions mainly focusing on the data format |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks awesome! I am rerunning the checks though, just to be sure. It looked like there was an additional package test that failed, which doesn't normally
I'll keep a lookout if one does happen to fail, thanks for checking that. But thanks for the approve! |
|
Ahh thanks for catching this, I'll get on that |
Just to preface this, this is amazing and definitely something sorely needed. Warning - words. I'm sorry. Things for this PRAgain, amazing that it works so well already. I don't really have much to say on the technical aspects of how stuff is ranked other than it seems to work well. Most of this is UI or UX stuff that I can see being brought up.
Things for future development of the feature
This is all I can really think of for the moment, I'm sure I'll have some other |
@Daeraxa I appreciate the insight on this one. And these are all amazing points. The one thing I have given some serious consideration to that you mentioned, is being able to search only core settings, or only package settings, do we think this could function as a flag? Such as The easiest one I think would be making the package something comes from more obvious, and adding a click handler to the entire card. Since you are correct, the cards are nearly identical to the package cards, but we could change that without much issue. |
Honestly so long as it is documented I personally don't mind what the syntax should be. However I wasn't familiar with the The one objection I can see to this is that we already use this exact same syntax for excluding stuff in the project find package. For example using |
I've been too deep in tree-sitter stuff to check this PR out, but from the screencast above, my main feedback is that I'd love to get the package name displayed more prominently, and I think it might even be a good idea to group the results by package. I can pretty easily see people using this search screen to jump to the settings for a particular package. Thus they'd search for In the screencast above, you search for Failing that, I think that all settings for package X should receive a massive relevancy score boost if the user's entire search term matches X. When you search for |
My other impulse is that this screen should not show any settings from themes that are not the active UI theme and syntax theme. I notice that For our purposes, I think a UI or syntax theme that isn't currently active is the equivalent of “disabled,” and the search results should exclude them even if |
One bodge for highlighting would be to add "#highlight-this" to the url, and have atom.workspace.open somehow process that and highlight the respective setting. Another thing is that "tab" could also refer to indentation settings... like "move cursor as if tab" was harder to find than I expected. |
@icecream17 raises a good point about how to treat visible new features that we want to get feedback on but don't necessarily want to push out to everyone. We're talking about shipping the new tree-sitter implementation behind a hidden/experimental setting and making it opt-in, so maybe that's worth doing here? Then we can all use it for a few weeks and decide what we like/dislike about it. |
I love the amount of feedback I'm getting on this PR, but to try to answer everyone. @Daeraxa : You make a fantastic point about using search modifiers. Tbh I had forgotten that DDG bangs only search other websites. Meanwhile @savetheclocktower : So it'd be trivial to increase the bonus for having the search match high with the package name itself, although I do still wonder that if that should be higher than having the setting key or title match. Since if someone types But you also make a good point about grouping search results. One that has also been brought up before. I wouldn't be at all opposed to this, but feel like the section would have to be collapsible. Since if the option is incorrect I'd hate to make people scroll to far to get past it. Although I do personally feel the best matching searches being first no matter the package feels better to me, but we can absolutely change that, or even make it configurable? @icecream17 I also totally agree I'd love to make URI's accept But finally about the feature flags I think this could be cool. I went ahead and posted a bit on Discord, since I feel we would have to flesh out that idea a little bit more and wanted to get more ideas. But thanks to everyone, I'll take a look at what I can today |
… `settings-panel.js` and import into both search settings and original `settings-panel`
Alright, so while trying to consider everyone's points and the things are easily doable now:
Here's an updated look of our search results with the above changes: EDIT: The package icons now appear on the left side of the title. Based on feedback on Discord |
Alright some new changes.
I am still trying to figure out how best to accomplish this for packages settings, but this is a good step forward on that front, and works without any issue in my testing.
|
Feel free to let me know if there's anything else we wanna see fixed. Since if possible I'd love to get this in the next release |
The work on the UI side is great and it all seems to work perfectly well. If we wanted to get this into the next release I suggest we might need to add a caveat or notice to the top saying that this is experimental or under active development or something as I've definitely got some issues with the searching side of things. My one main issue is that the results are still rather odd, is a score of 2 too low? Some of the scoring seems a bit random too. For example An example of a good match is
The next valid one is at position 4 - I would suggest putting the score up to 3 which drastically cuts down the number of matches but also cuts of some stuff that seems perfectly valid. Any idea why settings seem to be getting high scores when they don't even seem to have that string in the title or description? |
Yeah, I'd be hesitant to ship this unless it was behind an experimental config setting. I'm quite sure that we'll have a bunch of ideas for fixes once we use this for a while. |
seems like the length of the longest shared substring needs to be prioritized more? another smaller change might be to somehow use that space is used to separate words |
So fair enough, thanks everyone for the feedback, just excited about this one. But I can absolutely put this behind a config for now then and not have it enabled by default. Until then I'll play around more with the search, and already have some ideas around improving it such as maybe using Levenshtein's Distance w/ Word separators, or could try multi indexing on stop characters, or even normalizing stop characters altogether. But will take a look at all that in another PR. Sounds like the best thing to do for now is put this all behind a config and we can continue this features development on the next PR or over Discord, if that sounds good? |
Fine with me yeah, I'm only thinking about fending off issues that we already know about or that people think this is the final iteration of it. |
Alright @Daeraxa @savetheclocktower it's now hidden behind a setting. Within The text of this new setting says: "title": "Enable Experimental Settings Search Feature",
"description": "Will enable or disable the new Experimental Settings Search.", So if this sounds good, what do we think about merging? |
Honestly that sounds good to me, the main question now is how we socialise it, I suspect it deserves its own blog post + announcements so that people can see it and know to enable it. |
I'd be happy to write up a blog post for it! And especially listing it on the changelog might be enough |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 to include this.
[ EDIT: IMO it would be great to see this search feature mentioned in the release text (GitHub Releases page + release blog post if there is one) as a thing folks should try and give us feedback on. ]
I would personally lean toward some text warning this is experimental but enabling it by default. Maybe we include a link to a dedicated issue, where people can post feedback + where we invite feedback on Discord, wherever folks prefer.
Simply because it's useful even if results are casting a real wide net. And because getting it in front of more people is the surest way/might be a prerequisite to get more feedback. And users are not harmed by loose results, especially with the knowledge this is still under active development, other than if we are being protective of first impressions of the feature, I suppose.
So I do lean toward enabled by default with a warning about being experimental being visible somewhere on the search page.
(EDIT: Unless we see dev team members as the primary consumers/audience for feedback at this early stage? Eh, but I think folks who made the jump from Atom to Pulsar is a group biased toward those who are aware of how open-source and volunteer projects work and aren't afraid of a little WIP stuff if it's as feature-complete and usable as this, and so totally optional to use or not use as this is/harmless as this is IMHO. And the intent is clearly positive, even if the results algorithm could be tuned. Hmm.)
But I approve in general and of the particular implementation based on my testing that it essentially worked.
@DeeDeeG I do actually really like this implementation. Obviously as the author I'd love for it to be available by default, but of course still with the option to turn off. If we are okay with having a proper warning at the top of the page should I put that in? And absolutely the dedicated issue would be a great idea to cut down on any issues created about it. Do we think that sounds good? |
Hmm, so there is one failing test at the moment: https://github.com/pulsar-edit/pulsar/actions/runs/4421243758/jobs/7755862129?pr=416#step:9:279
|
Ahhhh I think I remember this one. This test originally passed. I changed it to check the next index of the side panel since we were adding a new panel to the top. But thanks for catching that, but here's the diff. |
Alright with the final choices in to fix some styling and getting everything squared aware lets go ahead and merge this one. Thanks a ton to everyone that has helped out and provided feedback here, I appreciate all of you! |
This PR adds a new side panel to
settings-view
titledSearch
.This panel now allows users to search Pulsar for the setting they are looking for. Without mattering if this is a core setting, editor setting, or a packages setting.
Here is a quick example of the feature in action:
As you've likely noticed the search does not provide the ability to directly change any settings, rather links to the page where they are located. While ideally you'd be able to change settings on this page, or could be linked to their location within the page, I wasn't able to find any simple way of doing so. Such as having the ability to change them on page would need to essentially recreate the majority of the existing
settings-view
functionality.Realistically I think our best approach may be to highlight the setting on the page once the user navigates to it. Which again may not be currently possible but is a good thing to keep in mind. This behavior would be very similar to how Android handles searching in the settings app.
How it Works
This new feature is really powered by the Longest Common Subsequence Algorithm which determines a similarity score between what the user has typed and certain fields of the settings entry.
In doing so, the following fields of a package are considered:
package.json
of the package.After calculating the similarity score of each discrete field using LCS, we then go ahead and award ranking bonus' depending on a few factors, with different factors resulting in a higher ranking bonus.
0.8
awards a0.2
ranking bonus0.5
awards a0.5
ranking bonus.0.8
awards a0.2
ranking bonus.0.8
awards a0.2
ranking bonus.Then finally ranking bonus' are applied to items that have a
1.0
similarity score (The highest score that can be given). Which means whatever the user typed in does exist exactly within the field.0.1
0.1
0.2
0.1
The values here were all chosen off what made the ordering of results 'feel' correct. I know they are rather arbitrary, but could always be tweaked further, or if we really wanted to could be exposed as a configurable setting.
Finally after awarding bonus' each setting entry is given it's total score, which is all previous scores adding together, and we filter the resulting items based on the
searchSettingsMinimumScore
which is a configurable value for end users. With this being a new entry into the settings ofsettings-view
.The Caveats
URI Handling
or otherwisecore.uriHandlerRegistration
cannot be linked to. Ironically enough there is no URI (as far as I can tell) that is able to link to the settings page ofURI Handling
, so if a setting for that item, which the only one there is, is the one listed above, is ever clicked on Pulsar will create a notification informing the user that this page cannot be linked to, and to go ahead and click on "URI Handling" on the settings sidebar.But with all of that said please let me know what you think. I thought this would be a fun new feature to add into Pulsar, and something the Atom creators wanted to do way back when in 2015 but wanted to wait on a total rewrite of settings to do so. So I really do think this could be a bonus for users that may have trouble finding settings.
Especially if they are package settings. Since previously if I wanted to find a packages settings I'd have to navigate to "Settings", then "Packages" then search or scroll until I find my package and click on it.
But now if I know what setting I want to change I can just search here for it and get linked to the right place.
I do feel like it's a good idea to mention real fast, that this new 'Search' page is not the default page of settings, that is still given to 'Core' and has not been changed.
Ideas for Improvement or slightly different Implementations
The two biggest improvements I'd want to see here, if I or anyone else can get to them or wants to talk about them:
The biggest blocker to something like the above, is that as far as I can tell, there's no way to do this via an Atom URI. But who knows maybe we could add support to the URI's being able to link to refs on the page or even some utility function that could find a ref and apply some class to it, to be able to highlight it.
PS. Sorry that my last few PR's have been huge with the longest PR descriptions, I'm just trying to find more to get familiar with within the core of the editor and am halfway writing such long PR descriptions for myself or the next person.