xmonad / xmonad-contrib

Contributed modules for xmonad
https://xmonad.org
BSD 3-Clause "New" or "Revised" License
589 stars 274 forks source link

Unclear semantics for Prompt completions #610

Open solomon-b opened 3 years ago

solomon-b commented 3 years ago

Problem Description

I'm pretty motivated to put in some work on XMonad.Prompt. There is a lot of potential improvements around navigation and scrolling through commands. However, the semantics of the completion system are very unclear and possibly buggy. Is there anyone here who can give me clear explanation of the expected behavior of the completions system?

I'm going to give some examples of weird behavior of completions. I'm hoping we can determine which of these behaviors are intended, which are bugs, and finally define the semantics for completions. If we can do that then I would like to refactor this module a bit in anticipation of doing some feature work to extend the Prompt interface.

Simple Completions (alwaysHighlight == False)

I am using an XMonad prompt because they are very simple and use the default definitions of XPrompt.

Given the prompt:

systemCtlPrompt :: X ()
systemCtlPrompt =
  let commands =
        [ ("foo", pure ())
        , ("bar", pure ())
        , ("baz", pure ())
        , ("qux", pure ())
        ]
  in xmonadPromptCT "TEST!!" commands promptConfig

Tab completion works as I would expect. If I have entered nothing into the command line then pressing tab cycles between all the completion list options. If I enter b and press tab, it cycles between bar and baz.

Now if I change the prompt to use two words per completion:

systemCtlPrompt :: X ()
systemCtlPrompt =
  let commands =
        [ ("1 foo", pure ())
        , ("2 bar", pure ())
        , ("3 baz", pure ())
        , ("4 qux", pure ())
        ]
  in xmonadPromptCT "TEST!!" commands promptConfig

Now I get what I would consider a bug. The first tab sets the prompt to 1 foo and subsequent tabs prepend the command line with more 1s.

This behavior is actually no surprise considering the default implementation of nextCompletion:

-- | Given the prompt type, the command line and the completion list,
-- return the next completion in the list for the last word of the
-- command line. This is the default 'nextCompletion' implementation.
getNextOfLastWord :: XPrompt t => t -> String -> [String] -> String
getNextOfLastWord t c l = skipLastWord c ++ completionToCommand t (l !! ni)
    where ni = case commandToComplete t c `elemIndex` map (completionToCommand t) l of
                 Just i -> if i >= length l - 1 then 0 else i + 1
                 Nothing -> 0

Given the default XPrompt definitions, what happens here is that we lookup the last word of the command line in the completion list--that word would be foo in our case--which will never be found thus setting nl = 0. Then the command line minus the last word is prepended to the entire completion string at index 0 and returned.

Based on the haddock description of this function, I believe what it should be doing is filtering the completion list for entries which contain the last word of the command line. I have no idea why this is a useful behavior for tab completion but that is what the haddock sort of describes.

highlight completions (alwaysHighlight == True)

The hlComplete code is a lot more confusing. Not so much to follow the subroutines, but to understand the desired behavior. The user experience of of high completions feels pretty similar to simple completions modulo edge cases.

Using the same prompt from earlier:

systemCtlPrompt :: X ()
systemCtlPrompt =
  let commands =
        [ ("1 foo", pure ())
        , ("2 bar", pure ())
        , ("3 baz", pure ())
        , ("4 qux", pure ())
        ]
  in xmonadPromptCT "TEST!!" commands promptConfig

The tab key with no command input cycles through all the available completions. If I type b then tab cycles between 2 bar and 3 baz.

It gets weird if you enter a multiword command. For example, if you type "foo " (that is with a space after foo) then you get the entire completion list. I would expect this to either yield no completions (there is no completion with "foo " as a substring) or it would yield the 1 foo completion (by stripping the trailng space from the "search").

If after typing "foo "you then press tab, the highlight stays on 1 foo, the command gets replaced with foo 1 foo, and the completion list is still unfiltered. Subsequent pressed of tab cycle the highlighted completion and change the command to that completion prepended with "foo ", until you cycle back to the first completion and the prepended foo goes away.

If we use another prompt example I can show another strange behavior:

scrotPrompt :: X ()
scrotPrompt = xmonadPromptCT "Screenshot Options" commands promptConfig
  where
    commands = [ ("1: Capture Screen", spawn "scrot")
               , ("2: Capture Selection", spawn "scrot -s")
               , ("3: Capture All Screens", spawn "scrot -m")
               , ("4: Capture with 3 second countdown", spawn "scrot -d 3 -c")
               ]

If I enter 1 as my command then the search options are filtered down to 1: Capture Screen. Now if I press tab then the command is replaced with 1: Capture Screen as expected, however the completion options are also now extended to include 3: Capture All Screens. i have not been able to replicate this behavior with any other prompts or command inputs.

Proposal

If someone can outline the intended behavior for both completion types then I would be happy to write a PR eliminating whatever edge cases are not intended as part of that behavior.

Alternatively I would propose we eliminate the distinction between both completion types and replace them with a simple fuzzy filter of the completion options based on the currently entered command and the tab key can cycle through the filtered list of completions. alwaysHighlight would be either removed or not do anything (depending on how we handle deprecation of type parameters).

LIke I said, I am motivated to work on and expand the functionality of XMonad.Prompts. I want to do some cooler stuff like pagination and multiselect but before I can get there I think we need to have sensible behavior for completions.

Checklist

slotThe commented 3 years ago

However, the semantics of the completion system are very unclear and possibly buggy. Is there anyone here who can give me clear explanation of the expected behavior of the completions system?

[…]

I'm going to give some examples of weird behavior of completions. I'm hoping we can determine which of these behaviors are intended, which are bugs, and finally define the semantics for completions.

I don't think that anyone who extensively worked on the prompt is still active, so at this point it's patchworks all the way down :) I would say that the "expected" semantics are those who users—right now—expect to have happen. Essentially, go wild.

Many of your examples of the prompt's inconsistency essentially stem from the problem that we don't exactly know which part of the currently entered prompt the user wants to have completed. There are some examples (like the shell prompt) where a user may have entered a command, a space, and then wants a directory auto-completed without touching the initially entered command. Other times things separated by spaces are to be treated as one thing; this creates some friction.

Alternatively I would propose we eliminate the distinction between both completion types and replace them with a simple fuzzy filter of the completion options based on the currently entered command and the tab key can cycle through the filtered list of completions. alwaysHighlight would be either removed or not do anything (depending on how we handle deprecation of type parameters).

It would certainly make the code a lot simpler. I personally don't have anything against this approach (then again, I always use alwaysHighlight, so I'm not sure how people who do not use this option at the moment would react).

solomon-b commented 3 years ago

Many of your examples of the prompt's inconsistency essentially stem from the problem that we don't exactly know which part of the currently entered prompt the user wants to have completed. There are some examples (like the shell prompt) where a user may have entered a command, a space, and then wants a directory auto-completed without touching the initially entered command. Other times things separated by spaces are to be treated as one thing; this creates some friction.

I haven't checked all the prompts, but in the case of the shell prompt it is using the default completion implementations for XPrompt. Rather then trying to have a single universal completion function, I think we should have a very simple default implementation and then defer specialized behavior to the various XPrompt instances.

As an initial project I could rewrite the default completion code as a I described above and save the current implementation outside of the typeclass. Then we can use the old completion code in the instance definitions for all prompts that currently rely on it.

slotThe commented 3 years ago

On Sun, Oct 03 2021 16:35, Solomon Bothwell wrote:

As an initial project I could rewrite the default completion code as a I described above and save the current implementation outside of the typeclass. Then we can use the old completion code in the instance definitions for all prompts that currently rely on it.

Go wild :+1:

solomon-b commented 3 years ago

Go wild +1

Will do :)

solomon-b commented 3 years ago

I'm still working on this. I'm putting together a more minimal prompt design in a seperate cabal project for more rapid prototyping. It isn't ready to discuss here but there is another Prompt related issue I would like to bring up.

The XPrompt typeclass is designed and used in a really odd way. It looks like an attempt at OOP in haskell that at the end of the day is using typeclass dictionaries to emulate the behavior of a plain record of functions. ie., we define a bunch of functions in a typeclass instance and then use a data constructor (XPType) to existentialize the instance and load it into our state.

I think we should just turn XPrompt into a record and call it a day. Thoughts?

slotThe commented 3 years ago

I think we should just turn XPrompt into a record and call it a day. Thoughts?

Would this be as extensible as the current system? It seems less so, but this may just be my initial impression.

It seems like we would just switch from having an "up-front cost" by using type classes to delaying this until later. E.g., especially when using multiple modes it's currently very easy to decide which function to call exactly (just query currentXPMode and be on your way). This would seem to be a bit more work when dropping existentials and making this a normal record of functions.

Another thing to think about here is just how much user code this would break

solomon-b commented 3 years ago

Would this be as extensible as the current system? It seems less so, but this may just be my initial impression.

Unless I am missing something subtle, we would have identical behavior but the code would be a lot more explicit.

It seems like we would just switch from having an "up-front cost" by using type classes to delaying this until later. E.g., especially when using multiple modes it's currently very easy to decide which function to call exactly (just query currentXPMode and be on your way). This would seem to be a bit more work when dropping existentials and making this a normal record of functions.

currentXPMode would work exactly the same way. The XPrompt type would be a record contaning all the functions currently in theXPrompt class (or whatever we decided to change them to when we clean up the semantics). Then we throw away XPType. OperationMode would then contain XPrompt values directly:

data XPrompt = XPrompt { showPrompt :: String, nextCompletion :: String -> [String] -> String, ... }

data XPOperationMode = XPSingleMode ComplFunction XPrompt | XPMultipleModes (W.Stack XPrompt)

currentXPMode :: XPState -> XPrompt
currentXPMode st = case operationMode st of
  XPMultipleModes modes -> W.focus modes
  XPSingleMode _ xprompt -> xprompt

Another thing to think about here is just how much user code this would break

How many people are actually writing their own XPrompt instances in their personal configs? I am under the impression that it is primarily used for writing other Prompt modules in xmonad-contrib.

slotThe commented 3 years ago

Unless I am missing something subtle, we would have identical behavior but the code would be a lot more explicit.

Mh interesting. I would almost say "go for it" if we didn't have to at least somewhat keep backwards compatibility in mind :/

How many people are actually writing their own XPrompt instances in their personal configs? I am under the impression that it is primarily used for writing other Prompt modules in xmonad-contrib.

I have never understood this either, but I've seen small prompts in quite a few user configurations. This may or may not still be something that we want to do at some point, but perhaps we should take this one step at a time (first fix the completion system, then talk about refactoring the core types)

solomon-b commented 3 years ago

Sorry for the slow response time. I've been caught up with work stuff. We can keep the typeclass for now, but we are going to need to change it substantially. It just doesn't really capture what it means to be a prompt.

I have a working prototype in a scratch project that uses this interface:

data Prompt = Prompt
  { filterCompletions :: String -> Completions -> Maybe Completions
  -- ^ Filter the set of possible completions.
  , fetch :: [String] -> IO (Maybe [String])
  -- ^ Update the list of completions based on the current command.
  , complete :: String -> String -> String
  -- ^ Modify the last word of the command based on the focused completion.
  , execute :: String -> IO ()
  -- ^ Perform an action based on the command.
  , divider :: Maybe Char
  -- ^ Optional character for subdividing the command string
  }

Ignore the fact that I used a record, this would be the core of a new XPrompt typeclass. This design differs from the current prompt behavior but it can handle both simple prompts and the shell prompt. The goal is to capture the basic actions that distinguish prompts in this record (or typeclass).

To make it more consistent, I broke up the act of completing the Command from shifting the focus. How we complete a command is highly dependent on the particulars of the Prompt--such as with the shell prompt--but shifting focus in the completions is always the same. Internally in my demo project I used a Zipper for storing completions so we it is easy to shift focus.

I don't think the divider field should exist. That should be baked into the fetch command but I haven't tried doing that yet.

This set of functions is able to handle simple prompts as well as complex IO heavy prompts like the shell prompt. Here are two examples to show simple and IO based prompts:

-- | Prompt to select a completion from a list and print it to stdout
simplePrompt xs = Prompt
  { filterCompletions = \cmd cmpls -> filterZ (isPrefixOf cmd) cmpls
  , fetch = \case
      [] -> pure $ Just xs
      _ -> pure Nothing
  , complete = (<>)
  , execute = \s -> putStrLn $ "WOW GOOD JOB: " <> s
  , divider = Nothing
  }

lookupDirectory :: [String] -> IO (Maybe [String])
lookupDirectory cmd = do
  let path = '.' : '/' : foldl (</>) "" cmd
  exists <- doesPathExist path
  if exists
     then fmap Just $ listDirectory path
     else pure Nothing

-- | Prompt to navigate the file system using completions and build up a path to print to stdout
pathPrompt :: Prompt
pathPrompt = Prompt
  { filterCompletions = \cmd cmpls -> filterZ (isPrefixOf cmd) cmpls
  , fetch = \case
      [] -> lookupDirectory []
      cmd -> lookupDirectory cmd
  , complete = (<>)
  , execute = \s -> putStrLn $ "WOW GOOD JOB: " <> s
  , divider = Just '/'
  }

My demo project is in a larger scratch repo. I'll break it into its own repo and link it here this week so that you can have a look at it.

I'm not married to this specific design, but I think we shouldn't be stuck with 14 year old technical debt out of fear of making breaking changes for users.

slotThe commented 3 years ago

We can keep the typeclass for now, but we are going to need to change it substantially. It just doesn't really capture what it means to be a prompt.

[…]

Ignore the fact that I used a record, this would be the core of a new XPrompt typeclass.

If we're making a breaking change anyways then there's no harm with going all the way and using the better design. I was just under the impression that the completion refactor you were planning was going to be backwards compatible with the current interface, which is why I had reservations about using a record.

I'm not married to this specific design, but I think we shouldn't be stuck with 14 year old technical debt out of fear of making breaking changes for users.

Well, XMonad has a track record of being really rather stable and not breaking too many things (even with 0.17.0 we somehow managed to not really have any gigantic breaking changes, which this would definitely be). This certainly needs to be deliberated thoroughly.

My demo project is in a larger scratch repo. I'll break it into its own repo and link it here this week so that you can have a look at it.

That sounds good :+1: The examples looks nice and make me hopeful that we can move forward with this idea in spite of the huge breaking change it would introduce

slotThe commented 2 years ago

@solomon-b any updates on this?

solomon-b commented 2 years ago

@slotThe thank you for the reminder. I got really caught up with work stuff and then various other side projects. i had made some progress on a branch but at this point its probably out of date. I still want to do this work, but i think it will have to wait until March.

slotThe commented 2 years ago

On Wed, Feb 23 2022 11:35, Solomon wrote:

I still want to do this work, but i think it will have to wait until March.

Not a problem at all! When you feel comfortable with the basic design do feel free to open a PR—I'm more than willing to help you iron out any remaining kinks and/or start porting the prompt modules to the new interface

solomon-b commented 2 years ago

Thank you! I'll reach out when I get back into it.

slotThe commented 1 year ago

@solomon-b Well, this was a while ago :) Is this still something you're interested in tackling?

solomon-b commented 1 year ago

Oh my. I can't believe its been so long since i looked into this.

I have somewhat mixed feelings about this project now. I could be convinced otherwise but I'm wondering if the prompt system should be considered out of scope for XMonad.

Especially given the call to action for migrating to Wayland I am thinking it might be worth considering what we could drop to make the Wayland migration realistic.

What if we broke out a bunch of features like prompts into standalone applications and then came up with really nice story for how everything can communicate at runtime?

geekosaur commented 1 year ago

Contribs, including Prompt, will need to be redone for a Wayland port anyway. And at minimum Prompt should be changed to hook into the handleEventHook instead of blocking the main loop.

I am not sure that runtime communication will work for all the uses of Prompt in contrib.

slotThe commented 1 year ago

As geekosaur already said, pretty much all of contrib (and definitely the prompt) will have to be scrapped for Wayland anyways, so "future compatibility" is not super relevant here, I think. I reckon most of the prompts that exist could be emulated with external applications like dmenu or rofi. Still, having this built into XMonad feels like a nice thing to me :)

Since the prompt is already a thing, considering it out of scope now will not happen, so I think that trying to improve it is still a worthwhile goal.

geekosaur commented 1 year ago

I actually disagree about external applications: many uses of the prompt alter internal xmonad state. One might argue that you could use xdotool for many of them, but xdotool doesn't exist (and can't with their current permissions model) for Wayland.

And improving it now will still guide a later Wayland port.