Opened 7 years ago

Closed 7 years ago

#10072 closed Bug (fixed)

Disallowed command should be disabled by changing its state

Reported by: Piotrek Koszuliński Owned by: Piotrek Koszuliński
Priority: Normal Milestone: CKEditor 4.1 RC
Component: General Version:
Keywords: Drupal Cc: wim.leers@…

Description (last modified by Piotrek Koszuliński)

This is follow-up of #9829.

Currently when command isn't allowed its exec() method blocks it. It should rather be blocked by being disabled what will be more semantic.

Change History (7)

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

Description: modified (diff)
Status: newconfirmed

comment:2 Changed 7 years ago by Frederico Caldeira Knabben

Pushed t/10072@cksource.

To make this happen we need to change the behavior of commands, making them disabled by default (instead of "off").

Still we have tests failing now, but there is a good probability that it is just an issue related to the tests, not to the implementations. I'm not putting it on review because of this.

comment:3 Changed 7 years ago by Piotrek Koszuliński

I checked the code and... have mixed feelings :).

This is that kind of patch which I understand, but I don't understand :D. Changes in code are clear for me, but not their implications.

PS. I'd avoid creating 2 functions for each cmd#checkAllowed method. Value may be cached in cmd object - we'd save some memory.

PPS. Filter#check caches results for string based tests.

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

Owner: set to Piotrek Koszuliński
Status: confirmedassigned

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

Status: assignedreview

I pushed fixes to dev and tests.

However, the last patch for tests proves that this solution may be incomplete.

All commands start as disabled.

Then on i.a. 'mode' event commands are refreshed. This event is fired after 'load' but just before 'instanceReady' (that's why test in clipboard/paste.html was failing - it is executed on 'load').

But after 'instanceReady', all newly added commands aren't refreshed. This caused the dialogadv/dialog.html test to fail.

For 99% of cases current solution should be sufficient. But I can imagine that this 1% may cause big WATFs :). We could solve this problem by automatically refreshing all commands added after 'instanceReady'.

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

Status: reviewreview_passed

I'm R+ing this issue now to speed things up, but we may fix issue mentioned in comment:2 after ticket is closed.

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

Resolution: fixed
Status: review_passedclosed

Fixed on major with git:013be3c on dev and c5dfe82 on tests.

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