darcs

Patch 2055 resolve issue2649: cleanup display of patches

Title resolve issue2649: cleanup display of patches
Superseder Nosy List bf
Related Issues
Status accepted Assigned To
Milestone

Created on 2020-07-16.13:01:16 by bf, last changed 2020-07-30.12:16:27 by ganesh.

Files
File name Status Uploaded Type Edit Remove
patch-preview.txt bf, 2020-07-16.13:01:15 text/x-darcs-patch
rename-_ss_ummary-to-_ww_ithsummary-in-darcs_ui__.dpatch bf, 2020-07-16.13:01:15 application/x-darcs-patch
unnamed bf, 2020-07-16.13:01:15 text/plain
See mailing list archives for discussion on individual patches.
Messages
msg22167 (view) Author: bf Date: 2020-07-16.13:01:15
The patch largely follows the plan of attack outlined in issue2649.

I am not screening it immediately since this is a change in behavior and I
can't rule out remaining bugs in the output during interactive patch
selection (which is something we have no tests for). As usual I will use it
for my daily work from now on so I guess I'll notice any misbehavior sooner
or later.

4 patches for repository http://darcs.net/screened:

patch a8d39968c2b3aa4642d8b4afbd85943511d27dfe
Author: Ben Franksen <ben.franksen@online.de>
Date:   Sun Jul 12 23:51:45 CEST 2020
  * rename [Ss]ummary to [Ww]ithSummary in Darcs.UI.*

patch 083ba698393d890d2700c0dad1fbc5c13a23f5ff
Author: Ben Franksen <ben.franksen@online.de>
Date:   Wed Jul 15 17:44:28 CEST 2020
  * resolve issue2649: cleanup display of patches

patch daaf9d19a310091f35eea498b0dc27cf20a5b0f3
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Jul 16 12:23:44 CEST 2020
  * RebaseChange: use nubSort instead of nub for listTouchedFiles
  
  I think we sort the list of files elsewhere, too.

patch 12fc6bcfb7374ecdc17aff51ade67828a5daebac
Author: Ben Franksen <ben.franksen@online.de>
Date:   Thu Jul 16 13:27:33 CEST 2020
  * D.UI.SelectChanges: remove unused exports, rename showCur to printCurrent
Attachments
msg22223 (view) Author: ganesh Date: 2020-07-20.23:58:13
I don't have any objection to the principle of this, though I wonder
if we really need it for 2.16 or not.
msg22227 (view) Author: bf Date: 2020-07-21.09:51:20
> I don't have any objection to the principle of this, though I wonder
> if we really need it for 2.16 or not.

We don't. But the RebaseChange fixes should go into 2.16 because the
output with -s and -v is pretty unreadable ATM i.e. w/o the patch info.
Disentangling that part from the behavior change is possible, but not
trivial. If we can agree that the change goes into 2.16 then I don't
have to spend that effort.
msg22258 (view) Author: ganesh Date: 2020-07-25.23:21:42
> We don't. But the RebaseChange fixes should go into 2.16 because the
> output with -s and -v is pretty unreadable ATM i.e. w/o the patch info.
> Disentangling that part from the behavior change is possible, but not
> trivial. If we can agree that the change goes into 2.16 then I don't
> have to spend that effort.

OK, I'm fine with it for 2.16 if you are reasonably happy it behaves well.
msg22264 (view) Author: bf Date: 2020-07-27.07:24:29
> OK, I'm fine with it for 2.16 if you are reasonably happy it behaves well.

I haven't observed any misbehavior yet.

However, examining the behavior more carefully than usual made me
realize that there is a missing feature here. Suppose you work
interactively and encounter a large patch, which you examine more
closely, using the 'v' or 'p' key. Depending on the default settings for
your pager this may mean that the patch description is now lost
somewhere far up in the terminal. It would be nice if we had another key
('r' for "repeat" or "re-print"?) that just prints the current patch in
plain format (i.e. non-verbose, no summary) without navigating to a
different patch.

I will screen this bundle and send another one with the above feature added.
msg22285 (view) Author: ganesh Date: 2020-07-30.08:46:28
>   * rename [Ss]ummary to [Ww]ithSummary in Darcs.UI.*

Fine.

>   * resolve issue2649: cleanup display of patches

> Logically, 'description' should default
> -- to 'mempty' while 'content' should default to 'displayPatch'. We define them
> -- the other way around so that 'Darcs.UI.PrintPatch.showFriendly' gives
> -- reasonable results for all patch types.

Is it effectively a future TODO to refactor this further, or is it
fundamental? (The answer might be a useful comment either way)

> showWithSummary :: ShowPatch p => p wX wY -> Doc
> showWithSummary p = description p $$ prefix "    " (summary p)

Given the above defaults, doesn't that mean that showWithSummary might
print out the whole patch followed by a summary of it?

Otherwise the patch looks fine. I haven't thought through each change in
detail but I like the principle of the introduction of 'content'.

>   * RebaseChange: use nubSort instead of nub for listTouchedFiles
>   * D.UI.SelectChanges: remove unused exports, rename showCur to printCurrent

Fine.
msg22287 (view) Author: bf Date: 2020-07-30.10:07:55
>>   * resolve issue2649: cleanup display of patches
> 
>> Logically, 'description' should default
>> -- to 'mempty' while 'content' should default to 'displayPatch'. We define them
>> -- the other way around so that 'Darcs.UI.PrintPatch.showFriendly' gives
>> -- reasonable results for all patch types.
> 
> Is it effectively a future TODO to refactor this further, or is it
> fundamental? (The answer might be a useful comment either way)

I find it quite hard to decide this, which is why I haven't elaborated
this point.

On the one hand, yes, the defaults are unintuitive, if we regard them
out of context. On the other hand, in the context of how they are used
for showFriendly and the other functions in Darcs.UI.PrintPatch, the
default definitions are the only ones that make sense.

I can't think of a possible refactor that "fixes" this, but I don't want
to completely rule out the possibility either.

>> showWithSummary :: ShowPatch p => p wX wY -> Doc
>> showWithSummary p = description p $$ prefix "    " (summary p)
> 
> Given the above defaults, doesn't that mean that showWithSummary might
> print out the whole patch followed by a summary of it?

A good point. Yes, it means exactly that. I have actually thought about
this case and could not find a principle fault with it.

We don't encounter this case in practice, though. The only place where
we call that function directly is in setEnvDarcsPatches which works only
with PatchInfAnd. showFriendly/printFriendly may hit this case but again
the only call site outside of Darcs.UI.Selectpatches is in
Darcs.UI.Commands.Util and Dracs.UI.Commands.Log where it gets passed a
PatchInfoAnd. That leaves interactive selection in
Darcs.UI.Selectpatches when used for prim patches. But none of the
commands that may involve interactive selection of prim patches accept
the --summary option; except whatsnew and that implements its own
interactive procedure (and that calls neither showFriendly nor
printFriendly).
msg22288 (view) Author: ganesh Date: 2020-07-30.10:31:33
OK, thanks for the explanations. I'll leave it to you to decide whether
to follow up with any clarification comments or similar.
History
Date User Action Args
2020-07-16 13:01:16bfcreate
2020-07-20 23:58:13ganeshsetmessages: + msg22223
2020-07-21 09:51:20bfsetmessages: + msg22227
2020-07-25 23:21:42ganeshsetmessages: + msg22258
2020-07-27 07:24:31bfsetmessages: + msg22264
2020-07-27 07:29:08bfsetstatus: needs-screening -> needs-review
2020-07-30 08:46:23ganeshsetstatus: needs-review -> review-in-progress
2020-07-30 08:46:30ganeshsetmessages: + msg22285
2020-07-30 10:07:56bfsetmessages: + msg22287
2020-07-30 10:31:35ganeshsetstatus: review-in-progress -> accepted-pending-tests
messages: + msg22288
2020-07-30 12:16:27ganeshsetstatus: accepted-pending-tests -> accepted