Opened 7 years ago

Closed 7 years ago

#5915 closed New Feature (fixed)

ImageDlgHideLink not ported from V2

Reported by: Wiktor Walc Owned by: Tobiasz Cudnik
Priority: Normal Milestone: CKEditor 3.5
Component: UI : Dialogs Version: 3.0
Keywords: Cc:

Description

Either I'm missing something or we forgot to port those settings:

  • ImageDlgHideLink
  • ImageDlgHideAdvanced
  • FlashDlgHideAdvanced

We have ported only:

  • linkShowAdvancedTab
  • linkShowTargetTab

Attachments (12)

#5915.patch (2.6 KB) - added by Martin 7 years ago.
Patch
5915_2.patch (1.4 KB) - added by Martin 7 years ago.
5915_3.patch (1.5 KB) - added by Martin 7 years ago.
5915_4.patch (1.4 KB) - added by Martin 7 years ago.
5915_5.patch (1.4 KB) - added by Martin 7 years ago.
5915_6.patch (1.5 KB) - added by Martin 7 years ago.
5915_7.patch (2.0 KB) - added by Martin 7 years ago.
5915_8.patch (2.1 KB) - added by Martin 7 years ago.
5915_9.patch (2.2 KB) - added by Sa'ar Zac Elias 7 years ago.
5915_10.patch (2.1 KB) - added by Tobiasz Cudnik 7 years ago.
5915_11.patch (2.5 KB) - added by Tobiasz Cudnik 7 years ago.
5915_12.patch (2.1 KB) - added by Tobiasz Cudnik 7 years ago.

Download all attachments as: .zip

Change History (52)

comment:1 Changed 7 years ago by Alfonso Martínez de Lizarrondo

I proposed to remove the link options in #5365 as they aren't included in the docs, but Fred said to leave it as is.

This doc explains how to get the same result using the API so it can be used for any other dialog or tab.

comment:2 Changed 7 years ago by Frederico Caldeira Knabben

Keywords: Discussion added; Confirmed removed

It's a very bad solution to have such static settings for these things. Right now, as pointed by Alfonso, the API is the right way for it, as it's generic, and can be applied to all kinds of dialogs.

What we could think about instead, is having some kind of "shortcut" for some common operations, just to make people's life easier.

For example, we could add checks on the dialog system, which looks for a configuration composed by "<dialog name> + Show + <tab name> + Tab", and automatically show/hide it based on that.

Something similar could be done to show/hide dialog elements, as well as setting their default values.

comment:3 Changed 7 years ago by Alfonso Martínez de Lizarrondo

Yes, while writting the reply this morning I saw that it can be done "easily" with one configuration entry to specify the tabs to remove from each dialog (or elements in a dialog).

I don't see the need to allow to specify if it must be shown or hidden, by default everything is there, so the config should be to specify which elements must be removed removeDialogContents = "Image:Advanced;Link:Advanced"

If we implement #5105 the option to remove elements can be just as easy as specify its id in the same way: removeDialogContents = "Image:Advanced;Link:Advanced;Image:Border"

comment:4 Changed 7 years ago by Frederico Caldeira Knabben

Keywords: Confirmed added; Discussion removed

Yes, that's the kind of generic approach we could have for it, and it's a good solution. So, let's go ahead with it.

comment:5 Changed 7 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 3.4CKEditor 3.5

comment:6 Changed 7 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 3.4.1CKEditor 3.5

comment:7 Changed 7 years ago by Martin

Owner: set to Martin
Status: confirmedassigned

Changed 7 years ago by Martin

Attachment: #5915.patch added

Patch

comment:8 Changed 7 years ago by Martin

Status: assignedreview

comment:9 Changed 7 years ago by Tobiasz Cudnik

Status: reviewreview_failed

Patch is OK, but coding style is wrong - please use tabs for indentation. Theres also unneeded change on L310 of _source/core/config.js file.

comment:10 Changed 7 years ago by Frederico Caldeira Knabben

Additionally, the removeDialogContents doesn't need to be initialized. It can be a "documentation only" setting, which is btw restricted to the dialog plugin, so it should not go into the core config file.

And yes, some reading about coding style and patterns would be good ;)

Changed 7 years ago by Martin

Attachment: 5915_2.patch added

comment:11 Changed 7 years ago by Martin

Status: review_failedreview

comment:12 Changed 7 years ago by Sa'ar Zac Elias

Status: reviewreview_failed
  • There are still coding style mistakes. Please read the coding style and patterns pages.
  • As Fred commented before, the setting doesn't need to be initialized. Instead, please add a descriptive comment about it. Example for such a comment can be found here.

Changed 7 years ago by Martin

Attachment: 5915_3.patch added

comment:13 Changed 7 years ago by Martin

Status: review_failedreview

comment:14 Changed 7 years ago by Sa'ar Zac Elias

Status: reviewreview_failed

Small things:

  • Why i <= removeContentDefinition.length - 1 and not simply i < removeContentDefinition.length?
  • For the same line, please take a look at here.
  • Please add the '@since 3.5' tag to the comment.
  • Please update your local copy before making any new patches, and make sure you're working on the correct branch.

comment:15 Changed 7 years ago by Garry Yao

In the API doc config doc sample comma is instead used as separator, also, as this configuration would probably break a dialog (because of a major tab missing) we need a disclaimer in the documentation.

Changed 7 years ago by Martin

Attachment: 5915_4.patch added

comment:16 Changed 7 years ago by Martin

Status: review_failedreview

Changed 7 years ago by Martin

Attachment: 5915_5.patch added

comment:17 Changed 7 years ago by Sa'ar Zac Elias

Status: reviewreview_failed

Let's cache the split string, so we won't have to split it every time a dialog is opened. Also, please add the disclaimer described by Garry.

Changed 7 years ago by Martin

Attachment: 5915_6.patch added

comment:18 Changed 7 years ago by Martin

Status: review_failedreview

comment:19 Changed 7 years ago by Tobiasz Cudnik

Status: reviewreview_failed

As i see 5915_6.patch still doesn't cache the result of the split method as suggested by Saare. Just the variable for a config property has been added.

Changed 7 years ago by Martin

Attachment: 5915_7.patch added

comment:20 Changed 7 years ago by Martin

Status: review_failedreview

comment:21 Changed 7 years ago by Sa'ar Zac Elias

Status: reviewreview_failed
  • Instead of removeDialog, let's call it removeDialogContents.
  • Why not to have an object to store the tabs to remove, instead of two-dimensional array? { dialogName: [ 'dialogTab', 'otherTab' ] } } for example.

Changed 7 years ago by Martin

Attachment: 5915_8.patch added

comment:22 Changed 7 years ago by Martin

Status: review_failedreview

comment:23 Changed 7 years ago by Sa'ar Zac Elias

Status: reviewreview_failed

You misunderstood my idea, and the patch seems to be made for trunk (should be made on the 3.5 branch). I will post a new patch in a few minutes.

comment:24 Changed 7 years ago by Tobiasz Cudnik

All names should be non case sensitive, as it creates problems when defining a config.

Additionally, used data structure is completely different than proposed one. It's about L198:

tabsToRemove.push( { dialogName: dialogContent[ 0 ], tabName: dialogContent[ 1 ] }); 

Which result is:

[
  {
    dialogName: 'Link',
    tabName: 'Advanced'
  },
  {
    dialogName: 'Link',
    tabName: 'Link'
  },
  { ... },
  ...
]

comment:25 Changed 7 years ago by Tobiasz Cudnik

Type: BugNew Feature

Changed 7 years ago by Sa'ar Zac Elias

Attachment: 5915_9.patch added

comment:26 in reply to:  24 Changed 7 years ago by Sa'ar Zac Elias

Replying to tobiasz.cudnik:

All names should be non case sensitive, as it creates problems when defining a config.

Let's bring #5138 into priority, and make this configuration case insensitive later.

comment:27 Changed 7 years ago by Martin

Status: review_failedreview

comment:28 Changed 7 years ago by Garry Yao

Status: reviewreview_failed

The latest patch is over complicated, even stranger seeing that for...in is used on an array, KISS pls.

comment:29 Changed 7 years ago by Tobiasz Cudnik

Owner: changed from Martin to Tobiasz Cudnik
Status: review_failedassigned

Changed 7 years ago by Tobiasz Cudnik

Attachment: 5915_10.patch added

comment:30 Changed 7 years ago by Tobiasz Cudnik

Status: assignedreview

comment:31 Changed 7 years ago by Garry Yao

Status: reviewreview_failed

As we already said:

Note that the tab names and dialog names are case sensitive!

Then we have to leave the field name case untouched, e.g. broken example of the config 'image:Link'.

Changed 7 years ago by Tobiasz Cudnik

Attachment: 5915_11.patch added

comment:32 Changed 7 years ago by Tobiasz Cudnik

Status: review_failedreview

11th patch brings case insensivity with some simple changes, as present dialogs and their's contents names are strongly different.

comment:33 Changed 7 years ago by Alfonso Martínez de Lizarrondo

I agree with Saare that the case sensitivity should be handled in #5138 instead of making this configuration a special situation.

comment:34 Changed 7 years ago by Garry Yao

Status: reviewreview_failed

Please, leave the case sensitivity now and having other ticket deal with this issue.

Changed 7 years ago by Tobiasz Cudnik

Attachment: 5915_12.patch added

comment:35 Changed 7 years ago by Tobiasz Cudnik

Status: review_failedreview

comment:36 Changed 7 years ago by Garry Yao

Status: reviewreview_passed

comment:37 Changed 7 years ago by Tobiasz Cudnik

Resolution: fixed
Status: review_passedclosed

Fixed with [6145].

comment:38 Changed 7 years ago by Frederico Caldeira Knabben

Resolution: fixed
Status: closedreopened

I see that the setting has been named "removeDialogContents", which made me think that I can remove anything from the dialog contents. But it looks like we can remove tabs only.

Please changed it's name to "removeDialogTabs", to make it clear.

comment:39 Changed 7 years ago by Frederico Caldeira Knabben

Component: GeneralUI : Dialogs

comment:40 Changed 7 years ago by Tobiasz Cudnik

Resolution: fixed
Status: reopenedclosed

Post fixed with [6146].

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