[darcs-devel] [patch655] resolve isssue1920: last regrets prompt in interactive mode

[Eric Kow]

Eric Kow <kowey at darcs.net> added the comment:

Was sure nice meeting you at the sprint, Johannes. Hope you had fun :-)


Attaching my review so I don't lose my place.  Trying to work out which 
of the two implementations we should apply, and what ideas to merge.

Bottom line is that I'd be happy to apply this in principle and then do 
tests plus some minor touch-ups (and have Johannes review said touch-ups 
if he's available).  

Haven't looked at the other implementation yet, though.

We need to fix our tests to add a last-regrets prompt to
record/amend tests

resolve isssue1920: last regrets prompt in interactive mode
-----------------------------------------------------------
> ] hunk ./src/Darcs/Commands/AmendRecord.hs 152
>                                        (filter (==All) opts)
>                                        (Just primSplitter)
>                                        (map toFilePath <$> files)
> -                 chosenPatches <- runSelection (selectChanges First 
ch) context
> +                 chosenPatches <- runSelection (selectChanges First 
True ch) context

So what we have here is that record/amend-record have a last-regrets
prompt and the other commands don't, so we modify all of them, adding
'True' if we want the prompt and 'False' otherwise.

I think one thing we've discovered in Darcs is that boolean flags tend
to travel around the code base a lot and get confusing when you have
more than one of them at a time.  We've since taken to just making up
our own little 2 member enums for that (eg. data Regrets = Regrets |
NoRegrets) for a little extra safety/clarity.

Also, I wonder if this is something we really want as a parameter to the
function, or if we're better off with one function and maybe something
like a selectChangesWithRegrets First ch.  The idea then would be that
people would use selectChanges by default, and selectChangesWithRegrets
would be a rarer case.

> hunk ./src/Darcs/SelectChanges.hs 314

Functions in SelectChanges now just thread the new askLastRegrets Bool
down to textSelect.

>  -- | The actual interactive selection process.
> hunk ./src/Darcs/SelectChanges.hs 415
> -textSelect :: forall p C(x y) . Patchy p => WhichChanges ->
> +textSelect :: forall p C(x y) . Patchy p => WhichChanges -> Bool ->
>               FL (TaggedPatch p) C(x y) -> PatchChoices p C(x y)
>               -> PatchSelectionM p IO (PatchChoices p C(x y))
> hunk ./src/Darcs/SelectChanges.hs 418
> -textSelect whch tps' pcs = do
> +textSelect whch askLastRegrets tps' pcs = do
>      userSelection <- execStateT (skipMundane whch >>
>                                   showCur whch >>
>                                   textSelect' whch) $
> hunk ./src/Darcs/SelectChanges.hs 425
>                       ISC { total = lengthFL tps'
>                           , current = 0
>                           , tps = FZipper NilRL tps'
> -                         , choices = pcs }
> +                         , choices = pcs
> +                         , lastRegretsPrompt = askLastRegrets }

textSelect then sets the ISC tuple accordingly.  The ISC tuple was
introduced to avoid the code smell of the ever-growing-parameter-list
code smell.  Seems like it's working.

>  textSelect' whch = do
>    z <- gets tps
> -  when (not $ rightmost z) $
> -       do
> -         textSelectOne whch
> -         textSelect' whch
> +  if (not $ rightmost z)
> +      then do textSelectOne whch
> +              textSelect' whch
> +      else textAskConfirm whch

This implements the trigger for the last regrets prompt

> +optionsConfirm :: String -> [KeyPress]
> +optionsConfirm jn =
> +    [ KeyPress 'd' ("confirm "++jn)
> +    , KeyPress 'q' ("cancel "++jn) ]

I would have called this optionsLastRegrets

> +-- | Asks the user for last regrets about the selection.
> +-- The message is 'Confirm record? [Dqjk], or ? for help:'
> +textAskConfirm :: forall p C(x y). Patchy p => WhichChanges
> +                     -> InteractiveSelectionM p C(x y) ()
> +textAskConfirm whichch = do

I would have called this textAskRegrets

> +    reask_user
> +    where reask_user = gets lastRegretsPrompt >>= \lr ->
> +              when lr $
> +                  do jn <- asks jobname
> +                     let prompt' = "Confirm " ++ jn ++ "?"
> +                     yorn <- liftIO $ promptChar $
> +                         PromptConfig { pPrompt = prompt'
> +                                      , pBasicCharacters =
> +                                          keysFor $ [ optionsConfirm 
jn
> +                                                    , optionsNav 
"hunk" ]
> +                                      , pAdvancedCharacters = []
> +                                      , pDefault = Just 'd'
> +                                      , pHelp = "?h" }
> +                     case yorn of
> +                       'd' -> return ()
> +                       'k' -> backOne >> showCur whichch >> 
textSelect' whichch
> +                       'q' -> liftIO $ do putStrLn $ jn++" 
cancelled."
> +                                          exitWith $ ExitSuccess
> +                       '?' -> do liftIO $ putStrLn $
> +                                     helpFor jn [optionsConfirm jn]
> +                                                [optionsNav "hunk"]
> +                                 reask_user
> +                       _ -> reask_user

Some potential trouble spots:

- This assumes we want to go back to textSelect', which is fine for now
  but I wonder if it will surprise us later on if the code grows.  I
  wonder if there is a way to avoid trouble there?

- Minor UI problem: we should probably find a way to use `thing`
  instead of assuming we want the word 'hunk' here (incidentally
  we want 'change' instead of 'hunk')

- A bit of of repetition here.
    [optionsConfirm jn] [optionsNav "hunk"]
  I think I would have done something like
     optionsBasic = optionsConfirm jn
     optionsAdv   = optionsNav "hunk"

Nothing that really bothers me a lot though.

> +noLastRegrets ::InteractiveSelectionM p C(x y) ()
> +noLastRegrets = modify (\isc -> isc { lastRegretsPrompt = False })

> hunk ./src/Darcs/SelectChanges.hs 753
>                 'p' -> liftIO $ unseal2 printPatchPager reprCur
>                 'l' -> printSelected whichch >> showCur whichch
>                 'x' -> liftIO $ unseal2 printSummary reprCur
> -               'd' -> skipAll
> +               'd' -> noLastRegrets >> skipAll
>                 'a' ->
>                     do
>                       askConfirmation
> hunk ./src/Darcs/SelectChanges.hs 757
> +                     noLastRegrets
>                       modChoices $ selectAllMiddles (whichch == Last 
|| whichch == FirstReversed)
>                       skipAll
>                 'q' -> liftIO $

Nice. These make sure you don't get an extra prompt if you hit 'd' or
'a' early on

__________________________________
Darcs bug tracker <bugs at darcs.net>
<http://bugs.darcs.net/patch655>
__________________________________