search

linuxha / node-red-contrib-mytimeout

A dynamic count down timer for node-red
MIT License
10 stars 10 forks source link

Pause? #13

Open readeral opened 5 years ago

readeral commented 5 years ago

What's your recommended method of pausing? The 'restarting' obviously can (essentially) become a new timer with the value of the timer when it stopped, but 'stop' setting payload to 0 seems a little counterintuitive to me.

Would you consider 'Stop' retain the payload for restarting, feeding that payload value back in to the node again upon a trigger? Alternatively, would you consider a "pause" action? Would be nicer than needing to maintain my own state of the timer external to the timeout node.

Otherwise Stop and Cancel are essentially the same, just one notifies you about it (on the first output), and the other doesn't.

linuxha commented 5 years ago

I hadn't considered a pause. At the moment I'm not sure if it could be added (because I haven't had a chance to give it some thought).

If the feature is added, the "pause" command would be used, not sure how to restart it yet (yes restart might seem obvious). I'll need to give it some thought. The Stop and Cancel have a separate use and will remain as is.

What would the output be for output one? It currently sends "on", "off" (or the appropriate safe/unsafe string) and stop. Cancel is not seen on output one, Also what to send on output 2?

To work around this for now, I can recommend grabbing the output from output 2

readeral commented 5 years ago

I'll see what I can put together in a pull request for you

readeral commented 5 years ago

I've opened a pull request. Hasn't had any problems for me.

I have had to make a structural change however. Rather than having a constant running setInterval, it's now started and stopped on the on() and off() (and pause()) functions - otherwise the timer would continue to count down despite the pause. This is because the TIX event listener function contained the logic for decrementing the timer (ticks), and Stop() influenced this logic by setting ticks to 0.

Although TIX still contains that logic, whether a payload is sent is now first determined by whether there is an active tick Interval or not (and so whether the TIX event is triggered in the first place).

Given this change there's probably an opportunity for simplifying the code somewhat, but I've left that alone.

Major release? I wouldn't necessarily consider this a breaking change, given that the input and output messages remain the same, aside from the extending of possible values, but anyone who was somehow relying on that constant Interval (not sure how given it's not exposed) may have concerns.

I would however consider this a breaking change in one area, and that is in initialising the timer. In the current iteration there is (I think... I can't recall now) a one second delay before the timer actually decrements, which has now been changed, as on calling startInterval() an initial event TIX is emitted before the Interval starts. For anyone who has timers specific enough to have programmed in compensation for this, it would be a breaking change.

linuxha commented 4 years ago

readeral, I'll try to get this into a new feature branch. So sorry that I've ignored this (yes over a year). Real life intruded.

readeral commented 4 years ago

Yeah all my node-red packages are in a similar state :-/

On Wed, 2 Oct 2019 at 8:04 am, Neil Cherry notifications@github.com wrote:

readeral, I'll try to get this into a new feature branch. So sorry that I've ignored this (yes over a year). Real life intruded.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/linuxha/node-red-contrib-mytimeout/issues/13?email_source=notifications&email_token=ADZK3CFK6X72SXSKGYGFNMDQMPCQHA5CNFSM4FSSZ2R2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEAC5GXI#issuecomment-537252701, or mute the thread https://github.com/notifications/unsubscribe-auth/ADZK3CAGTNHBTR764GIQZM3QMPCQHANCNFSM4FSSZ2RQ .

--

Alan Reader| e. areader0@gmail.com areader0@gmail.com | m. 0400 215 751 | fb. - readeral http://www.facebook.com/readeral | i. - instagram.com/readeral http://instagram.com/readeral

linuxha commented 4 years ago

I'm still working on this, I've decided it will be pause/continue as that seems to make the most sense. I'm still working on tests for this. Also I'm working on a few defects I've found. I'm rewriting the tests (and actually writing the requirements.). While rewriting the tests I found a few bugs.