Community
Participate
Working Groups
Since RAP 3.1 there is a neat possibility of configuring some aspects of the CKEditor (org.eclipse.nebula.widgets.richtext.RichTextEditor / org.eclipse.nebula.widgets.richtext.toolbar.ToolbarConfiguration). But the is still a gap for non-english enviroments / users: you can not change the UI language without recompiling RAP.
Well the issue is that the CKEditor needs the config.language setting at creation time. Language changes in the CKEditor instance at runtime are not supported. And by default it will use the locale of the browser, not the VM. I can provide a patch that by default uses Locale.getDefault() as the language to use by CKEditor and a method to update the editor similar to how it is done by CKEditor samples. But I don't understand why you need to recompile RAP, therefore I'm not sure if this solves the issue with RAP.
I tested the bundeld CKEditor (org.eclipse.nebula.widgets.richtext.RichTextEditor) in RAP 3.1 in a german enviroment (OS, Java, Browser). The UI language was still "en". The config.js contained a line " config.language = 'en'". The analysis of the source-code brought me to a solution, that I commited in https://github.com/arxes-tolina/rap/commit/03e733e8d5feecc07199fb4f27da513b6fdff31c . I can imagine, that it may be a problem of the usage of CKEditor in RAP and not the problem with CKEditor itself. If my solution is correct, I would like to provide a patch to merge this feature into the master-branch of RAP.
Hi all, all improvements in this direction are welcomed. Please create a Gerrit change about it. By looking into the commit above the only missing part is the language file itself - en.js, de.js... or it's not needed at all?
The language file/resource is still needed, but the ToolbarConfiguration decides which one should be used (Default: en.js). For which branch should I prepare a gerrit-change? Master?
New Gerrit change created: https://git.eclipse.org/r/77768
Ah, I see that RAP seems to have forked my original SWT implementation and is doing something differently. So this is nothing I need to take care of. Just as a small hint, I personally don't think that language is only something relevant for the toolbar and should be more general. But as I don't know about the modifications done by the RAP team, I don't have any feelings about it.
Dirk, can we have the same API in Nabula RichTextEditor - ToolbarConfiguration#getToolbarLanguage (currently missing) to be able to easy single-source it.
Is there a similar API in SWT implementation for setting the CKEditor language? If not - can we add the same API in both SWT and RAP implementation?
I commited&pushed this gerrit-change, but the CI failed with "Failed to load p2 repository with ID 'rap-extra-repo' from location http://build.eclipse.org/rt/rap/base-platforms/3.1/extra-dependencies/: HTTP Server 'Service Unavailable': http://build.eclipse.org/rt/rap/base-platforms/3.1/extra-dependencies/content.xml: HttpComponents connection error response code 503." https://hudson.eclipse.org/rap/job/rap-head-runtime-gerrit/1987/console I do not think, I can do anythig about it. @Dirk: you are right - the ToolbarConfiguration.java already allowed to configure some other properties too. But language was missig.
We have this error on the build server for the last 2-3 days and we are trying to find the reason/solution.
(In reply to Ivan Furnadjiev from comment #8) > Is there a similar API in SWT implementation for setting the CKEditor > language? If not - can we add the same API in both SWT and RAP > implementation? No there is currently no such API. As I already mentioned I could add that. I already tested that with the SWT examples and it seems to work. But as I also already explained, language is nothing specific to the toolbar. Therefore I think it should not be part of that API. Instead I added a getLanguage() in the RichTextEditor. I push this later or on the weekend, as I currently don't have access for pushing to Gerrit from where I am.
> But as I also already explained, language is nothing specific to the > toolbar. Therefore I think it should not be part of that API. Instead I > added a getLanguage() in the RichTextEditor. Could you update your change the the API that Dirk proposed above?
@Ivan: - I could not put mark the method as 'protected' because it is used in RichTextEditor to register a file as resource - I am not sure if I got it right. You mean to move "language" property to RichTextEditor? IMO ToolbarConfiguration is a nice class to contain it. The 'language'-Property is here only for the toolbar: not for for input or others. @Dirk: - can You post here a link to your idea, after you commited it?
Let wait till Dirk provides an API in the SWT Nebula RichTextEditor. By looking into the strings in en.js there are some not related to the toolbar. That's why I agree with Dirk about the API in RichTextEditor not in ToolbarConfiguration.
Got it :) I can check this next week.
In first place it looks like language is only related to the toolbar, because that is the UI element users interact with. But there might be also dialogs or anything else that needs to be localized. I don't know the details, but looking into the ckeditor documentation language is a general configuration. The configurations I encapsulated in the toolbar configuration are directly connected to the toolbar. http://docs.ckeditor.com/#!/guide/dev_uilanguage
+1
I created Bug 498392 and pushed my idea to support locale changes at runtime to Gerrit. Let me know what you think and if this API can be also used in the RAP implementation.
(In reply to Dirk Fauth from comment #18) > I created Bug 498392 and pushed my idea to support locale changes at runtime > to Gerrit. > > Let me know what you think and if this API can be also used in the RAP > implementation. Re-creating CKEditor at runtime is problematic in RAP and leads to crashes in some browsers. That's way the language in RAP must be set once in widget creation. From API point of view (have protected RichTextEditor#getLanguage method) is fine in RAP. To change the language, the programmer must subclass the RichTextEditor and override the getLanguage method. Another option would be a constructor with additional parameter Locale.
(In reply to Ivan Furnadjiev from comment #19) > Re-creating CKEditor at runtime is problematic in RAP and leads to crashes > in some browsers. That's way the language in RAP must be set once in widget > creation. Well there is no other way to dynamically change the locale at runtime. At least I didn't find one and the CKEditor examples do it the same way. By the way, this is also the current implementation for updating the toolbar at runtime. I haven't found another working solution for that so far. > From API point of view (have protected RichTextEditor#getLanguage method) is > fine in RAP. To change the language, the programmer must subclass the > RichTextEditor and override the getLanguage method. Another option would be > a constructor with additional parameter Locale. I disagree on the constructor parameter. But we could think about a setLanguage() method so a sub-classing is not necessary.
(In reply to Dirk Fauth from comment #20) > I disagree on the constructor parameter. But we could think about a > setLanguage() method so a sub-classing is not necessary. As I already said, we can't recreate the CKEditor at runtime. That's why, setLanguage method is NOT an option for RAP. We have to ensure that the language is set ONLY once when widget is created. Have a protected method getLanguage is fine with me (even it's not so nice to subclass RichTextEditor to set the language). Wojtek, what do you think?
(In reply to Ivan Furnadjiev from comment #21) > (In reply to Dirk Fauth from comment #20) > > I disagree on the constructor parameter. But we could think about a > > setLanguage() method so a sub-classing is not necessary. > > As I already said, we can't recreate the CKEditor at runtime. That's why, > setLanguage method is NOT an option for RAP. We have to ensure that the > language is set ONLY once when widget is created. Have a protected method > getLanguage is fine with me (even it's not so nice to subclass > RichTextEditor to set the language). Wojtek, what do you think? Well I'm not aware of RAP and how things are working there. But ok, we could also think about some additional configuration object that could be extended in the future for additional configurations. And similar to the ToolbarConfiguration it could be a constructor parameter.
(In reply to Dirk Fauth from comment #23) > Well I'm not aware of RAP and how things are working there. In RAP, the CKEditor does not live in it's own isolated browser, but in the same browser with the main RAP application. If you have more than one instance of a CKEditor (application with multiple RichTextEditors), recreating a single CKEditor leads to crashes in other. Unfortunately, I didn't find a stable way to recreate a specific CKEditor instance. Configuration object in constructor is fine with me, even constructor with 4 parameters are too much for my taste. That's why I suggested Locale parameter in constructor. But if you think that in the future, additional configuration options could be set too, constructor with RichTextConfig object is fine.
OK, I come up with an updated patch later. Regarding the udpate issue, I'm not sure what you have done to integrate the RichTextEditor in RAP. Obviously you are not using the default template but use something else. Dependent on what you are doing and the way you are doing it in the RAP port, you should be able to solve this with custom IDs per instance.
Hi all! Thanks for your involvment! - I agree with Dirk about subclassing of widgets (because org.eclipse.swt.widgets.Widget.checkSubclass()) so I would prefer injection too. A setter injection comes with the 'mutable'-smell, but maybe it is not needed here and not a common solutcion in SWT/RWT framework. On the other hand: bloated constuctor is a 'smell' too. So: Whatever! ;) - setting a static Locale is usually not desired in a RAP/RWT-context. Using per a Locale-Variable per Session is much better imo. So a kind of decoupling the "Locale.getDefault()" and CK-Editor-UI is needed and important. - If the CKEditor can not be recreated in a RAP-session, then it is so. In our Use-Cases it is perfectly ok. The user logs in, uses the editor, and does not want to change his toolbars language. If we would need another language, the user would need to re-login. But it is obvious: the generic use-case contains changing the UI language at runtime. I can not tell how much worth this generic use-case is. The idea with 'an ID per instance' would probably make it work, but at the cost of complexer code. I wait then for the updated version of Dirks patch, then I can update the gerrit-change.
OK, I updated the patch and added a RichTextEditorConfiguration object that is dynamically added to the configuration of the ckeditor. I have seen that in RAP there is a different way of configuring a ckeditor instance. But from the API point of view it should be ok for you aswell.
I will attempt to implement a RAP port of Dirks functionality with the priority to use similar API. I will not implement the RichTextEditor#setLanguage() methods, because in RAP the editor can not be recreated and this methods modify the injected configuration, which can be confusing. With the new configuration class RichTextEditorConfiguration some informations could be redundant. That leeds to a need of merging the configuraions.
I updated the code in gerrit, so that the solution is similar to teh SWT-Version. But I think there are still some issues with the API / implementation 1. the RichTextEditor has to many constructors: making them package-protected and offering a public builder would be an improvment imo 2. posibility of redundant information in both *Configuration classes makes the code harder to use; why do we need both? 3. the RichTextEditorConfiguration stores vaules in Map<String,String>. Those values are serialized into JsonString, I am not sure if it can be serialised into all JsonValue subclasses (like JsonString or JsonNumber) 4. the RAP-implementation needs to know, what configuration has to be updated (see RichTextEditor.js#setConfig()), so the values in a generic config with Map<String,String> will not update all of the CKEditor properties Please let me know, what do you think about this four points, so I can update the commit in gerrit if necessary.
(In reply to Wojtek Polcwiartek from comment #29) > I updated the code in gerrit, so that the solution is similar to teh > SWT-Version. > But I think there are still some issues with the API / implementation > 1. the RichTextEditor has to many constructors: making them > package-protected and offering a public builder would be an improvment imo I don't really like the builder pattern and IMO it doesn't make sense for convenient constructors with four parameters. > 2. posibility of redundant information in both *Configuration classes makes > the code harder to use; why do we need both? Because in version 1.0 we only introduced the ToolbarConfiguration and now with 1.1 additional configurations where requested by you. So for backwards compatibility we need ToolbarConfiguration and for new stuff the RichTextEditorConfiguration > 3. the RichTextEditorConfiguration stores vaules in Map<String,String>. > Those values are serialized into JsonString, I am not sure if it can be > serialised into all JsonValue subclasses (like JsonString or JsonNumber) Sounds like RAP so I don't have a clue > 4. the RAP-implementation needs to know, what configuration has to be > updated (see RichTextEditor.js#setConfig()), so the values in a generic > config with Map<String,String> will not update all of the CKEditor properties Definitely RAP so I don't have a clue
Dirk, Wojtek, some comments from my side about the API. RichTextEditorConfiguration -> 1. add configuration to configuration doesn't sound good. I would suggest to rename the addConfiguration to setOption - set an option to configuration. 2 I don't see the need of public setter/getter for language/default language. They are options like other options. Just add getOption(String key). 3. Rename getCustomConfiguration to get getAllOptions. Make sure that it returns a clone (safe-copy) of the internal map. Thus the API in RichTextEditorConfiguration will be simple - set/getOption + getAllOptions with public constants for the supported keys. With RichTextEditorConfiguration you could set everything which goes to CKEDITOR.config property. As RichTextEditorConfiguration can be used also to set tollbar related options (currently set by ToolBarConfiguration) only these new constructor are needed: org.eclipse.nebula.widgets.richtext.RichTextEditor.RichTextEditor(Composite, RichTextEditorConfiguration) org.eclipse.nebula.widgets.richtext.RichTextEditor.RichTextEditor(Composite, RichTextEditorConfiguration, style) The ToolBarConfiguration and constructors that use it can be deprecated. The programmer can use a constructor with old ToolBarConfiguration OR with new RichTextEditorConfiguration. We will not needed to merge two configs - ToolBarConfiguration and RichTextEditorConfiguration - serialize to JSON the one, which is set. What do you think?
(In reply to Ivan Furnadjiev from comment #31) > Dirk, Wojtek, some comments from my side about the API. > RichTextEditorConfiguration -> > 1. add configuration to configuration doesn't sound good. I would suggest to > rename the addConfiguration to setOption - set an option to configuration. Just a method name, I can go with that. > 2 I don't see the need of public setter/getter for language/default > language. They are options like other options. Just add getOption(String > key). Maybe not from the RAP point of view, but I see this as a convenience method that makes is easy for a user of the widget to change the locale. > 3. Rename getCustomConfiguration to get getAllOptions. Make sure that it > returns a clone (safe-copy) of the internal map. OK, I had in mind that someone might want to change the content of the map directly, but I agree that it would be better to secure the access. > Thus the API in RichTextEditorConfiguration will be simple - set/getOption + > getAllOptions with public constants for the supported keys. > With RichTextEditorConfiguration you could set everything which goes to > CKEDITOR.config property. Correct. > As RichTextEditorConfiguration can be used also to set tollbar related > options (currently set by ToolBarConfiguration) only these new constructor > are needed: > org.eclipse.nebula.widgets.richtext.RichTextEditor.RichTextEditor(Composite, > RichTextEditorConfiguration) > org.eclipse.nebula.widgets.richtext.RichTextEditor.RichTextEditor(Composite, > RichTextEditorConfiguration, style) > The ToolBarConfiguration and constructors that use it can be deprecated. > The programmer can use a constructor with old ToolBarConfiguration OR with > new RichTextEditorConfiguration. We will not needed to merge two configs - > ToolBarConfiguration and RichTextEditorConfiguration - serialize to JSON the > one, which is set. > What do you think? I disagree on the last statements. Again you are only looking at it from the RAP point of view. Your point of view is that everyone is aware that we are using a browser based widget. ToolbarConfiguration was created to abstract out that detail and give the possibility to customize the toolbar with default Java (by still granting the possibility to do javascript directly). I don't want to remove that API.
(In reply to comment #32) > ToolbarConfiguration was created to abstract out that > detail and give the possibility to customize the toolbar with default Java (by > still granting the possibility to do javascript directly). I don't want to > remove that API. That's not completely true. The most complex configuration (getToolbarGroupConfiguration) is still a JSON, which user must create by hand... and not as a Java JSON object, but as string. The remove buttons list is just comma-separated string with button names. Creation of this string is not complex. Thus, if we have: config.setOption("toolbarGroups", "["{\"name\":\"basicstyles\",\"groups\":[\"basicstyles\",\"cleanup\"]}]"); config.setOption("removeButtons", "PasteText, Styles"); is not much different from overriding the getToolbarGroupConfiguration/getRemoveButtonConfiguration. Public fields like removeStyles are good, but again they just simplify the creation of already simple comma-separated string. I don't inisist to remove the ToolBarConfiguration. What I don't like is a constructor with overlapping configuration objects: public RichTextEditor(Composite parent, RichTextEditorConfiguration editorConfig, ToolbarConfiguration toolbarConfig, int style). If "removeButtons" is defined in both, which one will win then??!!
(In reply to Ivan Furnadjiev from comment #33) > If "removeButtons" is defined in both, which one will win then??!! The one in ToolbarConfiguration as it is applied afterwards. But ok, if you feel that this would be inconsistent I have to rework the whole configuration design, as it is not only about toolbar and language. As this is not a trivial task overall I will need some time to satisfy a project I didn't had in mind when I created the widget.
I think, we can all agree, that possible inconsistences and redundancies are not the best solutions ever. The question is: which steps are needed and when to do them. The design with ToolbarConfiguration leads to a gap. It is not bad at all: it was enough to deal with most common use-cases. But we all see, that we ("developers"/"actors"/"customers") need more possibilities. Imo, it would be nice to have a easy-to-use, clean design without flaws/issues/suprises. Until the next release (RAP 3.2, SWT/Nebula - I am not sure), maybe there is enough time to do this. This functionality is still nothing that complex. And the configuration code is quite new (at least in RAP), so many changes seem natural. It would be good, if the configuration would store the values as correct datatypes ('(boolean) true' or 'Boolean.TRUE' and not '(String) "true"' ). My actual change in Gerrit can wait for other design changes, that satisfy SWT and RAP. I would appreciate Dirks redesign, so I can update the code in Gerrit. After this I could try to port it to RAP, and give my remarks to this.
It only leads to a gap regarding other configuration options. It is well suited for the toolbar configuration. And it also adds the ability to add custom toolbar buttons, which was the reason for the design as it is. If you complain about the ToolbarConfiguration design, you complain about my work. RAP just adapted it after I created the widget. Whatever RAP did in advance to make the configuration work in RAP is out of my scope. I agree that we should avoid possible inconsistencies and redundancies. This is why I answer and work on that topic. I'm just saying that it is not as easy and non-complex as it looks like. Only looking at the configuration point of view that is applied at startup is not enough. It might be for RAP, but not for SWT, as we need to distinguish between standalone and embedded (when used inside a table) and we need to support changes at runtime. Therefore we also have some processes to work on, e.g. resizing capabilities that change. But yes, looking only at the config stuff, it is sure pretty easy.
@Dirk I do not complain at all. The ToolbarConfiguration works fine, for the use-cases for which it was designed. But the gap is a fact. I have a great respect for work of other people - I did not wanted to underestimate the problem. Your arguments about resising&co convinced me, that there is more under the surface. But on the other hand: I have to stay critical and present my point of view. So as I already wrote: take your time and do it your way.
Dirk, we only want to make the API better, nothing else. Whatever API you create, we will adapt it in RAP. The only restriction for RAP (at least for now), is to be able to apply the configuration on widget creation. But I think that this is natural.
(In reply to Wojtek Polcwiartek from comment #37) > I have to stay critical and present my point of view. Sure, and please keep on going with that. Things can only get better if people are discussing. (In reply to Ivan Furnadjiev from comment #38) > we only want to make the API better If you remember I agreed on most of your suggestions. I take your remarks seriously and glad about feedback. > be able to apply the configuration on widget creation At least for the toolbar this is already possible. And as far as I have seen RAP is already using a different approach with config.js files. Nevertheless, my complaints here were only because of the "should be easy" statements, that all rely on the CKEDITOR.config topics only. And I wanted to point out that there are more things related to SWT usage that need to be considered.
Creating an implementation is much easier than designing an API. You can change the implementation at any point, but API.
(In reply to Ivan Furnadjiev from comment #40) > Creating an implementation is much easier than designing an API. You can > change the implementation at any point, but API. Correct. And you are requesting an API change now by introducing a general RichTextEditorConfiguration and deprecating the ToolbarConfiguration. Basically that is why there is such a discussion that was started for introducing the ability to change the locale. ;-)
(In reply to Dirk Fauth from comment #41) > (In reply to Ivan Furnadjiev from comment #40) > > Creating an implementation is much easier than designing an API. You can > > change the implementation at any point, but API. > > Correct. And you are requesting an API change now by introducing a general > RichTextEditorConfiguration and deprecating the ToolbarConfiguration. > Basically that is why there is such a discussion that was started for > introducing the ability to change the locale. ;-) Yes. And it is great, that you, Dirk, took this request as a opportunity for creating a generic solution. The sentence "This functionality is still nothing that complex" shows only, that I do not know realy, what happens under the hood of SWT and what are the use-cases for the RichTextEditor in SWT. Take the needed time - haste makes waste.
I pushed a backwards compatible patch to Gerrit with a general configuration design. Please let me know if that satisfies all needs you see.
I'm fine with the change. Thanks Dirk!
From the API point of view it looks good to me too. I am not sure how much of this functionality can be ported to RAP. We will see. Probably I will be ready with the RAP-Version on monday. Then I can give some more detailed feedback. Thanks a lot for Your contribution.
Wojtek, please add only language/defaultLanguage options. Don't try to port everything. We can extend it later if someone need it.
Gerrit change https://git.eclipse.org/r/77768 was merged to [master]. Commit: http://git.eclipse.org/c/rap/org.eclipse.rap.git/commit/?id=415dc4f3a393b149c33c88ecae3b0a3faeb48e6f
Fixed in master
