Add redo/undoStackSize properties to track stack sizes #53

jperedadnr · 2022-02-21T20:31:08Z

Fixes #50
For now, only size is exposed. Later on we could even expose the list of commands to undo/redo.

abhinayagarwal · 2022-02-22T05:43:52Z

Is there an advantage of exposing the entire size of the queues rather than exposing just boolean properties which return the state of the queues?

jperedadnr · 2022-02-22T08:34:19Z

Not really. For now all that matters indeed is if the stack is empty or not, and that can be a boolean. Later on we can think of exposing a list of strings with the commands to undo/redo.

jperedadnr · 2022-02-22T12:06:41Z

@abhinayagarwal I've updated the PR with boolean instead of integer.

I've also realized that the stackEmpty properties are needed on the modelView only, which has another commandManager, and not in the PieceTable. This misunderstanding is what caused #52 ...

abhinayagarwal

Since the PR updates CommandManager, a few tests with new functionality on CmdManagerTests would be nice.

src/main/java/com/gluonhq/richtext/RichTextArea.java

src/main/java/com/gluonhq/richtext/model/PieceTable.java

src/main/java/com/gluonhq/richtext/viewmodel/RichTextAreaViewModel.java

src/main/java/com/gluonhq/richtext/undo/CommandManager.java

eugener · 2022-02-22T21:55:49Z

src/main/java/com/gluonhq/Main.java

@@ -91,14 +91,19 @@ public Double fromString(String s) {
        CheckBox editableProp = new CheckBox("Editable");
        editableProp.selectedProperty().bindBidirectional(editor.editableProperty());

+        Button undoButton = actionButton(LineAwesomeSolid.UNDO, editor.getActionFactory().undo());
+        undoButton.disableProperty().bind(editor.undoStackEmptyProperty());


Not for this PR... but we should not expose this here. We should have enabled property on the Action which we can then bind to control in the demo. That property would be internally controlled by the state of the control. For example, hypothetically, if no selection, text decoration actions will be disabled, disabling related controls.

Sounds good.
So far the interface Action requires the viewModel as an argument for the apply method, but for the disabled/enabled property it doesn't seem the right approach? Would an abstract class be more practical, given that we could create each action with the viewModel as a constructor argument, that can be later on used by all the action methods?

Very much possible... I was basically trying to create actions without state, i.e. they learn about viewModel only at execution time. Action properties would require state, so passing viewModel would probably be the only choice.
The downside of this is that all actions would have to be recreated, if viewModel changes.
We could probably avoid that if, instead of passing model into it, we bind them somehow. I worry about that simply because, in my view, the model(text) will have to change, since control has to have that functionality.

I'll file an issue for this so it can be done in a follow-up PR

@eugener if the viewModel is going to change, then we should have full support for that in the skin.

This also leads to the fact that the control should be able to change the model/original text, and create a new viewModel out of this.

For instance, after a "File->New" (or alike), the model should have an empty original text, or after "File->Open", the model should have the original text and decorations.

I'll file another issue for this.

I'm not sure view model has to change if we redesign it a little. Currently it holds the text buffer. But if we can have text buffer as a property, then we can reassign it without changing the viewModel itself. ViewModel will react on text buffer changing, triggering changes in the view.
So... changing a "document" on control will in essence change the text buffer bound to viewModel.

Let's move this conversation to either #59 or #60

jperedadnr · 2022-02-23T11:24:40Z

@abhinayagarwal I've added a test

abhinayagarwal · 2022-02-23T11:41:38Z

src/test/java/com/gluonhq/richtext/undo/CmdManagerTests.java

+    private void update() {
+        emptyUndo.set(commander.isUndoStackEmpty());
+        emptyRedo.set(commander.isRedoStackEmpty());
+    }


There is no need for this method. All we need to do here is to execute TestCommand, undo and redo. Assert the state of the stacks using the new public methods.

It actually tests the new constructor with a Runnable

Then it needs a separate test :)

abhinayagarwal · 2022-02-23T11:44:19Z

src/test/java/com/gluonhq/richtext/undo/CmdManagerTests.java

+
+    @Test
+    @DisplayName("Passes multiple redo rules")
+    public void multipleRedoRules() {


This tests needs to be broken into at least 3 different tests:

Execute TestCommand and assert the size of both the stacks using the new methods.

Execute TestCommand followed by a undo. Assert the size of stacks.

Execute TestCommand. Fire undo followed by a redo. Assert the size of stacks.

Do you think I could simply add the asserts to the three existing tests:

public void executionRules() { ... Assertions.assertEquals( 1, commander.undoStack.size()); Assertions.assertEquals( 0, commander.redoStack.size()); + Assertions.assertTrue(commander.isUndoStackEmpty()); + Assertions.assertTrue(commander.isRedoStackEmpty()); Assertions.assertEquals( "Text-redo", commander.context.toString()); }

Yup, that's exactly what I was suggesting. Instead of asserting against emptyUndo and emptyRedo fields, we can directly assert the new methods which returns the state of the underlying stacks.

src/test/java/com/gluonhq/richtext/undo/CmdManagerTests.java

abhinayagarwal · 2022-02-23T14:19:33Z

src/test/java/com/gluonhq/richtext/undo/CmdManagerTests.java

+    @DisplayName("runnable called when redo is executed")
+    public void runnableIsCalledWhenRedoIsExecuted() {
+        StringBuilder text = new StringBuilder("Text");
+        AtomicBoolean aBoolean = new AtomicBoolean();


The AtomicBoolean can be a AtomicInteger and we could increment it in every call to the runnable, therefore being able to test the value as 1, 2 and 3 in runnableIsCalledWhenCommandIsExecuted, runnableIsCalledWhenUndoIsExecuted and runnableIsCalledWhenRedoIsExecuted respectively. I leave it for you to decide.

Makes sense, you would be testing that runnable is called on every command operation

… into 50-stack

abhinayagarwal

Overall the PR LGTM

abhinayagarwal · 2022-02-24T08:35:13Z

src/test/java/com/gluonhq/richtext/undo/CmdManagerTests.java

+        commander.execute(new TestCommand());
+        Assertions.assertEquals(1, aInteger.get());
+        commander.undo();
+        Assertions.assertEquals(2, aInteger.get());


Nitpicking here, but Assertion of value 1 and 2 needs to be a part of the two previous tests. We should perform command.execute, undo and redo. Finally, assert the value of aInteger to 3

abhinayagarwal · 2022-02-24T08:35:27Z

src/test/java/com/gluonhq/richtext/undo/CmdManagerTests.java

+    }
+
+    @Test
+    @DisplayName("runnable called when commands are executed")


s/command are/redo is

Add redo/undoStackSize properties to track stack sizes

678d4b2

jperedadnr requested review from eugener and abhinayagarwal February 21, 2022 20:31

Merge branch 'master' into 50-stack

6887253

Use boolean and move stackEmpty properties to viewModel only

62a1fe6

abhinayagarwal reviewed Feb 22, 2022

View reviewed changes

Address feedback

04a49ef

eugener previously approved these changes Feb 22, 2022

View reviewed changes

Add test

a11967d

jperedadnr dismissed eugener’s stale review via a11967d February 23, 2022 11:22

Merge branch 'master' into 50-stack

3d04a81

abhinayagarwal reviewed Feb 23, 2022

View reviewed changes

Rework tests

5be27d4

abhinayagarwal reviewed Feb 23, 2022

View reviewed changes

src/test/java/com/gluonhq/richtext/undo/CmdManagerTests.java Outdated Show resolved Hide resolved

Rework tests

d36b59b

abhinayagarwal force-pushed the 50-stack branch from 5db503f to d36b59b Compare February 23, 2022 14:16

abhinayagarwal reviewed Feb 23, 2022

View reviewed changes

jperedadnr added 2 commits February 23, 2022 17:18

Merge branch '50-stack' of https://github.com/jperedadnr/rich-text-area…

e234153

… into 50-stack

Change test

f7c3107

jperedadnr requested review from eugener and abhinayagarwal February 24, 2022 08:30

abhinayagarwal previously approved these changes Feb 24, 2022

View reviewed changes

Update tests

c4c16db

jperedadnr dismissed abhinayagarwal’s stale review via c4c16db February 24, 2022 08:45

Fix tests

5335bde

abhinayagarwal approved these changes Feb 24, 2022

View reviewed changes

jperedadnr merged commit 0989e1a into gluonhq:master Feb 24, 2022

jperedadnr deleted the 50-stack branch February 24, 2022 09:22

jperedadnr mentioned this pull request Feb 24, 2022

Introduce FaceModel and change ViewModel when needed #61

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add redo/undoStackSize properties to track stack sizes #53

Add redo/undoStackSize properties to track stack sizes #53

jperedadnr commented Feb 21, 2022

abhinayagarwal commented Feb 22, 2022

jperedadnr commented Feb 22, 2022

jperedadnr commented Feb 22, 2022

abhinayagarwal left a comment

eugener Feb 22, 2022

jperedadnr Feb 22, 2022

eugener Feb 22, 2022

jperedadnr Feb 23, 2022

jperedadnr Feb 23, 2022

eugener Feb 24, 2022

jperedadnr Feb 24, 2022

jperedadnr commented Feb 23, 2022

abhinayagarwal Feb 23, 2022

jperedadnr Feb 23, 2022

abhinayagarwal Feb 23, 2022

abhinayagarwal Feb 23, 2022

jperedadnr Feb 23, 2022

abhinayagarwal Feb 23, 2022

abhinayagarwal Feb 23, 2022

jperedadnr Feb 23, 2022

jperedadnr Feb 23, 2022

abhinayagarwal left a comment

abhinayagarwal Feb 24, 2022

abhinayagarwal Feb 24, 2022

Add redo/undoStackSize properties to track stack sizes #53

Add redo/undoStackSize properties to track stack sizes #53

Conversation

jperedadnr commented Feb 21, 2022

abhinayagarwal commented Feb 22, 2022

jperedadnr commented Feb 22, 2022

jperedadnr commented Feb 22, 2022

abhinayagarwal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jperedadnr commented Feb 23, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhinayagarwal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment