Skip to content

bug/issue 49: Fix findings described in ussue-49 - #50

Open
philliplbryant wants to merge 4 commits into
Col-E:masterfrom
philliplbryant:bug/issue-49
Open

philliplbryant wants to merge 4 commits into
Col-E:masterfrom
philliplbryant:bug/issue-49

Conversation

@philliplbryant

@philliplbryant philliplbryant commented Sep 14, 2026

Copy link
Copy Markdown
Contributor
  • Replaced reference-equality comparisons with .equals()/Objects.equals().
  • Simplified the redundant instanceof check to a null check.
  • Guarded @Nullable field dereferences.
  • Extracted DragUtils's dragBoard splitting into a shared helper function that uses a negative split limit, so an empty trailing field (e.g. an empty dockable identifier with no drop target) is no longer silently dropped.
  • Added unit test coverage for DragUtils's shared helper function.
  • Return the uncollapsed leaf state when the parent is null to prevent NPE.
  • Added unit test coverage for DockContainerLeaf's null parent issue.
  • Removed disallowed self-closing <p/> tags.
  • Added TestFX and Monocle test dependencies to core and to run JavaFX tests headless without a display

Of the findings, only the DragUtils splitting behavior and DockContainerLeaf's null parent issue were testable. None of the compared types in the other findings override equals(), so those fixes are preventative.

toggleCollapse now returns the unchanges state instead of throwing NPE.

When drag drop content ends in an empty identifier, extract identifier now returns an empty dockable identifier string instead of null.

Similarly, guards for @Nullable field dereferences are preventative with no observable changes in behavior. Other changes have no observable changes in behavior.

Phil Bryant and others added 4 commits September 14, 2026 14:53
* Replaced reference-equality comparisons with `.equals()`/`Objects.equals()`.
* Simplified the redundant `instanceof` check to a null check.
* Guarded `@Nullable` field dereferences.
* Extracted `DragUtils`'s `dragBoard` splitting into a shared helper function that uses a negative split limit, so an empty trailing field (e.g. an empty dockable identifier with no drop target) is no longer silently dropped.
* Added unit test coverage for `DragUtils`'s shared helper function.
* Remove disallowed self-closing `<p/>` tags.
* Fix an additional nullability issue.
* update Javadoc configurations remaining from a separate issue.
* Run JavaFX tests headless with Monocle so they pass on CI runners without a display.
Comment on lines -152 to +154
if (leaf.getParentContainer() != root && leaf.getParentContainer() != null)
if (!Objects.equals(leaf.getParentContainer(), root) &&
leaf.getParentContainer() != null)

@Col-E Col-E Sep 18, 2026

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A lot of these identity checks were intentional and should be kept, not converted to equals.

The check here and in many other cases is essentially "Is my parent the root?" - An identity check is much faster and will always yield the correct results in the current model. There is no scenario where an equality check would positively impact correctness. Equality checking is also much slower depending on the equals implementation (And if the impl checks children and they check theirs, potentially horrendous performance)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since a lot of the changes boil down to the same thing I won't comment on each case this happens for.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants