Refactored duplicated throttle and eventsBuffer code #2060
Conversation
|
Changed target branch to |
There was a problem hiding this comment.
The code works however I have some real doubts regarding code readability. For instance look at input method for someone reading that at first it's hard to tell exactly what is the reason for this order.
Instead I think it would be much more readable if we create a type for return values of tools.throttle and tools.eventsBuffer, especially given that we'll have a better docs for that and the types will be extensible themselves.
I created a test implementation for that in t/2045b branch. See how only the ThrottleBuffer is concerned about keeping arguments. EventsBuffer does not need to know anything about that.
However doing it that way will require us to put it in the major release, as it is an API addition. Also the types need to be exposed, and to go along with our standards the proposed type names should be lowercased which will result with... a name collision in case of EventsBuffer. I'm thinking about extracting it into a separate namespace within tools, so it could be like:
CKEDITOR.tools.buffers.throttle- ThrottleBuffer typeCKEDITOR.tools.buffers.event- EventsBuffer type
|
|
||
| function triggerOutput() { | ||
| lastOutput = ( new Date() ).getTime(); | ||
| scheduled = false; |
There was a problem hiding this comment.
This var should be consistent and use 0 here.
|
I agree that it could be simplified and better designed. Your changes look good and I didn't have a better option here than just use your branch. I added additional docs fixes and justified tests for new buffers. |
…s it is often overwrite and it would break the code.
There was a problem hiding this comment.
I have checked the code and initially I have overlooked one thing - often times context of buffer.input would be changed (and it's a very common case).
Again I have pushed the proposed code to t/2045b branch. The fix is creating a new input function (as a prop) that is bound to buffer context passed though the scope.
Works fine now, but please have a look at it and merge it. I'm concerned only about the docs. You need to do some serious clicking before you get to meaningful code listing 🤔
jacekbogdanski
force-pushed
the
t/2045
branch
from
e3c13a1
to
aecfd6f
Compare
|
Looks ok, I refactored docs a little - I think it's easier to track docs when they are as close as possible to a declared function. However, with all these changes refactored buffers seems slightly complicated, mostly thus creating input function inside constructor which isn't common practice. |
…ing can be found for easier discoverability.
|
I agree that it got more complicated. I don't like it, but we have spent already enough time trying to simplify this code. |
What is the purpose of this pull request?
Task
What changes did you make?
Refactored duplicated throttle and eventsBuffer code.
Closes #2045