darcs

Patch 245 resolve issue1839: k broken in interacti... (and 1 more)

Title resolve issue1839: k broken in interacti... (and 1 more)
Superseder Nosy List florent.becker, galbolle, kowey, tux_rocker
Related Issues
Status accepted Assigned To tux_rocker
Milestone

Created on 2010-05-12.16:19:28 by galbolle, last changed 2011-05-10.22:36:21 by darcswatch. Tracked on DarcsWatch.

Files
File name Status Uploaded Type Edit Remove
resolve-issue1839_-k-broken-in-interactive-selection.dpatch galbolle, 2010-05-12.16:19:28 text/x-darcs-patch
resolve-issue1839_-k-broken-in-interactive-selection.dpatch galbolle, 2010-05-15.14:32:42 text/x-darcs-patch
resolve-issue1839_-k-broken-in-interactive-selection.dpatch galbolle, 2010-05-28.09:16:39 text/x-darcs-patch
unnamed galbolle, 2010-05-12.16:19:28
unnamed galbolle, 2010-05-15.14:32:42
unnamed galbolle, 2010-05-28.09:16:39
See mailing list archives for discussion on individual patches.
Messages
msg11056 (view) Author: galbolle Date: 2010-05-12.16:19:28
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
msg11057 (view) Author: kowey Date: 2010-05-12.16:32:16
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
msg11073 (view) Author: galbolle Date: 2010-05-15.14:32:42
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
msg11096 (view) Author: kowey Date: 2010-05-20.15:55:03
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 :-)
msg11115 (view) Author: tux_rocker Date: 2010-05-26.16:58:27
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
msg11116 (view) Author: galbolle Date: 2010-05-26.17:12:34
-----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-----
msg11133 (view) Author: galbolle Date: 2010-05-28.09:16:39
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
msg11138 (view) Author: kowey Date: 2010-05-28.20:27:22
Back to you, Reinier.
msg11192 (view) Author: tux_rocker Date: 2010-06-02.17:00:49
>[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
msg11193 (view) Author: darcswatch Date: 2010-06-02.18:01:50
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
msg14289 (view) Author: darcswatch Date: 2011-05-10.20:36:37
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
History
Date User Action Args
2010-05-12 16:19:28galbollecreate
2010-05-12 16:24:23darcswatchsetdarcswatchurl: http://darcswatch.nomeata.de/repo_http:__darcs.net_.html#bundle-bc71e36e15432f0938e559476bc306e9c8adce0a
2010-05-12 16:32:16koweysetnosy: + kowey
messages: + msg11057
2010-05-15 14:32:42galbollesetfiles: + resolve-issue1839_-k-broken-in-interactive-selection.dpatch, unnamed
messages: + msg11073
2010-05-15 14:35:17darcswatchsetdarcswatchurl: http://darcswatch.nomeata.de/repo_http:__darcs.net_.html#bundle-bc71e36e15432f0938e559476bc306e9c8adce0a -> http://darcswatch.nomeata.de/repo_http:__darcs.net_.html#bundle-9d7fa15abb801c3546e769c3b649549cc0517ffd
2010-05-20 15:55:04koweysetnosy: + tux_rocker
messages: + msg11096
2010-05-22 14:07:42tux_rockersetassignedto: tux_rocker
2010-05-23 19:54:33tux_rockersetstatus: needs-review -> review-in-progress
2010-05-26 16:58:27tux_rockersetmessages: + msg11115
2010-05-26 17:12:34florent.beckersetnosy: + florent.becker
messages: + msg11116
2010-05-28 09:16:39galbollesetfiles: + resolve-issue1839_-k-broken-in-interactive-selection.dpatch, unnamed
messages: + msg11133
2010-05-28 09:19:55darcswatchsetdarcswatchurl: http://darcswatch.nomeata.de/repo_http:__darcs.net_.html#bundle-9d7fa15abb801c3546e769c3b649549cc0517ffd -> http://darcswatch.nomeata.de/repo_http:__darcs.net_.html#bundle-741080e4557e234063f9a5060ecf9652a68d721f
2010-05-28 20:27:22koweysetstatus: review-in-progress -> needs-review
messages: + msg11138
2010-06-02 17:00:49tux_rockersetmessages: + msg11192
2010-06-02 18:01:50darcswatchsetstatus: needs-review -> accepted
messages: + msg11193
2011-05-10 18:06:49darcswatchsetdarcswatchurl: http://darcswatch.nomeata.de/repo_http:__darcs.net_.html#bundle-741080e4557e234063f9a5060ecf9652a68d721f -> http://darcswatch.nomeata.de/repo_http:__darcs.net_reviewed.html#bundle-bc71e36e15432f0938e559476bc306e9c8adce0a
2011-05-10 20:36:37darcswatchsetmessages: + msg14289
2011-05-10 22:36:21darcswatchsetdarcswatchurl: http://darcswatch.nomeata.de/repo_http:__darcs.net_reviewed.html#bundle-bc71e36e15432f0938e559476bc306e9c8adce0a -> http://darcswatch.nomeata.de/repo_http:__darcs.net_reviewed.html#bundle-9d7fa15abb801c3546e769c3b649549cc0517ffd