On Sun, Oct 17, 2010 at 15:45:43 +0000, Florent Becker wrote:
> Sun Oct 17 17:41:31 CEST 2010 Florent Becker <florent.becker@ens-lyon.org>
> * resolve issue114: allow hunk-splitting in revert
> The UI is stolen from record-hunk-splitting and therefore subpar. It should be checked by native english-speakers and non-programmers.
I had a closer look at this.
We will need some follow-up work to get a custom UI for this. The
record text is not appropriate here (this will only affect the patch,
huh?), and the hints may need to be adapted.
The implementation seems to make sense, but I'm going to wait to apply
it until we have some kind of testing. This is my feeble attempt as one
reviewer to emphasise quality control a little bit by asking for testing
more frequently. Am just dipping my toes in the water, though, because
I'm still not sure how to go about it. For example, does the hunk editor
lend itself to unit testing? As a minimum, I guess we want some shell
tests using cat as our DARCS_EDITOR. (Maybe this patch a good candidate
for Zooko comments about how exactly a TDD hacker would have done the work?)
Also: we have a problem of test inertia caused by there not being enough
pre-existing test infrastructure. For example, it may not be very fair
to ask Florent to write tests for the revert hunk editor if the record
hunk editor tests don't exist either. Should we treat this sort of
lack-of-infrastructure problem as a sort of collective responsibility?
Sounds like a setup for the Bystander Effect...
user interface
---------------
This patch introduces the option of doing partial darcs revert on hunks.
For example, if you accidentally introduced some trailing whitespace
along with some useful changes, it'd be kind of nice to be able to just
nuke that space right there and then.
Right now, the UI looks like this:
Interactive hunk edit:
- Edit the section marked 'AFTER'
- Arbitrary editing is supported
- This will only affect the patch, not your working copy
- Hints:
- To split added text, delete the part you want to postpone
- To split removed text, copy back the part you want to retain
========================== BEFORE (reference) ==========================
new stuff 1
new JUNK stuff 2
new stuff 3
============================= AFTER (edit) =============================
old stuff 1
old stuff 2
============================= (edit above) =============================
So here what you would do is replace old stuff with new stuff minus the
JUNK.
darcs revert then asks you first about changing the old stuff to the new
stuff, and then about changing the new stuff to new stuff plus JUNK (so
no, don't revert that and then yes do revert that).
For UI issues, in case it helps to have a common set of reference
examples, we can use this repo from when we were first kicking around
hunk editor ideas:
http://patch-tag.com/r/kowey/rambling-robot
The hope is to collect use cases so we don't have to keep making them up
over and over again (patches would be great).
resolve issue114: allow hunk-splitting in revert
------------------------------------------------
> hunk ./src/Darcs/Commands/Revert.lhs 96
> (case touching_changes of
> NilFL -> putStrLn "There are no changes to revert!"
> _ -> do
> - let context = selectionContextPrim "revert" opts Nothing pre_changed_files
> + let context = selectionContextPrim "revert" opts (Just reversePrimSplitter) pre_changed_files
No surprises here, just provide a hunk splitter that we can use.
I guess SelectChanges is clever enough to auto-insert the 'e' option.
> -primSplitter = Splitter { applySplitter = doPrimSplit, canonizeSplit = canonizeFL }
> +primSplitter = Splitter { applySplitter = doPrimSplit
> + , canonizeSplit = canonizeFL }
This whitespace change is likely introduced it's easier to visually
compare with the reversePrimSplitter below.
> +doReversePrimSplit :: Prim C(x y) -> Maybe (B.ByteString, B.ByteString -> Maybe (FL Prim C(x y)))
> +doReversePrimSplit p = do
> + (text, parser) <- doPrimSplit (invert p)
> + let parser' p = do
> + patch <- parser p
> + return . reverseRL $ invertFL patch
> + return (text, parser')
Split the *inverse* of the patch (so the user sees the reverted state as
'after'), taking care to re-invert the results when the user hands
them back to us.
If I understand correctly, we could alternatively have just used the
primSplitter, in which case the user would see the patch the same way
as in darcs record. It may turn out that this is more natural (maybe
with some revert-specific language) because the interactive UI presents the
patch to you in forward form anyway. But this is just useless speculation.
I guess ideally we'd be sitting a Simon down and watching them try to use
the darn thing...
> +reversePrimSplitter :: Splitter Prim
> +reversePrimSplitter = Splitter { applySplitter = doReversePrimSplit
> + , canonizeSplit = canonizeFL}
Seems fine. Looks like anticipating different kinds of splitters
made it a lot easier to implement this desirable feature.
--
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
For a faster response, try +44 (0)1273 64 2905 or
xmpp:kowey@jabber.fr (Jabber or Google Talk only)
|
