astoff / devdocs.el

Emacs viewer for DevDocs
279 stars 16 forks source link

[Fix #24] Don't treat symbol at point as a default entry #25

Closed vspinu closed 1 year ago

vspinu commented 1 year ago

There are two ways to fix the issue:

  1. Insert symbol-at-point as the initial-input (this PR)
  2. Treat symbol-at-point as the default only when it matches the collection

1 has the disadvantage that the input is inserted by default. Thus the user has to delete the input when she does not want the thing at point.

2 has the disadvantage that symbol at point won't be part of the candidates. Mostly because the "." separator is not normally considered a symbol in most of the programming modes (don't ask me why).

I personally slightly prefer 1, but it's your call of course.

astoff commented 1 year ago

Option 1 does not comply with the completing-read convetions. From the docstring:

If INITIAL-INPUT is non-nil, insert it in the minibuffer initially,
  with point positioned at the end.  If it is (STRING . POSITION), the
  initial input is STRING, but point is placed at _zero-indexed_
  position POSITION in STRING.  (*Note* that this is different from
  ‘read-from-minibuffer’ and related functions, which use one-indexing
  for POSITION.)  This feature is deprecated--it is best to pass nil
  for INITIAL-INPUT and supply the default value DEF instead.  The
  user can yank the default value into the minibuffer easily using
  M-n.

I'm not sure what you mean in option 2. The current behavior is very simple (and useful, I think): M-n always inserts the symbol at point. If the docs don't contain an entry for it, you get 0 candidates, but at least you learned that these docs don't help you there.

Other people already asked for the unsanctified behavior you want. Then I added the optional INITIAL-INPUT argument of devdocs-lookup. So it's at least easy for you do define your own wrapper command.

vspinu commented 1 year ago

Option 1 does not comply with the completing-read convetions.

Indeed. It is actually not quite convenient either.

I'm not sure what you mean in option 2.

The problem with the default is that if the default is not part of the collection you get an entry in the completion which does not point anywhere. In my second first screenshot in #24. If you press RET you get an error. Also when M-n inserts the symbol the docless option is still there and you need to navigate away from it. Which is rather inconvenient.

I have modified the PR to insert the default only when it corresponds to one of the items in the collection. This makes sense especially because require-match is true.

astoff commented 1 year ago

AFAICS your current patch breaks future history, which is a very import feature: If you do M-x devdocs-lookup RET M-n then this should insert the symbol at point. Then you can choose the best matching option. To complicate things, M-x devdocs-lookup RET M-n may leave you with 0, 1 or many options depending on your completion style, and then hitting RET may choose one of them or ask you to refine the input depending on your completion UI.

The only place for improvement I can see here is the handling of null input, i.e. M-x devdocs-lookup RET RET. I agree it's broken right now. But this seems very tricky to me. For example in a Makefile I get the following for wildcard:

Screenshot from 2022-12-15 17-51-49

Now, what do you expect if the point is over wildcard and you type M-x devdocs-lookup RET RET? I can't think of a rule to pick the best candidate that works reasonably in a variety of manual styles and is completion-style agnostic.

astoff commented 1 year ago

I think this is about the best one can do with regards to the null input

modified   devdocs.el
@@ -562,7 +562,10 @@ INITIAL-INPUT is passed to `completing-read'"
          (cand (completing-read prompt coll nil t initial-input
                                 'devdocs-history
                                 (thing-at-point 'symbol))))
-    (devdocs--get-data (car (member cand cands)))))
+    (devdocs--get-data (or (car (member cand cands))
+                           (seq-some (lambda (s) (and (string-search cand s) s))
+                                     cands)
+                           (user-error "No match!")))))

 ;;;###autoload
 (defun devdocs-lookup (&optional ask-docs initial-input)

Or else skip the seq-some and give an error immediately.

vspinu commented 1 year ago

AFAICS your current patch breaks future history,

Not sure I follow. How exactly does this break the history? It's a valid pattern not to supply the default.

The only place for improvement I can see here is the handling of null input, i.e. M-x devdocs-lookup RET RET.

By null input do you mean the default input which is not part of the candidates? With my current patch this is not possible by design.

astoff commented 1 year ago

Not sure I follow. How exactly does this break the history?

To me it's important that M-x devdocs-lookup RET M-n inserts the symbol at point, unconditionally. In your current patch this is not the case, or am reading it wrong?

By null input do you mean the default input which is not part of the candidates?

Sorry I wasn't clear. I mean the case M-x devdocs-lookup RET RET, i.e., the user provides the empty input. It's a quirk of completing-read that this is allowed even if a match is required. So unfortunately this is allowed by design! It's the only case where (car (member cand cands)) can return nil.

vspinu commented 1 year ago

To me it's important that M-x devdocs-lookup RET M-n inserts the symbol at point, unconditionally. In your current patch this is not the case, or am reading it wrong?

Arh, I see what's going on. My problem is really ivy specific. Ivy somehow is smart enough to pick the element at point on M-n but for other completion mechanisms (vanila, vertico, fido) M-n inserts the default value if present.

Also the spurious first element which can be seen in my screenshot is ivy specific. There is surely a good reason why ivy places the default on top but in this case it's simply inconvenient.

So yes. You are completely right in your assessment, the only thing that can be done here is the null treatment.

astoff commented 1 year ago

I'm still interested in your opinion about the "null input" case: both for Ivy and standard completion users.

Is it better to produce an error or is it better do display the first entry that somehow contains the symbol at point in its title? IOW, use

(seq-some (lambda (s) (and (string-search cand s) s))
                                     cands)

The problem of trying to be clever, going back to your example, is that we may be lucky and pick tf.keras.layers.LSTM or we may pick something tangentially related, like tf.raw_ops.LSTMBlockCell. There's no way to be sure...

vspinu commented 1 year ago

My preference is for an explicit error. Figuring out an implicit default entry is crawling on the completion's backend territory. It would mean figuring what is displayed to the user by the specific backend, which I think is not possible.