astoff / devdocs.el

Emacs viewer for DevDocs
297 stars 17 forks source link

Various UX suggestion #10

Closed milouse closed 2 years ago

milouse commented 3 years ago

I’ve some other suggestion to make to improve (IMHO) the day to day use of devdocs.el. This PR can be seen as a debate in first place. However I’d be glad if you like it directly :)

(side note: I’ve sent the copyright document to the FSF, and am waiting for their answer, thus we have time to discuss)

This PR do the following thing:

Improve initial-input support for devdocs-lookup

The current devdocs--read-entry implentation will fail if the return value of the main completing-read call is not a propertized string from devdocs--entries. That means the default value, which was set as (thing-at-point 'symbol) will make the whole function to fail.

This fix does the following things:

Improve devdocs--read-entry behavior

This patch allow a user to confirm a match without having to deal with the completion buffer when an exact match exists, but other candidates are still available.

A user-error is now signaled if the user confirm a non-empty string with no matching entry.

milouse commented 3 years ago

(I remove syntactic changes for now to focus on features)

astoff commented 3 years ago

Admittedly, devdocs-lookup works better with one of the vertical completion systems (Vertico, Ivy, etc.), since those (1) have a "selected candidate" that is chosen by default and (2) allow to easily select candidates after the first one, even if they have the same name. So I'm assuming you want to improve the behavior of devdocs-lookup when using the good old minibuffer completion.

set initial-input to (thing-at-point 'symbol t) when it is nil. That mean the prompt now always show the thing under point. This point may be discussed, I’ve no strong opinion about it.

This is not the standard behavior in Emacs (compare with C-h f), so I will not add it. See also https://github.com/astoff/devdocs.el/pull/6, where an INITIAL-INPUT argument was added for anyone who wants to define their own wrapper function doing that.

If devdocs--read-entry returns nil (which is now possible), do not try to render nil in devdocs-lookup

So it fails silently? Seems potentially confusing!

This patch allow a user to confirm a match without having to deal with the completion buffer when an exact match exists, but other candidates are still available.

How is this useful? I suppose it depends a bit on the way a particular manual is indexed. If there are many entries with the same name but the first one is always the most interesting one, then I see the utility of this behavior. But it is a very specific assumption on the document.


In short, I'm not convinced these changes make sense. An improvement I would be more open to include is a default value similar to describe-function (C-h f). But this seems very hard to get right. Consider this example: if, in a Python buffer, the point is on the symbol open, then the desired manual entry would be open(), while if the point in on the symbol tuple, then the manual entry would be tuple (without parens). Other manuals will have other random convention.

astoff commented 3 years ago

Hey, I didn't mean to discourage you :-) Do you agree it's better to mimic describe-function, and did you give this any more thought as to to guess a reasonable default?

milouse commented 3 years ago

Hi, no worries :) I usually participate by batch to open-source project, and since the last time I did not take time to really come back again on this. I apologize for not having acknowledge your review. I understand your points, but did not take time to see if I agree or not xD I cannot promise anything, but as soon as I can give it some time again, I’ll let you know my feeling and make this PR evolves in one way or another. Thank you very much for your encouragements :)

astoff commented 2 years ago

This patch allow a user to confirm a match without having to deal with the completion buffer when an exact match exists, but other candidates are still available.

I believe this is fixed in the current version of the package, where I got rid of the invisible disambiguation cookies.

The other points about the default choice is still valid, but should maybe be resumed in a fresh issue, so I'll close this PR.