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

Keyword search #110

Merged
merged 13 commits into from
Dec 17, 2020
Merged

Keyword search #110

merged 13 commits into from
Dec 17, 2020

Conversation

InconsolableCellist
Copy link
Contributor

@Jelmerro What do you think? It's parsing the Map every time you visit a URL, though I think the performance impact may be negligible. I didn't know where I could stick and fetch a Map object without just pulling the new String from the settings.

@Jelmerro
Copy link
Owner

Before I go into details about the implementation, I would recommend to check the contribute paragraph of the readme. The most important parts of that paragraph with regards to writing code, are the ones suggesting to use editorconfig and eslint. The eslint style guide clearly wasn't used while working on this, and as such I don't like the way the code is written currently.

I think a better name for this setting would be searchwords, but let me know what you think of this name. Besides that, I think we shouldn't implement this with some kind of JSON, but similar to other list-like settings with a value and a key. Examples of that are "containercolors", "redirects" and "permissionsallowed". They separate individual entries with a , so that you can append and remove entries easily, and each entry uses a ~ to separate the key and value. In your example: searchwords+=yt~https://www.youtube.com/results?search_query=%s.

InconsolableCellist added 3 commits December 15, 2020 17:31
Renamed to 'searchwords'
Uses the string list syntax (key0~value0,key1~value1,...)
Adds checks on settings set()
Passes eslint
@InconsolableCellist
Copy link
Contributor Author

Alright, @Jelmerro, I updated it to use the string list and to pass eslint, so the style now seems consistent. I tried editorconfig and I believe it's passing, as I didn't see any complaints, but I'm not that familiar with that tool. My vim was already configured to indent with 4 spaces and use \LF, however, and there aren't any trailing whitespaces.

@Jelmerro
Copy link
Owner

My comment was mostly about the eslint style guide, I think there were no issues with editorconfig.

This is already much more consistent and it works well. There are just four small things I would like to change:

  • No default entries, this is a breaking change, as it potentially breaks the workflow of people searching for that on DDG. You can just set the default value to an empty string and let people add these searchwords themselves, the current help docs contain enough info for people to figure it out.
  • Can we highlight the navbar differently when a searchword will trigger instead of a regular web search? This way it doesn't come as a surprise when pressing enter. This becomes very important when people add a lot of these I think. You can also add an extra entry to the bullet points for the different categories in :h search. I was thinking of a color like purple or something by default (can of course be changed with colorschemes).
  • The help for searchwords should come right after search, currently it's not. Please move it to the right place in the list of settings, they should be ordered alphabetically. You could also briefly mention that the color will change when a searchword triggers (as noted above).
  • Finally, I am not sure if it makes sense to allow empty searches. Maybe it would be better to only trigger these searchwords if there is actually text behind them, and otherwise just do a regular search of just the word itself. This is another scenario where coloring the navbar for searchwords would help make clear when it activates.

Feel free to discuss these suggestions, this is just what makes sense to me.

@InconsolableCellist
Copy link
Contributor Author

  1. There's a default entry in settings.js in my commits above. Does it have to be somewhere else too? The default is for "yt"
  2. I wasn't able to quickly figure out where I should make those changes. Where is that logic located? Would it change the text color, background color, or the color of the "Explore" word?
  3. Oops, I forgot to reorder it after renaming it. Fixed
  4. This is fixed now too

@Jelmerro
Copy link
Owner

I meant that I would prefer there to be no searchwords by default, not that you need to define them at other places too. It's confusing to suddenly introduce this without making it opt-in. Please just set it to an empty string in the defaultSettings and let users define these themselves.

I was talking about the border color of the navbar. As explained in :h search, the border color changes depending on what will happen when you press enter. It would be great if it would (for example) color purple when these searchwords will trigger. The code for that is here. And this also requires to define the colors in the default colorscheme here and here. If this is too complex, or you don't have time for this, I'll probably merge it anyway and add this myself.

@InconsolableCellist
Copy link
Contributor Author

InconsolableCellist commented Dec 17, 2020

Good point about no keywords by default. I was figuring it'd be instructive to users, but there's really no point in forcing them to have a default, and the help explains how to use it.

I added a new style and set the bar to change colors (currently to #eee) when it detects a searchword.

I couldn't find a great way to avoid duplicating some code, now in isSearchword() in util.js:98 and actions.js:346. The only way I saw to solve it is something like a function that does "replace URL if URL is a Searchword/query pair (URL)", but that'd mean in suggest.js it'd have to check if the input and output to that function differ, and determine if it's a searchword from that. Alternatively, I could add a "isSearchword()" and "getSearchwordURL(URL)", but that would mean some duplicated code in util.js.

Ideally I'd want to return a touple, where the first element is True or False indicating if it's a searchword, and the second element is a key/value pair of the detected searchword and the replaced URL. You can probably do that with arrays in JS, but it just felt like it'd be a big departure from how this codebase does things, and isn't especially intuitive for future coders.

If you think those duplicated lines are a problem, or can think of a more elegant solution, let me know

@Jelmerro
Copy link
Owner

There are some small adjustments I want to make to the help and the appearance in the light colorscheme, but overall this seems ready to merge, thanks a lot for implementing this.

@Jelmerro Jelmerro merged commit 2769e45 into Jelmerro:master Dec 17, 2020
@InconsolableCellist InconsolableCellist deleted the keyword_search branch December 17, 2020 22:28
@InconsolableCellist
Copy link
Contributor Author

Great, thanks!

@InconsolableCellist InconsolableCellist restored the keyword_search branch December 18, 2020 06:34
Jelmerro added a commit that referenced this pull request Jan 5, 2021
* Checking in keyword search feature

* Adding help documentation for keywords feature

* Removing trailing whitespace

* Modifying keywordSearch feature to searchwords, rewriting

Renamed to 'searchwords'
Uses the string list syntax (key0~value0,key1~value1,...)
Adds checks on settings set()
Passes eslint

* Updating help page for searchwords

* Updating help page for searchwords

* Minor verbiage change for searchwords

* Re-ordering the help for searchwords

* Searchwords now doesn't search with an empty query

* Setting searchwords default value to empty string

* Changing the suggest bar when a searchword is entered

Co-authored-by: InconsolableCellist <you@example.com>
Co-authored-by: Jelmer van Arnhem <Jelmerro@users.noreply.github.com>
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.

None yet

2 participants