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

Prompt buffer (minibuffer overhaul) #1157

Closed
wants to merge 324 commits into from
Closed

Prompt buffer (minibuffer overhaul) #1157

wants to merge 324 commits into from

Conversation

Ambrevar
Copy link
Member

@Ambrevar Ambrevar commented Feb 17, 2021

This is a rebase of #1054.

Left to be done:

  • Backport test-fuzzy.lisp to make sure we have at least the same matching
    quality as master.

  • Tune paging. Have C-p recenter on suggestion?

    See if we can keep
    2eeafba ("prompt-buffer.lisp: remove
    element-in-view-port code"), or else revert it.

  • (Optional) make-source should return a lambda that creates
    minibuffer-source objects. Indeed, we don't want global, non-thread-safe
    minibuffer-source object. The source object should be instantiated with the
    minibuffer.

    On second though, do we really need a macro to create top-level sources? What
    about just using define-class? For instance

    (define-class minibuffer-foo-source (minibuffer-source)
      ((initial-suggestions '("foo" "bar"))
       (filter #'slow-identity-match)))
    
    (minibuffer:make
     :sources (list (make-instance 'minibuffer-foo-source)))
  • I'm thinking of renaming initializer and cleanup to constructor and
    destructor respectively. This would be consistent with modes.
    (It's done now.)

    While we are at it, we can make methods for them as suggested in Make mode constructors/destructors composable #1005. We
    could still keep the slots which would allow for instance-specific overrides.

  • Have prompt-buffer inherit from prompter instead of having a
    prompter slot. This would make the Nyxt side of things way less verbose, it
    will also be easier for the end user to invoke the prompt buffer.

  • Rename the prompter:prompter-source class to prompter:source.

  • Contextual help display and discoverable bindings.
    C-h b should display the bindings in a help buffer.

    Add an action which opens a nested prompt buffer with all the key
    bindings. Pressing return executes the bound command in the parent
    prompt buffer.

  • Should we have C-h m to explain how the prompt buffer works?

  • VI bindings

  • Resume previous prompt-buffers.

  • Extend support for input history.

    • set-url2 has it, add support for all commands.
    • Make sure duplicates are not inserted.
  • Highlight portion of matching text.

  • Implement search-buffer2 command using prompt-buffer.

  • [-] Restore multi-buffer search leveraging the new multiple source feature.

    Still need to fix following across buffers.
    
  • Make meta command which allows for selecting sources, then drop into new
    prompt buffer with given sources.

  • Performance: set-url2 takes a while to show up when the history is big.

  • switch-buffer2 loses the prompt focus when "following" the selection.

  • Cosmetic: The top of the prompt input is swallowed.

  • Cosmetic: Option to not display the column names / . For instance in "New
    URL" source.

  • Add with-prompt helper to avoid having to (when result ...) after each
    prompt.
    We could even support chained prompts, like sera:and-let*.

  • Echo message when no source has any suggestion (nor do they allow for raw
    user input) instead of showing a useless prompt-buffer.

    Actually, it may be useful to show an empty prompt buffer to emphasize
    what's happening (echo messages are not always seen), and to display the
    list of sources.
    
  • Add action to sort by column.
    Would be great if we had mouse support, i.e. click on the column name to
    sort by this column in ascending / descending order.

  • Add column filters.

    This one is tricky, because we want to make it both flexible and
    explorable.
    We could add actions, but it can be a bit tedious to go through multiple
    menus just to filter, say, by today's date or the +foo tag.

    One option would be that the action actually inserts a special syntax in
    the prompt which does the filtering.
    This special syntax could be started from (.

    Example:

    ;; first arg is column name
    (filter "URL" (match-host "example.org") (match-port "80"))
    (filter "tags" (match "lisp" "games") (match-not "racket"))

    Could also be cool to enable column and predicate completion.

    When implemented, remove tags.lisp.

  • Shouldn't we avoid to play with suggestion object from the Nyxt side?
    Then we should not have current-suggestion, marked-suggestions, etc.
    Instead we should have functions that return the values.

    Nyxt uses suggestion objects mostly to get the properties.  Can we do
    differently?
    Actually no since each object properties can be used by filters, the user
    must be able to access them.
    
  • Make sure actions support both a single selection and
    marked-suggestions when multi-selection-p is T.

  • Remove object-string, object-display if we don't need it anymore?
    Then remove object-display.lisp.
    What about tab-insert?

  • Remove fuzzy.lisp when done.

  • Remove must-match-p.

  • Use yes-no-source in if-confirm.

  • Suggestion properties should be an alist, so that properties can have
    arbitrary names and casing.
    I suggest un-dotted alists since they are shorter, more readable, more
    beginner-friendly.

    So replace `(:name "foo" :desc "bar")` by 
    `(("Name" "foo") ("My description" "bar"))`.
    
  • Rename properties to attributes to avoid confusion with symbol
    properties and "property lists" (plist).

  • Don't expose suggestion-maker, revert to suggestion-filter-funtion and
    rename it attribute-function.

  • Support multiple persistent actions, and add action to choose which one to
    enable.
    See also Multiple persistent actions emacs-helm/helm#1926.

  • 3 types of functions can be run from the prompt buffer:

    • actions
    • persistent actions
    • commands (bound to keys)

    Can we conflate these into just 2 types, e.g. should all commands be actions?
    If so, we need to away to set a bunch of default actions with their keymap.
    Inheritance? Store actions in prompter?

    But isn't the job of the prompt-buffer-mode to deal with keymaps? It would
    be more consistent for the user.

    We can list actions and the previous point will allow us to list persistent
    commands.
    We need a way to list commands. C-h b mentioned above?

    Add command to list all functions? Multi-source?

  • Style: Refactor update-suggestion-html and a few other functions in prompt-buffer.lisp.

@Ambrevar
Copy link
Member Author

I've just pushed support for async initial suggestions. This is great since this means that the prompt buffer will always pop up instantaneously even when sources take a while to display their initial suggestions. No more hangs!

@Ambrevar
Copy link
Member Author

You can now resume prompts! Try it out with the resume-prompt command! It's awesome!

@Ambrevar
Copy link
Member Author

Prompt buffer history is in, try it out with M-h.

Search is also tested and working: see search-buffer2 and search-buffers2.
Still needs some fine-tuning, but overall the prompter architecture seems
flexible enough to accommodate this! :)

@Ambrevar
Copy link
Member Author

@jmercouris: Last commit should be good to do! There are a few nits to fix here
and there, but overall the API should be relatively stable from now on.

You can start converting all the minibuffers to prompt-buffer! :)
One exception: bookmark filters. We still need to work on a generalized column
filtering syntax as described above.

Before getting started, have a look at the commits from last week until today
and let me know if you have any feedback.

@@ -46,7 +46,7 @@ Suitable as a `source' `suggestion-property-function'."
(t (list :default (write-to-string object)))))

(define-class suggestion ()
((value nil
((value nil ; TODO: Rename `data' as with the GHT? Maybe confusing since we have `match-data'.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I would also prefer data

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So any better naming suggestion for match-data then?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

matching-data?

@Ambrevar
Copy link
Member Author

Ambrevar commented Feb 23, 2021 via email

@Ambrevar
Copy link
Member Author

Shall we have the :sources initargs accept both a list and a source object?
I'd say yes, since-source prompts are only too common...

@jmercouris
Copy link
Member

Yes, it should accept either a list, or a single source, that would simplify a lot of code.

@Ambrevar
Copy link
Member Author

Ambrevar commented Feb 24, 2021 via email

source/window.lisp Outdated Show resolved Hide resolved
@jmercouris
Copy link
Member

jmercouris commented Feb 25, 2021

Do we want the :prompt argument to prompt to automatically append a colon if it is not present?

E.g.

(prompt
                  :prompt "Ropen buffer(s):"
                  :sources (list (make-instance 'recent-buffer-source)))

AND

(prompt
                  :prompt "Ropen buffer(s)"
                  :sources (list (make-instance 'recent-buffer-source)))

would be both acceptable?

@Ambrevar
Copy link
Member Author

Ambrevar commented Feb 25, 2021 via email

@Ambrevar
Copy link
Member Author

Ambrevar commented Feb 26, 2021

Any particular reason why you merged master?
I find that it makes it a bit difficult to follow the history now, I'd rather keep the history linear on this branch. What do you think?

Can we revert this change and, if need be, rebase against master instead?

@jmercouris
Copy link
Member

I merged master because I didn't want to rebase on you in case you were doing something. Sorry if that made things confusing! Why I merged master: I wanted to update password.lisp and didn't want to work with the old-code!

@jmercouris
Copy link
Member

How can I (prompt ...) without any sources just to collect raw input from the user? Is this possible?

I'm trying to replace calls like this:

(prompt-minibuffer
         :input-prompt argument)

@jmercouris
Copy link
Member

Not sure how to undo my merge. I tried to revert the merge commit, and then it gives me a prompt "Replay merges relative to parent?". Apparently this is expecting some sort of integer...

@jmercouris
Copy link
Member

I think I've got it figured out.

@jmercouris
Copy link
Member

Here is what I ended up doing:

  1. make a new branch from prompt-buffer (titled: prompt-buffer-changes) just in case
  2. revert the branch prompt-buffer to before the merge-commit
  3. cherry pick my changes from prompt-buffer-changes to prompt-buffer
  4. rebase master onto prompt-buffer
  5. force push

please let me know if you see anything problematic with this approach.

@Ambrevar
Copy link
Member Author

To undo the merge, it's easier to

  • backup the branch
  • reset to the commit before the merge.
  • rebase against master.
  • cherry-pick the commits you added after the merge.

You are right that you should not rebase a public branch. But if you let me know in advance and I confirm, then you can go ahead.
Otherwise, just use the "old code" in password.lisp: the change is trivial and rebasing will be trivial as well when we are done with this branch.

@jmercouris
Copy link
Member

Aha you pretty much just wrote exactly what I did :-D

@Ambrevar
Copy link
Member Author

Ambrevar commented Feb 26, 2021 via email

@jmercouris
Copy link
Member

John Mercouris notifications@github.com writes:
How can I (prompt ...) without any sources just to collect raw input from the user? Is this possible? I'm trying to replace calls like this: (prompt-minibuffer :input-prompt argument)
Have a look at set-url2, it has a New URL source. I think we should have a prompter:query-input-source just like we have a prompter:yes-or-no-source, since this is probably going to be a common use case. Thoughts?

That's more or less what I ended up doing for now. I guess we could call it "raw-input-source" or something.

@jmercouris
Copy link
Member

I don't understand why using prompt in command-commands.lisp fails within execute-extended-command. Can you please try it and see if you can see what is happening?

@Ambrevar
Copy link
Member Author

Ambrevar commented Mar 2, 2021 via email

@jmercouris
Copy link
Member

Ready to merge :-)

@Ambrevar
Copy link
Member Author

Ambrevar commented Mar 16, 2021 via email

@Ambrevar
Copy link
Member Author

Merged!

There are important issues before we can release a new pre-release though. I'll open a new issue about it.
I'll also open another issue about what we've discussed here that hasn't been implemented yet.

@Ambrevar Ambrevar closed this Mar 17, 2021
@Ambrevar
Copy link
Member Author

See #1212.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants