Skip to content

Notify listeners when days off are removed, and add the missing removal methods - #2830

Open
Natalie-the-technician wants to merge 2 commits into
bardsoftware:masterfrom
Natalie-the-technician:daysoff-removal-notifies
Open

Notify listeners when days off are removed, and add the missing removal methods#2830
Natalie-the-technician wants to merge 2 commits into
bardsoftware:masterfrom
Natalie-the-technician:daysoff-removal-notifies

Conversation

@Natalie-the-technician

Copy link
Copy Markdown
Contributor

What is wrong

HumanResource.addDaysOff() has no counterpart. The only way to take an absence
away is to reach into the DefaultListModel that getDaysOff() hands out and
mutate it directly, and nothing observes that model. Adding an absence and
removing one are therefore not the same kind of operation: adding resets the
cached LoadDistribution and fires fireResourceChanged(), removing does
neither.

Steps to reproduce

  1. Create a resource and give it exactly one day off. Press OK.
  2. Open the Resources Chart. The absence is drawn as a yellow block.
  3. Open the resource again, delete the day off, press OK.

Expected: the block disappears, and anything listening to the resource model is
told that something changed.

Actual: the block stays, and no resourceChanged event is fired at all.

Cause

GanttDialogPerson.applyChanges() clears the list before writing the edited
intervals back:

person.getDaysOff().clear();
for (DateInterval interval : myDaysOffModel.getIntervals()) {
  person.addDaysOff(new GanttDaysOff(interval.getStart(), interval.getEnd()));
}

With at least one interval left, the last addDaysOff() repairs both by
accident. Delete the last absence and the loop body never runs, so nothing
calls resetLoads() and nothing fires. resetLoads() drops the memoised
myLoadDistribution, which is the only cache of the computed absences — that is
why the chart keeps drawing the old picture.

That dialog line is the symptom rather than the defect. A fix that only makes
that one line fire leaves the getter handing out an unobserved mutable model,
and the next caller that removes an absence — a batch edit, an importer, a
script — reintroduces the same gap.

What this changes

1. HumanResource observes its own days off list. A ListDataListener on
myDaysOffList calls onDaysOffChanged(), which does resetLoads() and
fireResourceChanged(). Every mutation through getDaysOff() is covered at
once, including the removal of the last interval, which used to notify nobody.
addDaysOff() drops its own resetLoads()/fireResourceChanged() so that it
does not fire twice.

This is safe for the copying constructor: areEventsEnabled is false there
before the copy loop runs, and instance initialisers run before the constructor
body, so the listener is already installed and stays silent for the copy.

2. The two missing counterparts, in the shape HumanResourceManager already
uses for its own list (add / remove / clear):

boolean removeDaysOff(GanttDaysOff gdo)
void clearDaysOff()

removeDaysOff returns whether anything was removed, which is also the answer to
"did the listeners hear about it" — the list stays silent when the interval was
not there. It is clearDaysOff rather than a bare clear() because a resource
holds assignments and custom properties too.

GanttDialogPerson.applyChanges() now calls clearDaysOff() instead of
getDaysOff().clear(). No production code writes to the handed-out list any
more. getDaysOff() itself is left exactly as it was — signature, return type
and body — so every reader of it is unaffected.

One caveat, documented on the method and pinned by a test rather than changed
here: removeDaysOff matches by Object.equals, the way the list matches.
GanttDaysOff only overloads equals(GanttDaysOff) and does not override
equals(Object), so an interval built afresh from the same two dates is not the
one the resource holds. Callers should pass an instance obtained from the
resource.

The number of notifications does not change

Clearing and then writing N intervals back still costs (M > 0 ? 1 : 0) + N
resourceChanged events, where M is the number of intervals before OK, because
clearDaysOff() calls the very same DefaultListModel.clear() the dialog used
to call itself — one event for the whole removal, none at all on an empty list.
The single event this adds is exactly the one that was missing in the "delete the
last absence" case.

Tests

HumanResourceDaysOffTest

  • removing the last day off notifies the listeners just as adding it does

HumanResourceRemoveDaysOffTest

  • removing a single day off takes it off the resource and notifies once
  • removing a day off the resource does not have changes nothing and notifies nobody
  • clearing the days off empties the list and notifies once, whatever the count
  • clearing an empty list of days off notifies nobody
  • the dialog's clear-all-and-rewrite costs the same notifications either way
    walks every combination of M and N in 0..3 through both the old route and
    clearDaysOff() and pins the absolute event counts, so an implementation that
    fired once per interval would be caught.

Without the first change the first test fails with
expected: <1> but was: <0>. ./gradlew test --continue is green on the branch;
the six tests above are the only ones added and no existing test changes.

HumanResource has addDaysOff() but no counterpart: the only way to remove
an absence is to mutate the DefaultListModel handed out by getDaysOff(),
and nothing observes that model. Adding resets the cached LoadDistribution
and fires; removing does neither.

The resource properties dialog clears the list before writing the edited
intervals back. With intervals left the last addDaysOff() repairs both by
accident; delete the LAST absence and no event is fired at all. Measured on
screen: the absence keeps being drawn on the resource chart while the
dialog's list is already empty.

Fixed at the root rather than at the call site -- the resource now observes
its own list, so every mutation through getDaysOff() resets the loads and
notifies, including callers added later. addDaysOff() drops its own
resetLoads()/fireResourceChanged() so that it does not fire twice.

Safe for the copying constructor: areEventsEnabled is false there before
the copy loop runs, and instance initialisers run before the constructor
body, so the listener is installed and stays silent.
A day off could be given to a resource through addDaysOff(), but there was no
way to take one away again: the only route was to reach into the
DefaultListModel handed out by getDaysOff() and mutate it. That is what the
resource properties dialog did, and it left HumanResource with a collection that
has an entry point but no exit.

Add the two missing counterparts, following the shape HumanResourceManager
already uses for its own list of resources (add / remove / clear):

  boolean removeDaysOff(GanttDaysOff) -- take a single interval away
  void clearDaysOff()                 -- take all of them away

removeDaysOff returns whether anything was removed, because that is also the
answer to "did the listeners hear about it": the list stays silent when the
interval was not there. Without the return value a caller would have to look
into the handed-out list again, which is the thing being avoided. It is named
clearDaysOff rather than plain clear() because a resource holds assignments and
custom properties too.

Matching is by Object.equals, the way the list itself matches. GanttDaysOff only
overloads equals(GanttDaysOff) and does not override equals(Object), so an
interval built afresh from the same two dates is not the one the resource holds.
That is documented on the method and pinned by a test rather than changed here.

GanttDialogPerson.applyChanges() now calls clearDaysOff() instead of
getDaysOff().clear(). No production code writes to the handed-out list any more.

The number of notifications does not change: clearing and then writing N
intervals back still costs (M > 0 ? 1 : 0) + N resourceChanged events, because
clearDaysOff() calls the very same DefaultListModel.clear() the dialog used to
call itself. A test walks every combination of M and N in 0..3 through both
routes and pins the absolute counts, so a future implementation that fires once
per interval would be caught.

getDaysOff() is deliberately left exactly as it was -- signature, return type and
body. Narrowing it is a separate change; the list it hands out is still mutable,
so the old route remains open to anyone who takes it.
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.

1 participant