emacs-helm / helm-dictionary

Helm source for looking up dictionaries
31 stars 12 forks source link

Looking up words online #4

Closed michael-heerdegen closed 10 years ago

michael-heerdegen commented 10 years ago

Hi,

I think it woud be cool when you were able to browse online dictionaries on the fly from the minibuffer when using helm-dictionary. This could be achieved easily by providing a dummy source that doesn't do anything unless you expicitly consult it.

I could prepair a pull request if you are interrested.

Regards,

Michael.

tmalsburg commented 10 years ago

Being able to query online dictionaries would certainly be nice. But what do you mean with on the fly from the minibuffer? And what is a dummy source that doesn't do anything unless I consult it (isn't that true for all sources)? Could you please elaborate a bit?

michael-heerdegen commented 10 years ago

Being able to query online dictionaries would certainly be a nice feature. But what do you mean with on the fly from the minibuffer?

I meant, from one and the same helm session, without the need to start a new command.

And what is a dummy source that doesn't do anything unless I consult it (isn't that true for all sources)? Could you please elaborate a bit?

A dummy source in Helm is, I think, a source that doesn't perform matching and has no candidates. helm-source-buffer-not-found is such a source. In our case, my source for online look up doesn't do anything unless you select an action, which will look up your input online.

Ok, just wanted to see if you are interested. I'll make a pull request, then you can try it yourself.

michael-heerdegen commented 10 years ago

I have created a new branch "online-lookup" for demonstration. Please try and tell me what you think.

Hit TAB to see the available online dictionaries. They are consulted with the current input. RET consults any online dict and exits the helm session, while C-z keeps it alive.

tmalsburg commented 10 years ago

Neat, I like it. Here are some comments:

1.) Helm first shows "Lookup Online" and below it "Search dictionary". The other direction would be better because the online search is a fall-back when there are no offline results.

2.) An alternative to showing just one dummy entry would be to show one entry for each online resource. This may not be idiomatic but it saves keystrokes. What do you think?

3.) Instead of providing a custom function for browsing URLs, I would prefer to rely on whatever the user defined as his default browser (browse-url-default-browser).

4.) Just out of curiosity: Is there a reason why you have a function that generates the dummy source instead of a defvar as it's commonly done?

michael-heerdegen commented 10 years ago

Titus von der Malsburg notifications@github.com writes:

Neat, I like it. Here are some comments:

1.) Helm first shows "Lookup Online" and below it "Search dictionary". The other direction would be better because the online search is a fall-back when there are no offline results.

I foresaw that you would say that ;-) I agree, but helm doesn't respect the order of the given :sources in our case. I'll see what I can do.

2.) An alternative to showing just one dummy entry would be to show one entry for each online resource. This may not be idiomatic but it saves keystrokes. What do you think?

I think that it's a good idea!

3.) Instead of providing a custom function for browsing URLs, I would prefer to rely on whatever the user defined as his default browser (browse-url-default-browser).

Here I'm not sure. I don't have set this variable at all - it is undefined, even after using w3m or eww.

Eh, one moment - browse-url-default-browser' is a function. Do you meanbrowse-url-browser-function'? This will by default call a browser outside of Emacs, like firefox. I don't think this would be a good default behavior. It is slow and distracting. You can't copy and paste from there with Emacs commands. I think it would be totally legitimate as a valid value of that option, but I'm not sure if it should be the default. Your system browser may need to be started first, or is on another workspace etc.

4.) Just out of curiosity: Is there a reason why you have a function that generates the dummy source instead of a defvar as it's commonly done?

The only reason is that this way I can respect the current value of helm-dictionary-online-dicts. If the source would be a constant and the user changes `helm-dictionary-online-dicts' after loading the package, it would have an effect.

If we implement point 2., we will have to create several sources on the fly, one for each entry of `helm-dictionary-online-dicts'. I don't think it's necessary to let the user define these dummy sources.

Regards,

Michael.

tmalsburg commented 10 years ago

I foresaw that you would say that ;-) I agree, but helm doesn't respect the order of the given :sources in our case.

So how does Helm sort the sources?

Eh, one moment - browse-url-default-browser' is a function. Do you meanbrowse-url-browser-function'?

Oh right, I mixed those up. I was basically suggesting to use browse-url to open the url. The user can set this up to use an external browser but using eww-browse-url works just as well:

(setq browse-url-browser-function 'eww-browse-url)

The thing is that how urls are browsed is really orthogonal to the question of what should be browsed. Therefore, I think the how should be handled separately. If it simply comes to down to redefining browse-url-browser-function, I think we wouldn't be asking too much from the user. Apart from these considerations, I'm not sure if I will be using a browser in emacs. On my machine eww is slow and freezes emacs during IO.

The only reason is that this way I can respect the current value of helm-dictionary-online-dicts.

Oh right, makes sense.

If we implement point 2., we will have to create several sources on the fly, one for each entry of `helm-dictionary-online-dicts'.

I was thinking of one source that has one dummy candidate for each remote dictionary:

Lookup online:
Search "test" in dict.leo.org. 
Search "test" in en.wiktionary.org.
... 
michael-heerdegen commented 10 years ago

Titus von der Malsburg notifications@github.com writes:

So how does Helm sort the sources?

Good question. I raised an issue at the helm site.

Eh, one moment - browse-url-default-browser' is a function. Do
you meanbrowse-url-browser-function'?

Oh right, I mixed those up. I was basically suggesting to use browse-urlto open the url. The user can set this up to use an external browser but usingeww-browse-url` works just as well:

(setq browse-url-browser-function 'eww-browse-url)

The thing is that how urls are browsed is really orthogonal to the question what should be browsed. Therefore, I think the how should be handled separately.

Yes, I agree.

If it just comes to down to redefining browse-url-browser-function I think we wouldn't be asking too much from the user.

In any case, we should provide several working predefined alternatives.

If we implement point 2., we will have to create several sources
on the fly, one for each entry of `helm-dictionary-online-dicts'.

I was thinking of one source that had one dummy candidate for each remote dictionary:

Lookup online: Search "test" in dict.leo.org. Search "test" in en.wiktionary.org.

Oh, that makes sense. I already uploaded the implementation of the multiple sources approach - I don't like it much. I like your idea more.

michael-heerdegen commented 10 years ago

I uploaded the variant with dummy candidates. Please give it a try. The order problem remains.

tmalsburg commented 10 years ago

Well, I like it. We're abusing helm a bit because we list actions not candidates to perform actions on. But I can live with it. Can I make changes to your branch? I'd like to simplify your code a bit.

michael-heerdegen commented 10 years ago

Well, I like it. We're abusing helm a bit because we list actions not candidates to perform actions on. But I can live with it.

I can too, since it increases usability.

Can I make changes to your branch? I'd like to simplify your code a bit.

Of course, just do what you think is right ;-)

tmalsburg commented 10 years ago

Ok, I switched to browse-url and added documentation. In the documentation, I also explain how eww can be used to view online dictionaries. Looks good?

Now, we only have to figure out how to get the sorting of the sources right.

michael-heerdegen commented 10 years ago

Titus von der Malsburg notifications@github.com writes:

Ok, I switched to browse-url and added documentation. In the documentation, I also explain how eww can be used to view online dictionaries. Looks good?

Everything looks very good!

One thing: Some users might prefer to use a browser function different from browse-url-browser-function', e.g. I. It is appropriate to use browse-url-browser-function's value as default, but the user should not be forced to either change `browse-url-browser-function' just for this package, or to use defadvice.

Here's my proposal: Introduce a new defcustom helm-dictionary-browser-function'. Valid values are nil and every suitable value ofbrowse-url-browser-function'. The default will be nil, which just means "Use the value of `browse-url-browser-function'".

All we would need to do is to use

(action
 . (lambda (cand)
     (let ((browse-url-browser-function
        (or helm-dictionary-browser-function
        browse-url-browser-function)))
       (browse-url (format cand
               (url-hexify-string helm-pattern))))))

WDYT? It's just a few more lines, and would make users like me very happy :-)

Now, we only have to figure out how to get the sorting of the sources right.

Probably there's no canonical way to achieve this, at least currently. Let's see if Thierry can suggest a solution.

Regards,

Michael.

tmalsburg commented 10 years ago

Just made a commit (41f763005ea4eff5220de7f41eecf11ace18b548) along the lines of your proposal. However, the default of the new customization variable is browse-url-default-browser instead of nil. If the default is to use browse-url-default-browser that should be reflected in the value of the variable.

michael-heerdegen commented 10 years ago

Thanks. But please see my notes on the commit. I think you misunderstood what I meant: I wanted the default to be the users setting of `browse-url-browser-function', not 'browse-url-default-browser.

tmalsburg commented 10 years ago

Just pushed a commit with the nil-solution. Hopefully, the issue is solved now. :)

michael-heerdegen commented 10 years ago

Titus von der Malsburg notifications@github.com writes:

Just pushed a commit with the nil-solution. Hopefully, the issue is solved now. :)

Thanks, looks ok. BTW, shouldn't we use const' for the nil choice instead offunction-item'?

I think we can also add w3m as a choice (it's missing in the outdated declaration of browse-url-browser-function'). The according function isw3m-browse-url', I think. I've tested it, it seems to work well.

tmalsburg commented 10 years ago

shouldn't we use const' for the nil choice instead offunction-item'?

Right, thanks for catching that.

`w3m-browse-url'

This function is currently not part of Emacs but provided by a third-party package (emacs-w3m). Does it make sense to offer it as an option under these circumstances? Adding emacs-w3m as a dependency for helm-dictionary seems like overkill.

thierryvolpiatto commented 10 years ago

Titus von der Malsburg notifications@github.com writes:

This function is currently not part of Emacs but provided by a third-party package (emacs-w3m). Does it make sense to offer it as an option under these circumstances? Adding emacs-w3m as a dependency for helm-dictionary seems like overkill.

Have a look at `helm-browse-url'.

Thierry Get my Gnupg key: gpg --keyserver pgp.mit.edu --recv-keys 59F29997

tmalsburg commented 10 years ago

Thanks Thierry. I'm not sure I understand this: helm-browse-url-default-browser seems to replicate what browse-url-default-browser does, except that the various browsers are given a different priority. Effectively, helm-browse-url does more or less the same as browse-url. What's its purpose then?

michael-heerdegen commented 10 years ago

Titus von der Malsburg notifications@github.com writes:

shouldn't we use const' for the nil choice instead
offunction-item'?

Right, thanks for catching that.

`w3m-browse-url'

This function is currently not part of Emacs but provided by a third-party package (emacs-w3m). Does it make sense to offer it as an option under these circumstances?

Yes, it's the most widely used browser that runs inside Emacs. Many people use it. Also `browse-url-w3' isn't part of Emacs, btw (as well as firefox, galeon and the such aren't).

Adding emacs-w3m as a dependency for helm-dictionary seems like overkill.

We don't add any dependency, we just offer the possibility to use it when people explicitly want to use it.

Why should we offer possibly not installed system browsers, but disallow possibly not installed Emacs based browsers, in an Emacs package?

I think it's totally justified -- no (require ...) of anything needed. People wanting to use it have to take care of installing, setting and loading it properly.

michael-heerdegen commented 10 years ago

Titus von der Malsburg notifications@github.com writes:

What's its purpose then?

Maybe searching actively for an installed browser on any system, in a configurable order?

Titus, should we make it available as a choice (why not?)?

Anyway, thanks Thierry for mentioning this.

tmalsburg commented 10 years ago

notifications@github.com writes:

Yes, it's the most widely used browser that runs inside Emacs. Many people use it. Also `browse-url-w3' isn't part of Emacs, btw (as well as firefox, galeon and the such aren't).

Well, these functions are part of Emacs even if the underlying browsers are not. If firefox is not installed, browse-url-firefox produces a somewhat helpful error message. Compared to that, we just get a "Symbol's definition is void.", if emacs-w3m is not installed.

Anyway, I added emacs-w3m to the list.

Why should we offer possibly not installed system browsers, but disallow possibly not installed Emacs based browsers, in an Emacs package?

It wouldn't be disallowed because any arbitrary function is allowed as a value for the customization variable, not just the listed functions.

Titus

Please note my new email address: malsburg@posteo.de

My Gmail address will soon be deactivated.

Titus von der Malsburg DFG Research Group 868: Mind and Brain Dynamics Dept. of Linguistics, University of Potsdam http://www.ling.uni-potsdam.de/~malsburg/

tmalsburg commented 10 years ago

Maybe searching actively for an installed browser on any system, in a configurable order?

browse-url-default-browser also searches and the order of browsers is not configurable in helm as far as I can see.

Titus, should we make it available as a choice (why not?)?

Sure, I will add it. Ignoring helm's facilities for opening urls certainly doesn't make sense.

michael-heerdegen commented 10 years ago

Titus von der Malsburg notifications@github.com writes:

Maybe searching actively for an installed browser on any system, in a configurable order?

browse-url-default-browser also searches and the order of browsers is not configurable in helm as far as I can see.

I mean, you can configure `helm-browse-url-default-browser-alist' and set your priorities. Your setting will then DTRT on different systems.

Titus, should we make it available as a choice (why not?)?

Sure, I will add it. Ignoring helm's facilities for openen urls certainly doesn't make sense.

Thanks.

michael-heerdegen commented 10 years ago

Titus von der Malsburg notifications@github.com writes:

Compared to that, we just get a "Symbol's definition is void.", if emacs-w3m is not installed.

IMHO not a problem, since the user has to choose it explicitly. And the error message is even helpful.

Anyway, I added emacs-w3m to the list.

Thanks.

Why should we offer possibly not installed system browsers, but disallow possibly not installed Emacs based browsers, in an Emacs package?

It's wouldn't be disallowed because any arbitrary function is allowed as a value for the customization variable, not just the listed functions.

Of course, but this way the users see which settings are principally available. And they don't have to look up the name of the function. This is what defcustom is for.

tmalsburg commented 10 years ago

Well, I think this branch is ready to be merged. I really like this feature and already use it a lot. Michael, please feel free to add yourself to the author list. I'll wait for your confirmation before merging. Thanks for your contributions.

michael-heerdegen commented 10 years ago

Yes, I too think it's ready for merging. Please just do it. Feel free to mention my contributions in the header if you want, thanks.

After the merge, I would like to change the order of the custom items of `helm-dictionary-browser-function'. It is strange that eww is meantioned after Netscape...

tmalsburg commented 10 years ago

Merged. Thanks again.

michael-heerdegen commented 10 years ago

Can we delete the online-lookup branch?

tmalsburg commented 10 years ago

Whoops, I deleted it in my local repo and thought that would propagate. Just deleted it.

michael-heerdegen commented 10 years ago

Works for me in Magit. Branch manager, and hit k on the branch of the remote.