This is an archive of the discontinued Mercurial Phabricator instance.

undo: preview interactive ui
ClosedPublic

Authored by felixmerk on Aug 7 2017, 3:32 PM.
Tags
None
Subscribers

Details

Reviewers
stash
quark
Group Reviewers
Restricted Project
Commits
rFBHGXfc85e73e3882: undo: preview interactive ui
Summary

Provide interactive preview. undo -p now provides support of using arrow keys
and return to select state. The interactive ui is set up to be generic so
potentially could be used for other commands.

Test Plan
  1. Running a bash script that sets up an interesting repo
  2. hitting hg undo -p and trying every key combination Specifically: control-C still works, q quits, left goes forward, right goes backwards, return brings you to a state
  3. running redo after preview and return
  4. running undo a few times and then using undo -p to go back forward
  5. going out of bounds both up and down
  6. hitting other random keys during hg undo -p
  7. running hg undo -p with other flags

Diff Detail

Repository
rFBHGX Facebook Mercurial Extensions
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

felixmerk created this revision.Aug 7 2017, 3:32 PM
Herald added a reviewer: Restricted Project. · View Herald TranscriptAug 7 2017, 3:32 PM
stash requested changes to this revision.Aug 8 2017, 5:18 AM
stash added a subscriber: stash.

Looks really good!

Small nits + can you provide the test plan?

hgext3rd/interactiveui.py
23–24

I'm not sure, but do we need to worry about licensing?

35–36

If this error happened, then ch may not be initialized.

This revision now requires changes to proceed.Aug 8 2017, 5:18 AM

Current test plan involves doing the following:

  1. Running a bash script that sets up an interesting repo
  2. hitting hg undo -p and trying every key combination Specifically: control-C still works, q quits, left goes forward, right goes backwards, return brings you to a state
  3. running redo after preview and return
  4. running undo a few times and then using undo -p to go back forward
  5. going out of bounds both up and down
  6. hitting other random keys during hg undo -p
  7. running hg undo -p with other flags. Note that --branch doesn't give you the correct preview
hgext3rd/interactiveui.py
23–24

I honestly don't know much about licensing. The code seems to be licensed under BSD.

felixmerk edited edge metadata.Aug 8 2017, 6:55 PM
felixmerk updated this revision to Diff 655.
quark added a subscriber: quark.Aug 8 2017, 7:35 PM

Will review later. Test Plan could be moved to commit message by having a "Test Plan:" section header.

hgext3rd/interactiveui.py
23–24

I would copy the entire https://github.com/pallets/click/blob/master/LICENSE file here as a block comment (as requested by the license - "Redistributions of source code must retain the above copyright notice, this list of conditions and the following disclaimer.", plus a short comment saying that we made some changes to it).

felixmerk edited the test plan for this revision. (Show Details)Aug 8 2017, 7:56 PM
felixmerk updated this revision to Diff 657.
stash accepted this revision.Aug 9 2017, 10:28 AM
stash added inline comments.
hgext3rd/interactiveui.py
111–122

Why do you need str(repr(...))?

hgext3rd/undo.py
639–642

going out of bounds both up and down

What's the expected behaviour in that case?

This revision is now accepted and ready to land.Aug 9 2017, 10:28 AM
felixmerk added inline comments.Aug 9 2017, 2:21 PM
hgext3rd/interactiveui.py
111–122

That does seem rather redundant ...

hgext3rd/undo.py
639–642

A blank output

felixmerk updated this revision to Diff 711.Aug 9 2017, 2:21 PM
quark accepted this revision.Aug 9 2017, 9:24 PM

Maybe move the license to before getchar, since the name getchar is also from there.

hgext3rd/interactiveui.py
8

Better to add

from __future__ import absolute_import

here.

felixmerk updated this revision to Diff 740.Aug 10 2017, 1:55 PM
This revision was automatically updated to reflect the committed changes.