gmethvin / directory-watcher

A cross-platform Java recursive directory watcher, with a JNA macOS watcher and Scala better-files integration
Apache License 2.0
264 stars 34 forks source link

Optional onIdle(count) method added #76

Closed treblereel closed 3 years ago

treblereel commented 3 years ago

In some cases it's very useful to be able to get a change set after some predefined period, for example, when we use APT and we want to get a list of generated files and we know that the whole generation stage usually takes 500ms. In this PR i introduce the simple mechanism that calls an onIdle method after timeout expired and passes the number of events within that timeout.

@mdproctor ^^

gmethvin commented 3 years ago

Also I think for the idle timeout you could use ScheduledExecutorService:

ScheduledExecutorService scheduledExecutor = Executors.newSingleThreadScheduledExecutor();
ScheduledFuture<?> idleFuture = null;

void onIdle(int count) { // called when we start waiting for new events
  idleFuture = scheduledExecutor.schedule(() -> { onIdleTimedOut(count); }, idleTimeout, idleTimeoutUnit);
}

void onActive(int count) { // called when we start getting events again
  if (idleFuture != null) {
    idleFuture.cancel(true);
    idleFuture = null;
  }
}

void onIdleTimedOut(int count); // call this when the timeout has passed after `onIdle`.

Also with ScheduledExecutorService it'd be easier to keep track of multiple timeout tasks if you want to do that.

treblereel commented 3 years ago

I have switched to ScheduledExecutorService and transfered all OnIdle logic into ChangeSetListener, is it closer to what we want ?

As you can see, i have implemented additional ChangeSetListener constructor that accepts timeout and consumer, because current ChangeSetListener doesn't allow to subscribe to onIdle(int) directly.

wdyt ?

treblereel commented 3 years ago

@gmethvin if i am not mistaken, Mark mentioned that this functionality can be moved from DirectoryWatcher to ChangeSetListener. We are planning to use OnIdle functionality with ChangeSetListener, but, to be honest, i can't say, i am happy with my current implementation, adding listener to listener doesn't look nature.

So if you have any ideas how to improve it i ll be happy to implement it

gmethvin commented 3 years ago

@mdproctor @treblereel I'm just trying to understand the overall use case so we can design the best API. This is why I'm asking how it's meant to be used. As it stands right now, it appears the idle timeout functionality could be a separate listener type rather than part of ChangeSetListener.

Is the idea that you want ChangeSetListener to retrieve a new change set the next time the watcher is idle? In that case, for example, you could have an API that blocks until idle and retrieves the change set of all changes since the last time the watcher was idle. On its own it just seems awkward to have the idle listener as part of ChangeSetListener since it doesn't seem directly related to rest of the functionality.

mdproctor commented 3 years ago

I think you are right. While the onIdle notification needs to be in core. We can move the timeout notification to it's own listener. We can then ask users to add the the pair of listeners (IdleTimeOutListener, ChangeSetListener), via composite holder. Keeping the timer logic in it's own class like that, will mean easier to maintain and test code.

gmethvin commented 3 years ago

@mdproctor I think either way there's a use case for a helper like DirectoryChangeListener.fromListeners(listeners).

In this case though I was wondering if we should have something like DirectoryChangeListener.withIdleDelay(timeoutMillis, listener), which would just delay the call to listener.onIdle with your timeout code. Not totally set on that API though.

gmethvin commented 3 years ago

@mdproctor @treblereel I merged this along with 05c155e02871a7b6f6c73a11d58b001fd9f38ed7 which addresses my comments. Let me know if that works.