Feature: (optional) UM can ignore identity changes emitted from stream

[JordanMartinez]

• also adds javadoc to UndoManagerFactory and UndoManager
• also uses default methods to reduce code duplication in the factory

Address #14

[JordanMartinez]

Since there's no Travis CI on here:

~/Git Projects/UndoFX $ git branch
* addIdentity
  master
  placeholderForRound2Work
  undo-redo
~/Git Projects/UndoFX $ gradle test
:undofx:compileJava
:undofx:processResources UP-TO-DATE
:undofx:classes
:undofx:compileTestJava
:undofx:processTestResources UP-TO-DATE
:undofx:testClasses
:undofx:test
:undofx:jar
:undofx-demos:compileJava
:undofx-demos:processResources UP-TO-DATE
:undofx-demos:classes
:undofx-demos:compileTestJava UP-TO-DATE
:undofx-demos:processTestResources UP-TO-DATE
:undofx-demos:testClasses UP-TO-DATE
:undofx-demos:test UP-TO-DATE

BUILD SUCCESSFUL

Total time: 1.455 secs

[JordanMartinez]

Actually, let me update the build file to resolve links to ReactFX properly, too.

[JordanMartinez]

Ok. Good to go.

[TomasMikula]

You forgot the main thing: test for identity after merging. This can be done in the private C[] merge(C c1, C c2) method, returning an empty array when the result of merge is identity.

Also, to reduce the number of constructors, a constructor should take isIdentity only if it also takes merge. When there is no merge, instead of passing in isIdentity, the user can just filter changeStream with !isIdentity.

[JordanMartinez]

You forgot the main thing: test for identity after merging. This can be done in the private C[] merge(C c1, C c2) method, returning an empty array when the result of merge is identity.

Sorry. Somewhere in the process, I thought the isIdentity only applied to the changes that are emitted by the eventstream, not the merge process. Re-reading through #14, I realize that's not the case. I'll fix it possibly tomorrow but definitely by next week.

[JordanMartinez]

Fixed.

On another thought, should there be a 3rd constructor with a merge parameter but no isIdentity parameter because it assumes that every emitted event and merged event is non-identity?

[TomasMikula]

Yes. That also preserves compatibility with the previous version.

[JordanMartinez]

Fixed

[JordanMartinez]

I've updated it to use the empty array approach now.

[JordanMartinez]

I've addressed your comments.

Additionally, I removed the javadoc for merge when isIdentity = c -> false because there's no point in mentioning the change annihilation part since that will never happen in those cases.

[JordanMartinez]

Since I haven't heard anything back from you about the documentation issue, I'd like to clarify some things to insure no offense was made and to insure this gets done. I initially removed the javadoc concerning the changeStream, apply, and invert parameters because I thought your comment implied it was pointless to document them. Then you later implied that documenting these parameters was worth it, and listed what could be documented. You also indicated another place where documentation could be improved merge. Since this PR was taking longer than expected (I take blame for misunderstanding and misreading), taking time away from papers I still need to write before my semester ends, and since you implied that additional documentation was needed but did not specify what exactly, so that I could write it quickly and easily, I thought that you should document it because it would meet your standards and be immediately merged whereas it might be delayed by miscommunication between us via comments of "this could be better documented" or "you forgot to do this." So, while I'm guessing you haven't responded due to being busy with other matters and haven't been offended, I want to insure I did not somehow offend you by my comment.

With that being said, I'd also guess that it would be inconvenient for you to literally document this yourself: you would need to add my repo, check out a local copy, document everything, submit the changes to my repo, where I would then have to merge it, and then you would merge this PR here. Thus, it would be better if I could fix the documentation and update this PR. Since I am unsure what I should write, I think it would be faster if you wrote the documentation via comments and I updated my PR to include those comments in the documentation. Your thoughts?

[TomasMikula]

Don't worry, no offense taken, just being busy ;)

I initially removed the javadoc concerning the changeStream, apply, and invert parameters because I thought your comment implied it was pointless to document them.

I only meant to imply that those specific comments were pointless (duplicating the code).

you implied that additional documentation was needed

You said you were going for completeness. I pointed out there was something worthwhile to document [if you really wanted to].

but did not specify what exactly, so that I could write it quickly and easily

I just wanted to give you the chance to figure it out yourself (if you wanted). If not, I can as well just merge (since I'm already happy with the code) and update comments myself afterwards.

[TomasMikula]

Good luck with your papers 😉

[JordanMartinez]

Ah, thanks for the clarification. That makes sense now.

I just wanted to give you the chance to figure it out yourself (if you wanted). If not, I can as well just merge (since I'm already happy with the code) and update comments myself afterwards.

Thanks for not taking away my opportunity to learn. While I think completeness is best, I probably won't be able to get to this until after my semester finishes (mid-may ish). So, the goal is to fix RTFX ASAP so that a new release without any regressions is made, then it makes sense to merge this now. If you're willing to wait, then I could do it after my semester finishes, but the context of our conversation will not be as fresh in my mind.

Good luck with your papers

Thank you sir!

[JordanMartinez]

Just fetched your new updates. Why did you add the nextToUndo/Redo? (And wouldn't nextUndo/Redo be easier to read than what you have?)

[TomasMikula]

Yeah, I guess maybe those would have been better names...

The motivation is to be able to peek at the next/previous change, so that one can display "Undo: Insert text", "Redo: Move object" as menu item labels or button tooltips.

[JordanMartinez]

The motivation is to be able to peek at the next/previous change, so that one can display "Undo: Insert text", "Redo: Move object" as menu item labels or button tooltips.

Mm.. Good idea.

Yeah, I guess maybe those would have been better names...

Maybe in UndoFX 1.3.2?

[TomasMikula]

Yeah, but I'm not going to make another release just for that.

[JordanMartinez]

Makes sense.