Opened 4 years ago

Closed 4 years ago

#12279 closed New Feature (fixed)

Allow passing custom evaluator to node.getAscendant

Reported by: AlexW Owned by: Artur Delura
Priority: Normal Milestone: CKEditor 4.4.5
Component: General Version:
Keywords: Cc:

Description

getAscendant is a bit limited, would be handy if it could filter by classes too.

// <div class="outer"><div class="inner"><p><b>Some text</b></p></div></div>
// If node == <b>
ascendant = node.getAscendant('div.outer'); // ascendant == <div class="outer">

Change History (14)

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

Status: newconfirmed

I was also surprised that this function can do so little - I needed to look for an attribute and had to write it manually. So I'm also for extending it, though it must be a callback, not a CSS selector, because this will be faster and simpler to code (shorter too).

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

Do we want indroduce function as a second/third argument (second one is optional) or allow place function as a first argument instead of string (node name)? I preffer second option because it's more flexible - we are not forced to specify node name. Both options of course with backward compability.

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

2nd option

comment:5 Changed 4 years ago by Artur Delura

Status: assignedreview

Solution and tests in branch:t/12279. See documentation as well. We could also implement http://en.wikipedia.org/wiki/Visitor_pattern in the future.

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

We could also implement ​http://en.wikipedia.org/wiki/Visitor_pattern in the future.

I don't think it's possible or necessary:

The visitor pattern requires a programming language that supports single dispatch and method overloading

comment:7 Changed 4 years ago by Artur Delura

I already implemented this pattern in JavaScript, maybe because I didn't know that such stuff is required ;). Function as a first class object is enought i think. It could bring us a lot of flexibility and less code.

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

Status: reviewreview_failed

Well, it's a variation about visitor pattern (because in your callback you have to handle type discovery - in visitor pattern it should happen automatically). But that's just naming - not important here, since callback is a standard (and much simpler) solution in JS world.

I found few issues after first glance:

  1. Callback should be executed with CKEDITOR.dom.element instance. But avoid creating instances when filtering by name (performance).
  2. Run linter.
  3. You can compare CKEDITOR.dom.nodes using assert.areSame without accessing $ property.
  4. You should split test cases into separate function and name them properly. It's hard to guess what you're testing. Don't use assertion message for that.
  5. 'func_check' is not the right id. Neither name itself, nor its format. Use something that will tell that it's for getAscendant test - the same as other tests do.
  6. We use el rather than elem.

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

Status: review_failedreview
  1. Run linter.

Found unused variable.

  1. 'func_check' is not the right id. Neither name itself, nor its format. Use something that will tell that it's for getAscendant test - the same as other tests do.

As always I write tests in same style as closest environment is written.

Changes in the same branch.

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

Status: reviewreview_failed

As always I write tests in same style as closest environment is written.

Well, from the same file:

<div id="insertAfter"><!--abc-->def</div>
<div id="insertBefore"><!--abc-->def</div>
<div><br><br><!--a--><br><div>text<!--b-->text<p>text<br><i id="getAddress1">text</i></p></div></div>
<div><span><span><span><span><i id="getAddress2"></i></span></span></span></span></div>
<iframe id="getDocument"></iframe>
<div id="getNSN">
<i id="getNSN1">1</i>
<i id="getNSN2">2</i>
<i id="getNSN3">3</i>

So func_check definitely does not fit here. All ids are camel cased and are references to tested methods names.


var el = ( conditionChecker === reference ? new CKEDITOR.dom.node( $ ) : $ );

This may be optimized. You can avoid two things:

  1. Unnecessary assignment if callback was passed - don't create unnecessary el variable in this case.
  2. Checking whether callback was passed in every loop step.

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


var el = ( conditionChecker === reference ? new CKEDITOR.dom.node( $ ) : $ );

This may be optimized. You can avoid two things:

  1. Unnecessary assignment if callback was passed - don't create unnecessary el variable in this case.

I make such variables to avoid very long lines, which make code hard to read. When I will join this two lines into one it will have over 100 characters length. Common standard is less than 80. Maybe you don't see problem but I do, because in my IDE I have font size set to 18.

  1. Checking whether callback was passed in every loop step.

So you want to move that check out of while loop? Just like:

var customChecker = conditionChecker === reference;
while() {

If yes, it does not make sense. We shouldn't optimize everything, but just parts which are slow. I made a quick performance tests and found out that such checking need to be called 23585 times to take 1ms. It's nothing.

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

First of all - do not create microbenchmarks. Never do that. You just tested empty loop or no loop at all.

Second - the reason for this changes is sanity - it simply does not make sense to test the value every time or make variable assignments when they are totally unnecessary there.

Also, think about readability - if you make a variable check every time, does this mean that this variable may change during the iteration? Code which does not do unnecessary things will be more clear for the reader.

Finally, note that I didn't mention performance in comment:9 ;).

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

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

Status: review_failedreview

Second - the reason for this changes is sanity - it simply does not make sense to test the value every time or make variable assignments when they are totally unnecessary there.

Also, think about readability - if you make a variable check every time, does this mean that this variable may change during the iteration? Code which does not do unnecessary things will be more clear for the reader.

Finally, note that I didn't mention performance in comment:9 ;).

That make sense, thought you want this improvements because of performance. +1 for readability :) Changes in same branch:t/12279.

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

Milestone: CKEditor 4.4.5
Resolution: fixed
Status: reviewclosed
Summary: Allow class selection in getAscendantAllow passing custom evaluator to node.getAscendant

There were still some issues which I mentioned earlier and which hasn't been resolved, so I pushed additional commit.

Merged to master with git:8749148.

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