2 patches for repository http://darcs.net:

Fri May  7 21:19:54 CEST 2010  Florent Becker <florent.becker@ens-lyon.org>
  * resolve issue1839: k broken in interactive selection

Wed May 12 18:15:52 CEST 2010  Florent Becker <florent.becker@ens-lyon.org>
  * resolve issue1843

Attachments

• resolve-issue1839_-k-broken-in-interactive-selection.dpatch
• unnamed

On Wed, May 12, 2010 at 16:19:28 +0000, Florent Becker wrote:
> Wed May 12 18:15:52 CEST 2010  Florent Becker <florent.becker@ens-lyon.org>
>   * resolve issue1843

Hmm, that should probably be slightly more descriptive for the long term.

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

2 patches for repository http://darcs.net:

Fri May  7 21:19:54 CEST 2010  Florent Becker <florent.becker@ens-lyon.org>
  * resolve issue1839: k broken in interactive selection

Sat May 15 16:05:07 CEST 2010  Florent Becker <florent.becker@ens-lyon.org>
  * resolve issue1843:  interactive 'v' prints double entries

Attachments

• resolve-issue1839_-k-broken-in-interactive-selection.dpatch
• unnamed

Sorry, in case anybody thought I was reviewing this, I'm a bit busy at
work right now (probably till 27 May).

Any takers?

Reinier, perhaps you'd fancy a chance to get to know the SelectChanges
system (excuse me if you're already familiar with it, just clumsily
trying to spread out as much codebase awareness as possible).  That
said, with the new changes, I think only Florent and Ganesh really know
it well :-)

Hi Florent, others,

I can't review the issue 1839 fix very well because I don't know in
what way 'k' was broken. I can't easily reproduce it when I try to
record something, and the ticket is not very informative.

As for the issue 1843 fix, it looks OK if it works with the 'e' key.
I'm going to check that now if my battery lets me.

Some more comments I had while reading the code:

In the first patch, "resolve issue1839: k broken in interactive
selection", I see:
> addfile ./tto

What's that?

> -textSelect' whch = do
> -  z <- gets tps
> -  when (not $ rightmost z) $
> +textSelect' whch =
> +    do
> +      z <- gets tps
> +      when (not $ rightmost z) $

Is this darcs coding style approved? I certainly use and favor the
style that is removed here...

And a general nagging comment about the SelectChanges code: why didn't
you add many comments during the recent overhaul? Darcs.Patch.Choices
is a good example of how it should be :)

Bye,
Reinier

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Le 26/05/2010 18:58, Reinier Lamers a écrit :
> 
> 
> Reinier Lamers <tux_rocker@reinier.de> added the comment:
> 
> Hi Florent, others,
> 
> I can't review the issue 1839 fix very well because I don't know in
> what way 'k' was broken. I can't easily reproduce it when I try to
> record something, and the ticket is not very informative.
> 
What happened is that 'k' would show you the same patch again. The
problem is that it did rewind one patch, but then it went to look for
the next undecided patch, and skipped again the patch to which you just
rewinded. This make 'k' inoperant. To test this, you need to have at
least 2 hunks, and answer 'y', then 'k'.

> As for the issue 1843 fix, it looks OK if it works with the 'e' key.
> I'm going to check that now if my battery lets me.

It should work with 'e', but my tests were not thorough.
> 
> Some more comments I had while reading the code:
> 
> In the first patch, "resolve issue1839: k broken in interactive
> selection", I see:
>> addfile ./tto
> 
> What's that?
> 
That should not be there, i'll amend it out.

>> -textSelect' whch = do
>> -  z <- gets tps
>> -  when (not $ rightmost z) $
>> +textSelect' whch =
>> +    do
>> +      z <- gets tps
>> +      when (not $ rightmost z) $
> 
> Is this darcs coding style approved? I certainly use and favor the
> style that is removed here...

That is the result of editing back and forth, I can restore that in the
amend.
> 
> And a general nagging comment about the SelectChanges code: why didn't
> you add many comments during the recent overhaul? Darcs.Patch.Choices
> is a good example of how it should be :)
> 
Yes, that was on my todo-list before those bugs.

Bye,

Florent
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkv9V9oACgkQTCPcDztjGo7fEACfcoCqa/oQVoAb32bDJuP20MxE
ZfMAn3wZA2kjJvWQcHL2aKsJqorkx8KS
=QGpW
-----END PGP SIGNATURE-----

2 patches for repository http://darcs.net:

Thu May 27 23:17:10 CEST 2010  Florent Becker <florent.becker@ens-lyon.org>
  * resolve issue1839: k broken in interactive selection

Thu May 27 23:19:45 CEST 2010  Florent Becker <florent.becker@ens-lyon.org>
  * resolve issue1843:  interactive 'v' prints double entries

Attachments

• resolve-issue1839_-k-broken-in-interactive-selection.dpatch
• unnamed

Back to you, Reinier.

>[resolve issue1839: k broken in interactive selection
>Florent Becker <florent.becker@ens-lyon.org>**20100527211710
> Ignore-this: 4f64f36ce0c6645537e16a99bd7f73b2
>] hunk ./src/Darcs/SelectChanges.hs 348
>              FL (TaggedPatch p) C(x y) -> PatchChoices p C(x y)
>              -> PatchSelectionM p IO (PatchChoices p C(x y))
> textSelect whch tps pcs = do
>-    userSelection <- execStateT (textSelect' whch) $
>+    userSelection <- execStateT (skipMundane whch "Skipped" >> textSelect'
> whch) $ ISC { total = lengthFL tps
>                          , current = 0
>                          , tps = FZipper NilRL tps

This skipMundane call used to be inside textSelect'.

>hunk ./src/Darcs/SelectChanges.hs 359
>               InteractiveSelectionM p C(x y) ()
> textSelect' whch = do
>   z <- gets tps
>-  skipMundane whch "Skipped"
>   when (not $ rightmost z) $
>        do
>          textSelectOne whch

Here we see it's removed from textSelect'. textSelect' is a recursive 
function, called again after every keypress. When you used 'k' to go back to a 
patch you already decided about, this skipMundane call would skip that 
decided-about patch. So you would see the same patch as you saw before you 
pressed 'k'.

>hunk ./src/Darcs/SelectChanges.hs 603
>              jn_cap = (toUpper $ head jn) : tail jn
>          yorn <- promptUser singleFile the_default
>          case yorn of
>-               'y' -> decide whichch True tp >> skipOne
>-               'n' -> decide whichch False tp >> skipOne
>-               'w' -> postponeNext >> skipOne
>+               'y' -> decide whichch True tp >> skipOne >> skipMundane
> whichch "Skipped" +               'n' -> decide whichch False tp >> skipOne
> >> skipMundane whichch "Skipped" +               'w' -> postponeNext >>
> skipOne >> skipOne >> skipMundane whichch "Skipped" 'e' | (Just s) <- spl
> -> splitCurrent s
>                's' -> currentFile >>= maybe
>                        (return ())
>hunk ./src/Darcs/SelectChanges.hs 609
>-                       (\f -> decideWholeFile whichch f False)
>+                       (\f -> decideWholeFile whichch f False) >>
> skipMundane whichch "Skipped" 'f' -> currentFile >>= maybe
>                        (return ())
>hunk ./src/Darcs/SelectChanges.hs 612
>-                       (\f -> decideWholeFile whichch f True)
>+                       (\f -> decideWholeFile whichch f True) >>
> skipMundane whichch "Skipped" 'v' -> liftIO $ unseal2 printPatch reprCur
>                'p' -> liftIO $ unseal2 printPatchPager reprCur
>                'l' -> printSelected whichch

Now that skipMundane is gone from the top of textSelect', we must call it 
explicitly after those keys have been pressed that do have to take us to the 
next undecided patch.

>[resolve issue1843:  interactive 'v' prints double entries
>Florent Becker <florent.becker@ens-lyon.org>**20100527211945
> Ignore-this: bdcec9798ec4b536bb628dad0c337949
>] hunk ./src/Darcs/SelectChanges.hs 348
>              FL (TaggedPatch p) C(x y) -> PatchChoices p C(x y)
>              -> PatchSelectionM p IO (PatchChoices p C(x y))
> textSelect whch tps pcs = do
>-    userSelection <- execStateT (skipMundane whch "Skipped" >> textSelect'
> whch) $ +    userSelection <- execStateT (skipMundane whch "Skipped" >>
>+                                 showCur whch >>
>+                                 textSelect' whch) $
>                      ISC { total = lengthFL tps
>                          , current = 0
>                          , tps = FZipper NilRL tps

In this patch, with the 'showCur whch' line, we do something similar to what 
we did with the 'skipMundane whch "Skipped"' line in the previous patch. 
Instead of calling it in one place in the loop, where it was called too often, 
we move the call to a couple of places where we actually need it.

>hunk ./src/Darcs/SelectChanges.hs 597
>          o <- asks opts
>          let singleFile = isSingleFile (tpPatch tp)
>              reprCur = repr whichch (Sealed2 (tpPatch tp))
>-         liftIO . (unseal2 (printFriendly o)) $ reprCur
>          options' <- options singleFile
>          theSlot <- liftChoices $ patchSlot' tp
>          let

This change makes darcs not always show the current patch before a prompt

>hunk ./src/Darcs/SelectChanges.hs 603
>              the_default = get_default (whichch == Last || whichch ==
> FirstReversed) theSlot jn_cap = (toUpper $ head jn) : tail jn
>          yorn <- promptUser singleFile the_default
>+         let nextPatch = skipMundane whichch "Skipped" >> showCur whichch
>          case yorn of

This defines a helper action 'nextPatch' that makes darcs skip to the next 
undecided patch and show it to the user.

>hunk ./src/Darcs/SelectChanges.hs 605
>-               'y' -> decide whichch True tp >> skipOne >> skipMundane
> whichch "Skipped" -               'n' -> decide whichch False tp >> skipOne
> >> skipMundane whichch "Skipped" -               'w' -> postponeNext >>
> skipOne >> skipOne >> skipMundane whichch "Skipped" -               'e' |
> (Just s) <- spl -> splitCurrent s
>+               'y' -> decide whichch True tp >> skipOne >> nextPatch
>+               'n' -> decide whichch False tp >> skipOne >> nextPatch
>+               'w' -> postponeNext >> skipOne >> skipOne >> nextPatch
>+               'e' | (Just s) <- spl -> splitCurrent s >> showCur whichch
>                's' -> currentFile >>= maybe
>                        (return ())
>hunk ./src/Darcs/SelectChanges.hs 611
>-                       (\f -> decideWholeFile whichch f False) >>
> skipMundane whichch "Skipped" +                       (\f ->
> decideWholeFile whichch f False) >> nextPatch 'f' -> currentFile >>= maybe
>                        (return ())
>hunk ./src/Darcs/SelectChanges.hs 614
>-                       (\f -> decideWholeFile whichch f True) >>
> skipMundane whichch "Skipped" +                       (\f ->
> decideWholeFile whichch f True) >> nextPatch 'v' -> liftIO $ unseal2
> printPatch reprCur
>                'p' -> liftIO $ unseal2 printPatchPager reprCur
>hunk ./src/Darcs/SelectChanges.hs 617
>-               'l' -> printSelected whichch
>+               'l' -> printSelected whichch >> showCur whichch
>                'x' -> liftIO $ unseal2 printSummary reprCur
>                'd' -> skipAll
>                'a' ->
>hunk ./src/Darcs/SelectChanges.hs 628
>                'q' -> liftIO $
>                       do putStrLn $ jn_cap++" cancelled."
>                          exitWith $ ExitSuccess
>-               'j' -> skipOne
>-               'k' -> backOne
>+               'j' -> skipOne >> showCur whichch
>+               'k' -> backOne >> showCur whichch
>                _   -> do liftIO . putStrLn $ helpFor jn options'

These make darcs show the patch where it is needed.

>hunk ./src/Darcs/SelectChanges.hs 6`32
>+showCur :: forall p C(x y) . Patchy p => WhichChanges
>+           -> InteractiveSelectionM p C(x y) ()
>+showCur whichch = do
>+  o <- asks opts
>+  c <- currentPatch
>+  case c of
>+      Nothing -> return ()
>+      Just (Sealed2 tp) -> do
>+             let reprCur = repr whichch (Sealed2 (tpPatch tp))
>+             liftIO . (unseal2 (printFriendly o)) $ reprCur
>+
>+
> text_view :: forall p C(x y u r s). Patchy p => [DarcsFlag] -> Maybe Int ->
> Int -> [Sealed2 p] -> [Sealed2 p]
>             -> IO ()

This defines a function to show the current patch. reprCur is duplicated in 
textSelectOne, but it's only 6 words.

I'm going to push it, thanks!

Reinier

This patch bundle (with 2 patches) was just applied to the repository http://darcs.net/.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_.html#bundle-741080e4557e234063f9a5060ecf9652a68d721f

This patch bundle (with 2 patches) was just applied to the repository http://darcs.net/reviewed.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_reviewed.html#bundle-741080e4557e234063f9a5060ecf9652a68d721f