Copy Undo over from apptools by aaronayres35 · Pull Request #813 · enthought/pyface

aaronayres35 · 2020-11-25T22:03:10Z

As mentioned in step 6a of https://github.com/enthought/ets/blob/master/docs/source/eeps/eep-6.rst
this PR simply copies over the apptools.undo subpackage from apptools (and updates references to apptools in the code to reference pyface instead).

After the next pyface release we will want to deprecate apptools.undo in apptools.

codecov-io · 2020-11-25T22:12:32Z

Codecov Report

Merging #813 (012e27f) into master (0d1516e) will increase coverage by 0.43%.
The diff coverage is 88.97%.

@@            Coverage Diff             @@
##           master     #813      +/-   ##
==========================================
+ Coverage   40.81%   41.25%   +0.43%     
==========================================
  Files         508      522      +14     
  Lines       27845    28110     +265     
  Branches     4219     4251      +32     
==========================================
+ Hits        11366    11596     +230     
- Misses      15999    16018      +19     
- Partials      480      496      +16

Impacted Files	Coverage Δ
pyface/ui/wx/tree/tree.py	`0.00% <ø> (ø)`
pyface/workbench/workbench.py	`53.19% <50.00%> (ø)`
pyface/undo/action/command_action.py	`64.28% <64.28%> (ø)`
pyface/workbench/i_editor.py	`65.78% <66.66%> (ø)`
pyface/undo/undo_manager.py	`81.81% <81.81%> (ø)`
...yface/undo/action/abstract_command_stack_action.py	`86.66% <86.66%> (ø)`
pyface/undo/command_stack.py	`89.65% <89.65%> (ø)`
pyface/undo/action/redo_action.py	`91.66% <91.66%> (ø)`
pyface/undo/action/undo_action.py	`91.66% <91.66%> (ø)`
pyface/i_widget.py	`100.00% <100.00%> (ø)`
... and 40 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4751783...012e27f. Read the comment docs.

rahulporuri

Can you also move the documentation for undo from apptools to pyface?
Can you also move the examples for undo from apptools to pyface?
Can you check the test coverage for all of the new code? I think we will want to/need to add more tests.

rahulporuri · 2020-11-26T08:07:40Z

+
+    # This is the currently active command stack and may be None.  Typically it
+    # is set when some sort of editor becomes active.
+    active_stack = Instance("apptools.undo.api.ICommandStack")


not sure why we can't just import and use ICommandStack here instead of using the string.

There is a circular import, I think - UndoManager references ICommandStack and vice-versa.

rahulporuri · 2020-11-26T08:10:35Z

+
+    # This is the currently active command stack and may be None.  Typically it
+    # is set when some sort of editor becomes active.
+    active_stack = Instance("apptools.undo.api.ICommandStack")


It looks like IUndoManager and ICommandStack depend on one another - which is why we can't import and use ICommandStack here. Can you add a comment here regarding the same?

aaronayres35 · 2020-11-30T14:39:28Z

+
+
+.. _API: api/index.html
+.. _example: https://svn.enthought.com/enthought/browser/AppTools/trunk/examples/undo/


This link is clearly out of place here, but I believe it was actually previously broken on apptools as well. Likewise, this final section of "API Overview" seems unnecessary with auto built api docs. This file needs to be updated, but I think I will leave those changes for a separate PR unless a reviewer would like to see them done here, in which case I am happy to do so

Let's get them fixed in this PR instead of after the PR.

aaronayres35 · 2020-11-30T15:06:28Z

Can you check the test coverage for all of the new code? I think we will want to/need to add more tests.

Here is a screenshot of the coverage report:

Some of these would benefit from additional tests (looks like undo_manager specifically) as you mentioned. Should I try to write them as part of this PR, or do that in a subsequent PR @rahulporuri?

rahulporuri · 2020-11-30T16:44:36Z

Should I try to write them as part of this PR, or do that in a subsequent PR @rahulporuri?

Can you add those as part of this PR? IMO its not too much to review.

…ated api docs

…is is all available in auto generated api docs). Also remove broken links

aaronayres35 · 2020-11-30T21:35:54Z

pyface.undo.action is currently untested, but I am unsure on what the benefits would be of testing. The classes exposed by the api basically just have a perform method which calls a method on command_stack or undo_manager which are already tested. Also, testing if perform works is handled by the tests for Action. It feels incorrect to leave this sub package without tests, but I am unsure what specifically it would be useful to test (rather than just writing noisy tests for the sake of writing something).

One thing I see that may be beneficial is AbstractCommandStackAction has a method _on_stack_updated that simply calls self._update_action() when the active_stack is updated. This can change whether the actions are enabled, and their names, and probably warrants a test.

rahulporuri

Mostly LGTM with a few minor comments.

rahulporuri · 2020-12-01T09:17:05Z

+
+# Enthought library imports.
+from pyface.api import GUI, YES
+from pyface.workbench.api import Workbench


can you open an issue regd porting the undo example code from pyface workbench to pyface tasks?

Opened #821

Co-authored-by: Poruri Sai Rahul <rporuri@enthought.com>

rahulporuri · 2020-12-01T14:02:10Z

@aaronayres35 before you merge, can you update the coverage report?

aaronayres35 · 2020-12-01T14:11:37Z

Can you check the test coverage for all of the new code? I think we will want to/need to add more tests.

Here is a screenshot of the coverage report:

This is the updated coverage report after the added tests:

copy Undo over from apptools

d3f9133

rahulporuri reviewed Nov 26, 2020

View reviewed changes

rahulporuri mentioned this pull request Nov 30, 2020

Bug fix release 7.1.1? #819

Closed

aaronayres35 added 2 commits November 30, 2020 08:30

copy over undo examples from apptools

024e55a

copy over undo documentation from apptools

e6199b3

aaronayres35 commented Nov 30, 2020

View reviewed changes

aaronayres35 added 3 commits November 30, 2020 08:48

update copyright headers

11e3f4b

follow convention of instantiating traits for trait definitions

025fbc5

add comment about circular dependency workaround

61ffe41

aaronayres35 mentioned this pull request Nov 30, 2020

Deprecate apptools.undo after pyface 7.2.0 minor release enthought/apptools#243

Closed

aaronayres35 requested a review from rahulporuri November 30, 2020 16:27

aaronayres35 added 5 commits November 30, 2020 11:04

add #: to trait definitions comments so traits come up with autogener…

eba2f19

…ated api docs

remove unneeded redundant portion of docs mentioning API overview (th…

244c6be

…is is all available in auto generated api docs). Also remove broken links

getting the undo example working

8654a7b

undoing an unneeded change

3e16b61

adding more tests

8fcedc5

add some simple tests for undo.actions

a35c2f7

rahulporuri approved these changes Dec 1, 2020

View reviewed changes

aaronayres35 and others added 2 commits December 1, 2020 07:40

Apply suggestions from code review

76bdcc0

Co-authored-by: Poruri Sai Rahul <rporuri@enthought.com>

another suggestion from code review

012e27f

aaronayres35 mentioned this pull request Dec 1, 2020

Port Undo examples from workbench to tasks #821

Open

aaronayres35 merged commit 75709d0 into master Dec 1, 2020

aaronayres35 deleted the copy-undo-from-apptools branch December 1, 2020 14:12



		.. _API: api/index.html
		.. _example: https://svn.enthought.com/enthought/browser/AppTools/trunk/examples/undo/

Conversation

aaronayres35 commented Nov 25, 2020

Uh oh!

codecov-io commented Nov 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

rahulporuri left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rahulporuri Nov 26, 2020

Choose a reason for hiding this comment

Uh oh!

corranwebster Nov 30, 2020

Choose a reason for hiding this comment

Uh oh!

rahulporuri Nov 26, 2020

Choose a reason for hiding this comment

Uh oh!

aaronayres35 Nov 30, 2020

Choose a reason for hiding this comment

Uh oh!

rahulporuri Nov 30, 2020

Choose a reason for hiding this comment

Uh oh!

aaronayres35 commented Nov 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rahulporuri commented Nov 30, 2020

Uh oh!

aaronayres35 commented Nov 30, 2020

Uh oh!

rahulporuri left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rahulporuri Dec 1, 2020

Choose a reason for hiding this comment

Uh oh!

aaronayres35 Dec 1, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rahulporuri commented Dec 1, 2020

Uh oh!

aaronayres35 commented Dec 1, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-io commented Nov 25, 2020 •

edited

Loading

rahulporuri left a comment •

edited

Loading

aaronayres35 commented Nov 30, 2020 •

edited

Loading