I'm sending this mainly because of the defaulting to 'n'
behaviour it adds. We'll need to reconcile the separator text
and other help text with Eric's submission at some point.

Also I haven't run the tests yet or even tested the changed
behaviour very carefully - just want to get it out there to help
the discussion.

1 patch for repository http://darcs.net:

Mon Jan 18 20:57:54 GMT 2010  Ganesh Sittampalam <ganesh@earth.li>
  * tweaks to hunk editing interface
  Allow arbitrary number of intermediate states
  Default final "filler" patch to 'n'

Attachments

• tweaks-to-hunk-editing-interface.dpatch
• unnamed

tweaks to hunk editing interface
--------------------------------
Ganesh Sittampalam <ganesh@earth.li>**20100118205754

hunk ./src/Darcs/Patch/Split.hs 63
>  -- splitters for Prim etc, and the generality doesn't cost anything.
>  data Splitter p
>    = Splitter {
> -              applySplitter :: FORALL(x y) p C(x y)
> -                              -> Maybe (B.ByteString,
> -                                        B.ByteString -> Maybe (FL p C(x y)))
> +              applySplitter :: FORALL(x y) p C(x y) -- ^patch to split
> +                            -> Maybe (B.ByteString, -- ^text to present for
editing
> +                                      B.ByteString -> Maybe ((FL p :> FL p :>
FL p) C(x y))
> +                                       -- ^parse edited text into split
patches, in three parts
> +                                       -- according to whether they should be
presented to the
> +                                       -- user: default yes, ask, default no.
> +                                     )
>                -- canonization is needed to undo the effects of splitting
>                -- Typically, the list returned by applySplitter will not
>                -- be in the simplest possible form (since the user will have

Ok, so we will silently accept the first part, ask about the second,
and silently reject the third part. Is the first part useful? On the
other hand, it does no harm being there.

hunk ./src/Darcs/Patch/Split.hs 92
>  -}
>  
>  -- Does not canonize as there is no generic operation to do this.
> -withEditedHead :: Invert p => p C(x y) -> p C(x z) -> FL p C(x y)
> -withEditedHead p res = res :>: invert res :>: p :>: NilFL
> +withEditedHead :: Invert p => p C(x y) -> p C(x z) -> (FL p :> FL p :> FL p)
C(x y)
> +withEditedHead p res = NilFL :> (res :>: NilFL) :> (invert res :>: p :>: NilFL)

We don't add anything silently, we ask the user about their edited
version of the patch, and hide the 'filler' patch from them. Problem:
the hidden 'filler' patch will reappear if they use 'k' in the
interactive selection.


hunk ./src/Darcs/Patch/Split.hs 115
>  noSplitter = Splitter { applySplitter = const Nothing, canonizeSplit = id }
>  
>  
> -doPrimSplit :: Prim C(x y) -> Maybe (B.ByteString, B.ByteString -> Maybe (FL
Prim C(x y)))
> +doPrimSplit :: Prim C(x y) -> Maybe (B.ByteString, B.ByteString -> Maybe ((FL
Prim :> FL Prim :> FL Prim) C(x y)))
>  doPrimSplit (FP fn (Hunk n before after))
>   = Just (B.concat $ intersperse (BC.pack "\n") $ (helptext ++ [sep] ++ before
++ [sep] ++ after ++ [sep]),

ok, will eventually be superseded by the new version of the help text.

hunk ./src/Darcs/Patch/Split.hs 118
> -         \bs -> do let ls = BC.split '\n' bs
> -                   (_, ls') <- breakSep ls
> -                   (before', ls'') <- breakSep ls'
> -                   (after', _) <- breakSep ls''
> -                   return (hunk before before' +>+ hunk before' after' +>+
hunk after' after))
> -    where sep = BC.pack "==="
> +         \bs -> do let intermediates = dropWhile (==before) . revDropWhile
(==after) . -- ignore unchanged blocks
> +                                       tail . -- drop lines before first
separator
> +                                       unfoldr breakSep . -- split on
separator, dropping any lines after last separator
> +                                       BC.split '\n' $ bs
> +                   return (NilFL :> mkHunks before intermediates after))
> +    where sepStr = "==="
> +          sep = BC.pack sepStr
>            helptext = map BC.pack ["Interactive hunk edit:",

ok

hunk ./src/Darcs/Patch/Split.hs 126
> -                                  " - Edit the first set of lines to insert a
new change before the current one.",
> -                                  " - Edit the second set of lines to insert
a new change after the current one."]
> +                                  " The lines between the " ++ sepStr ++ "
separators show the old and new",
> +                                  " states of the hunk. Edit either to
provide an alternative new state.",
> +                                  " You will then be offered the changes
between the alternative new state",
> +                                  " and the old state to confirm."
> +                                 ]
>            hunk b a = canonize (FP fn (Hunk n b a))

This is confusing, but should eventually disappear.

hunk ./src/Darcs/Patch/Split.hs 132
> +          mkHunks b [] a = hunk b a :> NilFL -- no edits made, just offer
original patch
> +          mkHunks b [i] a = hunk b i :> hunk i a -- final "fixup" should
default to no
> +          mkHunks b (i:is) a = case mkHunks i is a of ms :> ns -> (hunk b i
+>+ ms) :> ns
>            breakSep xs = case break (==sep) xs of
>                             (_, []) -> Nothing
>                             (ys, _:zs) -> Just (ys, zs)

ok

hunk ./src/Darcs/Patch/Split.hs 138
> +          revDropWhile p = reverse . dropWhile p . reverse
>  doPrimSplit _ = Nothing
>  
>  -- |Split a primitive hunk patch up

ok

hunk ./src/Darcs/SelectChanges.hs 477
>                  -> do newText <- editText "darcs-patch-edit" text
>                        case parse newText of
>                          Nothing -> repeat_this
> -                        Just ps -> do let tps_new = snd $ patchChoicesTpsSub
(Just (tag tp)) ps
> +                        Just (ys :> ms :> ns)
> +                                -> do let tps_new = snd $ patchChoicesTpsSub
(Just (tag tp)) (ys +>+ ms +>+ ns)
> +                                          tags_new = mapFL tag tps_new
> +                                          yes_tags = take (lengthFL ys) tags_new
> +                                          no_tags = drop (lengthFL ys +
lengthFL ms) tags_new
> +                                          subst_pc = substitute (seal2 (tp
:||: tps_new)) pc
> +                                          new_pc = foldr force_no (foldr
force_yes subst_pc yes_tags) no_tags
>                                        text_select splitter
>                                                    jn whichch critopts (n_max
+ lengthFL tps_new - 1) n

hunk ./src/Darcs/SelectChanges.hs 486
> -                                                  tps_done (tps_new+>+tps_todo')
> -                                                  (substitute (seal2 (tp :||:
tps_new)) pc)
> +                                                  tps_done
(tps_new+>+tps_todo') new_pc
>  
>              's' -> do_next_action "Skipped"  (Noun "change") $ skip_file
>              'f' -> do_next_action "Included" (Noun "change") $ do_file


This puts the new patches in the yes, no or wait bucket of the
patch-choices according to where they are in the FL :> FL :> FL.

I think it is best not to default the new hunks to 'n', since the user will be
able to reach them by using 'k'. I think it's best for them to always see the
filler patches than to accidentaly stumble on them when they use 'k'. But maybe
showing the filler patch is not such a problem, especially given that the edited
and filler patch will be offered next to each other. So i'd rather go with an
interface which offers the filler patches by default, and then apply this if
people do get confused, and we have a way to not show the filler patches, even
with 'k'.

Tagged further-discussion-needed.

I think having the filler patches available via 'k' is quite important for
consistency with the rest of the interactive selection UI. If you have patches A
and B being offered initially by record, with B depending on A (I think this can
happen if A is a hunk patch and B is a replace patch), then B defaults to 'n' if
you select A, but you can still use 'k' to go back and select B, which then
automatically selects 'A' too.

So if you get into the same situation after interactive editing, e.g. by editing
A in the scenario above, or by doing multiple interactive edits to artificially
create dependent hunk patches, I think the same thing should happen.

On Wed, Jan 20, 2010 at 08:01:17 +0000, Ganesh Sittampalam wrote:
> I think having the filler patches available via 'k' is quite important for
> consistency with the rest of the interactive selection UI. If you have patches A
> and B being offered initially by record, with B depending on A (I think this can
> happen if A is a hunk patch and B is a replace patch), then B defaults to 'n' if
> you select A, but you can still use 'k' to go back and select B, which then
> automatically selects 'A' too.

Actually, one more reason to show the filler patches is that then users
know they have to revert them.

It also ties into the hypothetical revert-when-recording feature.

OK, is there a good way to get out of indecision mode?

I think that maybe with the right explanation, we can make the filler
patches obvious.

Ganesh: would you be comfortable making the final call?

-- 
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
PGP Key ID: 08AC04F9

Eric Kow <kowey@darcs.net> writes:
> OK, is there a good way to get out of indecision mode?
>
> I think that maybe with the right explanation, we can make the filler
> patches obvious.
I am strongly in favour of *not* hiding them using the 'n' by default
tactic. Also, I'd love to see a help text improvement on HEAD. We have
collected a lot of text, it should only be matter of distilling it into
the online help text.

Btw., I dislike the 'filler' terminology as well. I don't see what these
patches are "filling". In normal usage, they are the "second half" of
the split. I don't find it surprising to see the second half (it would
probably be more surprising to not see it). And I also agree with the
revert argument (I.e. anything I say 'n' to in record should be later
available to darcs revert, and nothing else, unless dependencies caused
a skipped question -- but these are announced as well.)

Yours,
   Petr.

Oh well, I guess we're going with keeping the filler patches for now.  I
realise they are a cause for confusion, but I suspect it's better to pay
that confusion up front (during record) than later (when they do a darcs
whatsnew later on).