Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#13337 closed New Feature (fixed)

Convenient way to listen to widget events through the repository

Reported by: Artur Delura Owned by: Artur Delura
Priority: Normal Milestone: CKEditor 4.5.0
Component: General Version: 4.3 Beta
Keywords: Cc:

Description (last modified by Piotrek Koszuliński)

Listening to widget events isn't easy if one is not the widget author. Widgets are created dynamically so first we need to listen to repository#instanceCreated and add listeners to every created instance of the type that we are interested in.

More convenient way would be a good addition to the API:

editor.widgets.onWidget('image2', 'data', func() {});

Change History (11)

comment:1 Changed 9 years ago by Artur Delura

Owner: set to Artur Delura
Status: newassigned

comment:2 Changed 9 years ago by Artur Delura

Changes and tests in branch:t/13337b.

comment:3 Changed 9 years ago by Artur Delura

Description: modified (diff)
Status: assignedreview

comment:4 Changed 9 years ago by Piotrek Koszuliński

Code:

  1. onWidget misses description of arguments. You can simply redirect the reader to even#on, but these arguments should be mentioned.
  2. widget.name === widgetName -> use ==.

Tests:

  1. I don't like the idea of mocking everything – the whole environment. This makes tests unrealistic and means that whatever changes in the code you may need to adjust your mocks' behaviours... if you realise it of course. In the past we mocked editor in few tests and the only outcome of this was that after some time they turned out to test unrealistic and invalid scenarios because the editor changed. But it took us a lot of time to find this out, because beside that the tests looked fine. So – mock modules and do not mock the whole application (the editor in this case). Never mock things like dom.element, because you will not be able to do so.
  2. In my opinion the easiest way to test this feature is to spy on widget.prototype.on(). For start you need just two tests:
    • one that checks that on() is called on correct widget instances (check the names) from repository.instances object (you can even mock all those objects if you want) and that correct arguments are passed.
    • one that checks that on repo#instanceCreated the widget.on() method is called with proper arguments. You can mock the instance passed to instanceCreated widget if you want.

As you may agree now – you mocked the wrong things. You can easily work with a real editor and real repository if you mock widget instances. But you don't even need to do that if you spy on widget.prototype.on. What's more – you wrote tests for event#fire and event#on because you started checking if args like context and data work. This is unnecessary – these methods have their own tests. You just need to check that these methods are called with the right arguments.

  1. There's one more test that I would like to see – a fully realistic one this time – whether you can listen on widget#init event using this method. The reason why I want this test is that without digging in the docs it is not fully clear if repo#instanceCreated is fired after or before widget#init. If after, then it won't be possible.

comment:5 Changed 9 years ago by Piotrek Koszuliński

Status: reviewreview_failed

comment:6 Changed 9 years ago by Artur Delura

Status: review_failedassigned

comment:7 in reply to:  4 Changed 9 years ago by Artur Delura

Replying to Reinmar:

Code:

  1. onWidget misses description of arguments. You can simply redirect the reader to even#on, but these arguments should be mentioned.

Done

  1. widget.name === widgetName -> use ==.

How about adding this as a rule for jscs or jshint?

Tests:

  1. I don't like the idea of mocking everything [...]

I had similar doubts.

  1. In my opinion the easiest way to test this feature [...]

Yep, I didn't think of it that way. It's better to test exacly what you wrote.

comment:8 Changed 9 years ago by Piotrek Koszuliński

How about adding this as a rule for jscs or jshint?

I doubt any of them has a rule to require ==. Especially that sometimes === must be used and it's case sensitive.

comment:9 in reply to:  4 Changed 9 years ago by Artur Delura

Status: assignedreview
  1. There's one more test that I would like to see [...]

As we talk yesterday we don't need this test because we found out that there is no such event like widget#init.

Changes and tests in branch:t/13337b

Version 0, edited 9 years ago by Artur Delura (next)

comment:10 Changed 9 years ago by Piotrek Koszuliński

Resolution: fixed
Status: reviewclosed
Version: 4.3 Beta

Merged to major with git:83e9980.

comment:11 Changed 9 years ago by Piotrek Koszuliński

Description: modified (diff)
Summary: Improve way of listening to events on widgets repo.Convenient way to listen to widget events through the repository
Note: See TracTickets for help on using tickets.
© 2003 – 2022, CKSource sp. z o.o. sp.k. All rights reserved. | Terms of use | Privacy policy