Add description to search engines (fixing #1144) #1169
Conversation
|
Hey, @mijoharas. You know the tests are failing on this, yes? Shouldn't be too hard to fix up. |
|
alright, tests are fixed for me. |
| suggestion = new Suggestion(queryTerms, "search", searchEngineMatch, queryTerms[0] + ": " + queryTerms[1..].join(" "), @computeRelevancy) | ||
| searchEngineMatch[0] = searchEngineMatch[0].replace(/%s/g, queryTerms[1..].join(" ")) | ||
| suggestion = new Suggestion(queryTerms, "search", searchEngineMatch[0], | ||
| searchEngineMatch[1] + ": " + queryTerms[1..].join(" "), @computeRelevancy) |
There was a problem hiding this comment.
Looks like we're doing queryTerms[1..].join(" ") twice; here, and in the line two above. Surely that's not necessary?
And, if you refactor it, can you fix the indentation on this line?
There was a problem hiding this comment.
Hi, sure, I could factor the queryTerms[1..].join(" ") out onto the previous line, I just wrote it quickly.
Could you tell me what you mean about the indentation? if it's the line starting searchEngineMatch[1], that is an argument to suggestion, I could obviously change the indentation if you think that'd be better (indent it further) but I just left it where it was. (would you indent it further?
|
Edit: That's all bogus. |
|
Ah. I think I get what you're doing now. But what if I start with: then decide I want to temporarily disable the Now the |
|
Couldn't this ambiguity be resolved by adding something like #1154? |
|
Yes, @mrmr1993. Something like this might work:
What do you think, @mijoharas? |
|
Not sure if they were ever intended as possibilities, but this suggestion breaks some
|
|
This started (over in #1009) as a vimium equivalent to chrome's search-engine support. Probably best to keep it simple, clear and clean, for now. |
You're right, that case would have should be fine (bar will not have a comment). That is unless I messed something up... |
Something similar is supported: will have I decided to use both as a comment syntax because I don't particularly like making the comments at the end of the lines as it makes my file with all of my searchengines in it difficult to parse. I personally prefer the first (comment on immediately previous line) syntax which is why I made it the main comment-description syntax |
That is a good point, I hadn't used the " comment syntax myself (which makes obvious sense given this is vimium) so hadn't included it. I'd be happy to make the change. If you aren't a fan of the comments-as-descriptions features, we could always make an alternative markup (e.g. single |
|
My sense, @mijoharas, is to keep it simple and mirror what chrome does. I suggest we go with: being two comments, a search engine without a description, and a search engine with a description. So no special meaning to an extra newline, and very little extra syntax, and comments are treated the same as they are elsewhere. |
|
As pointed out by @mrmr1993 this suggestion breaks several different possible searches:
We support all of these currently, and breaking on space would stop them working (at least we support top and bottom, not sure about quickedit but I have no reason to think it shouldn't work). If you want to have an inline search engine with description would the current inline syntax: be ok? this way we break on |
|
I've had a think, could we not just take a double comment symbol (followed by a space?) to be the start of a description? Then we'd have giving no description to giving the description Similarly, if we use would only give a description to In any case, I much prefer preceding line comments to inline comments for this, since I feel is vastly clearer on a glance than or any variant (especially if the line wraps in the options page text box, as this example does). |
| else | ||
| split_pair = pair.split(/: (.+)/, 2) | ||
| command_name = split_pair[0] | ||
| inline_comment = split_pair[1].split(/\s#/) |
There was a problem hiding this comment.
If a line doesn't contain the string :, won't this error out (since split_pair[1] === undefined, so we're calling undefined.split)?
There was a problem hiding this comment.
reading it now it does look that way... I'm sure I've tested it with blank lines e.t.c. (as they're in my own search-engines page which I checked to ensure there weren't any regressions) but that may have been before I changed something else.
I'll take another pass at cleaning this up when I get a chance.
Out of interest, if we go with the double-comment-space idea that you suggested, would that be in addition to the inline comment or instead of? (currently it supports two syntaxes, comment on line before and inline comment, with the first taking priority over the second)
There was a problem hiding this comment.
Assuming that @smblott-github agrees that using ## and "" for description lines is ok, I would say just drop the inline comment version (and inline comments altogether):
- It breaks any search engine whose definition inadvertently uses
#somewhere. - It's confusing to have multiple ways to do the same thing (and to remember the order of precedence).
- I genuinely can't think of a situation (for this form field!) where an inline comment would have any advantage over a comment on one of the surrounding lines.
- It makes the code simpler.
Did you have any particular reason for wanting to keep it?
|
@mrmr1993 I like your idea of double comment space for description. I'll implement it that way when I get some time (provided other people agree that it's a good way to go?). |
|
What we're doing here is essentially inventing a language for describing search engines. In vim, there's a real language with a parser, data types, an execution engine and so on. In vimium, we don't have all of that. We just map the text to some JavaScript data structure (with no feedback in the case of errors) and hope for the best. Whatever "language" we come up with, it must be easy for users to understand (without reference to a wiki page), and not likely to confuse or be the source of errors. I'm concerned that Perhaps it's time to accept that we're trying to push too much complexity into a plain old text box. Instead, we could bite the bullet and implement the search-engine setting as an HTML table, with each row having three fields. It would look very-much like the equivalent chrome setting (and the new exclusion-rule setting). |
|
@smblott-github you're right that that's an option, but personally I'm opposed to it. Firstly the reason I'm opposed is that I can copy/paste lots of search engines at once, if someone else has a bunch of handy search engines that are useful I can copy all of his. One of the things I like about vim and emacs (and sublime text with it's json config files) is that I can see parts of other peoples configs that I like and take those as a whole. I much prefer that to something like visual studio where I have to enter things in gui config screens line by line or the chrome search engines input. Secondly, I imagine the primary users of vimium are developers, who firstly won't find the markup difficult to understand (simpler even than markdown which github/stackoverflow use, only two types of markup) and will appreciate the flexibility of config files. In addition, since it's a config file I can trivially write an extension to vim/emacs so I can use my search-engines config file for that and search This is just one datapoint but I would definitely like easily copyable flat config files (partially because I want to write an extension for emacs/vim to open chrome pages from my editor and wanted to use the same config file). Let me know what you guys think. |
|
Again, this is a personal preference, and I'll agree with whatever you guys go with in the end but I do prefer flat text (also it's more vim-ish 😄 ) |
|
👍 for text configs. If we add more text-based fields, we could embed a parser framework in Vimium and just store parse rules for each, which should actually our code's reduce complexity. |
|
Hi @mijoharas. It would be great to make progress on this with a next release on the horizon. We really need something which is easy to use and hard to break. I think that rules out definitions spanning multiple lines, or giving the comment marker Suggestions:
While it might be nice to have, support for exotic mappings (such as |
|
@smblott-github the structured user interfaces are a pain compared to plain text. Not being able to comment out, comment about, group and reorder lines is a big feature loss, which makes it much harder to come back to the setting after some time and change/manage them. I still really cannot see the issue with
@philc do you have any thoughts on this? |
|
I'd like to point out that this breaks existing functionality, examples that I personally use are: With the first example you can just type down into the vomnibar which will then add the root of the current url to downforeveryoneorjustme.com to tell you whether the url is down. note that the second example is already escaped but could be unescaped with: this allows you to use google to search whatever site you are on for whatever your query is (e.g. on this page type These examples are currently supported and with this we can recreate lots of bookmarklets and add them to the vomnibar under our searchengine use case, I find this powerful and extensible and wouldn't want to lose that. |
|
The feature we're discussing is search engine support, not execute arbitrary JavaScript here. We should not compromise the common case in order to accommodate the exotic. We can already do site specific searches: If you substituted So, with descriptions, it would look something like: This doesn't capture all of the examples in this thread. But it captures a fair few. And, it doesn't compromise the common case. (Can execute arbitrary JavaScript here not be accommodated by simply defining your own bookmarklet?) |
|
Great discussion guys, but I think we should take the simplest version and
|
|
You could write your own bookmarklet. But what we are writing here is the vomnibar. It is intended to emulate extend and improve upon the omnibar. I wrote the search engine feature because I didn't want to type All of the "execute arbitrary javascript" examples that you posted work perfectly well in the omnibar and work perfectly well right now in the vomnibar. I don't see why we would reduce the available features and introduce a new syntax I use chrome and vimium for their flexibility, I don't want to constrain what I can do. I don't often use javascript but it is supported in the omnibar, it is supported in the chrome search engine features, it is supported in the vomnibar and I don't want to do anything to break it. |
|
also @philc, what do you think's simple. obviously the simplest is to not ship the feature, if that's so then we shipped it last release what do you think? |
. Otherwise personally I think the simplest is
|
In most programming languages (and vimscript), both of those lines would be On Mon, Nov 3, 2014 at 11:46 PM, Michael Hauser-Raspe <
|
|
Hmmm... I understand the point and it does make sense to go with a "principle of least surprise". My main reservation is that we would be reducing the feature set and introducing breaking changes (and the worst kind of breaking changes, those that affect me! 😄 ). e.g. the javascript bookmarklets above, which work in chrome search, work in the omnibar and vomnibar but this change would break in the vomnibar. Would you consider something like docstrings, e.g: obviously, there are alternate docstring syntaxes, too. Again, I just really don't want to break (my and possibly other peoples) code. Otherwise I suppose that descriptions aren't really too important and don't really need to be added. One other point is that if you are worried about code complexity (as that code I checked in does look a bit on the ugly side) I can assure you that's just from trying to support both inline and line-before comments at once (and admittedly writing it somewhat unclearly). Changing to this syntax will give clear intuitive code. Again, I am sorry for being contrary and you do make good points, but I don't want to break things for a feature that I don't think is too important (the descriptions). |
|
Closed in light of #1389. |
Adding description to search engines to fix #1144