Opened 4 years ago

Closed 4 years ago

#12150 closed New Feature (fixed)

Expose getNestedEditable and is* widget helper functions

Reported by: Piotrek Koszuliński Owned by: Artur Delura
Priority: Normal Milestone: CKEditor 4.5.0 Beta
Component: UI : Widgets Version: 4.3 Beta
Keywords: Cc:

Description

They are useful also for widgets definitions, so they should not be private.

Change History (31)

comment:1 Changed 4 years ago by Jakub Ś

Status: newconfirmed
Version: 4.3 Beta

comment:2 Changed 4 years ago by Artur Delura

Owner: set to Artur Delura
Status: confirmedassigned

comment:3 Changed 4 years ago by Artur Delura

I think we should expose constructors in such way. I see that we don't have standard for that.

  • We are exposing constructor by passing it as field which start from lowercase letter.
  • I like the idea how it's done in undo plugin - first we create object literal and then add to it constructors as a property which name is exactly the same as contructor.

Changes in branch:t/12150, but I think we should also make changes listed above.

Last edited 4 years ago by Artur Delura (previous) (diff)

comment:4 Changed 4 years ago by Artur Delura

Status: assignedreview

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

We have a standard that says - constructors in the public API are lowercased. You can find this everywhere except undo plugin. So in this case the undo plugin is incorrect and moreover, what the undo plugin exposes is private, so we could change that, but what the widget plugin exposes is public so we can't touch this.

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

Status: reviewreview_failed

Since we are adding methods to public API we need at least basic tests for them.

Moreover:

  • @returns {CKEDITOR.dom.element|null} Element - this notation is incorrect - null is not a type. Null is "not an object". Usually we explain in the textual part of returned value's description that null may be returned. For example: "Widget wrapper or null if wrapper was not found.".
  • The newly defined methods should be moved to the right place - this can be e.g. right after Widget class prototype. Now they are surrounded by private stuff.

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

Replying to Reinmar:

  • @returns {CKEDITOR.dom.element|null} Element - this notation is incorrect - null is not a type. Null is "not an object". Usually we explain in the textual part of returned value's description that null may be returned. For example: "Widget wrapper or null if wrapper was not found.".

Why null is not a type? Why null is not an object? And finally: Why overcomplicate things and not keep them semantic? I know that there is no constructor for null, but AFAIK JSDuck support values in @param, @return etc. See here.

comment:8 Changed 4 years ago by Piotr Jasiun

Agree. CKEDITOR.dom.element|null} is far more readable then description. On the other hand if we use one convention we should stick to it.

Last edited 4 years ago by Piotr Jasiun (previous) (diff)

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

Hmm... You're right. I must have remembered this incorrectly.

So - I'm ok with null in the type of returned value, but remember that we use /, not |. And still it's sometimes good to explain when null may be returned (not in this case though).

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

Replying to Reinmar:

Hmm... You're right. I must have remembered this incorrectly.

So - I'm ok with null in the type of returned value, but remember that we use /, not |. And still it's sometimes good to explain when null may be returned (not in this case though).

Added extra commit with fixes. Do we need test for is* methods? They are simple as hell, I dunno. Changes in branch:t/12150.

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

There should be at least one test per method.

comment:12 Changed 4 years ago by Artur Delura

Status: review_failedreview

I added extra tests. Plase don't check jscs because they will be fixed directly in major. Changes in branch:t/12150.

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

Plase don't check jscs because they will be fixed directly in major.

I don't understand. Major is clean, so the branch should be clean too.

comment:14 Changed 4 years ago by Artur Delura

Status: reviewassigned

Major is dirty, but I'll handle all so I'm getting off from review for a while.

comment:15 Changed 4 years ago by Artur Delura

Status: assignedreview

comment:16 Changed 4 years ago by Piotr Jasiun

I measured how big is the file size increase of minified widgets plugin.js and minified ckeditor.js build with the widget plugin, before and after changes:

branch major diff % increase
plugins.js 28649 28041 608 2
plugins.js.gz 9314 9238 76 0.8
ckeditor.js 559108 558481 627 0.11
ckeditor.js.gz 166127 166019 108 0.065

Change is not very big, but making methods public makes editor package definitely bigger.

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

Very interesting analysis. One more thing that I'm curious is what would be the result if in the widget plugin we used these functions via their names (as isXxx()) instead by using them as Widget.isXxx() and we exported them to the Widget constructor in one place.

comment:18 Changed 4 years ago by Piotr Jasiun

Anyway R- because of this test. Generally it is bad practice to create big tests. If such tests fail, you do not know what goes wrong and need to learn how the whole test works. Tests should be as small and as simple as possible. One test should check only one thing. Especially in this case when you have multiple isTrue/isFalse assertions without descriptions, so if one of then will fail you will have no idea what is wrong.

comment:19 Changed 4 years ago by Piotr Jasiun

Status: reviewreview_failed

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

BTW. I noticed that API documentation needs to be fixed.

  1. Order of tags is incorrect.
  2. The @member tag does not have to be specified.
  3. Some descriptions are missing.
  4. In few places I found {boolean}. The type is {Boolean}.

comment:21 Changed 4 years ago by Piotr Jasiun

Also this commit should be squashed with the previous one with introduce these changes.

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

Replying to pjasiun:

Anyway R- because of this test. Generally it is bad practice to create big tests. If such tests fail, you do not know what goes wrong and need to learn how the whole test works.

Generally speaking yes, but this is test for static functions. Each part is independent, I think this test need more comments (each assert might have a message what is expected, and DOM elements might be described) rather than split into parts.

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

Generally speaking yes, but this is test for static functions. Each part is independent, I think this test need more comments (each assert might have a message what is expected, and DOM elements might be described) rather than split into parts.

This test should be split into far more than 2 test. Basically - one test per one method. You should avoid testing more than one thing at a time. Sometimes it's hard, but this time it's simple.

Last edited 4 years ago by Piotrek Koszuliński (previous) (diff)

comment:24 Changed 4 years ago by Artur Delura

Status: review_failedassigned

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

Owner: changed from Artur Delura to Piotrek Koszuliński
Status: assignedreview

I pushed two fixes to branch:t/12150:

  1. First adds docs.
  2. Second hides two methods which have no value for other developers.

comment:26 Changed 4 years ago by Piotr Jasiun

Status: reviewreview_failed

Tests are still not good enough.

Firstly I have no idea what this test do. Why the first and third assertion should be false, and the second true? I have also no simple why to check if the method is covered properly. If such test fail I will not know which assertion is red and why. The same about the rest of the refactored tests.

The simplest solution is to add description message to assertions, but it is better to create separate tests with that description in the tests name. It is not more work and this way we would know which assertions fails (not only the first) and why.

Secondly parserEl1..5 are meaningless names for variables. Test would be far more readable if you would use:

assert.isFalse( Widget.isParserWidgetElement( parserElDataWidgetFalse ) );

Instead of:

assert.isFalse( Widget.isParserWidgetElement( parserEl3 ) );

Finally, we do not check if static functions exists, especially when we check if it works properly in the same test suit, so this test is not needed.

comment:27 Changed 4 years ago by Artur Delura

Owner: changed from Piotrek Koszuliński to Artur Delura
Status: review_failedreview

Changes in branch:t/12150.

comment:28 Changed 4 years ago by Piotr Jasiun

I rebased branch on newest major and pushed one more tests improvement. But I have one more doubt.

Would not it better to have isWidgetElement method which accept both CKEDITOR.dom.node and CKEDITOR.htmlParser.node instead of two methods? The same for is*WidgetWrapper. Our API would be simpler and build smaller. Also we could replace isDom* with just is*, because this is not clear what this Dom stands for? I understand that this is the type of the parameter and it makes sense for internal usage, but as an API it looks bad and the type of the parameter is described anyway.

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

Those functions cannot be merged without complicating them internally (it must be checked whether an argument is an instance of X or Y or none), writing more tests etc. Complication will affect performance and tests will need time. Both are things I would like to avoid, especially at this stage of working on this ticket.

Last edited 4 years ago by Piotrek Koszuliński (previous) (diff)

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

Also, we need to remember that these are helper methods, not a first-level API which should be optimised in terms of usage and clarity.

comment:31 Changed 4 years ago by Piotr Jasiun

Resolution: fixed
Status: reviewclosed

Ok. So I closed this ticket as it is. git:96d5130

Note: See TracTickets for help on using tickets.
© 2003 – 2017 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy