Community
Participate
Working Groups
File encoding selection and validation in "project properties > resource" (ui.ide) is different from the one in "run configurations > common" (debug.ui). I've wrote a patch to change both plug-ins and allow debug.ui having the same behavior and reusing code from ui.ide. The problem is that even tough ui.ide changes are simple, debug.ui team doesn't have commit access to implement them. This is the related bug: http://bugs.eclipse.org/bugs/show_bug.cgi?id=244708. We start ui.ide discussion after comment 10. Could you guys please help us to implement the enhancement? You could just apply the patch, and move encoding validation out from the internal package if suitable (we discuss this in the comments). Thanks in advance.
Created attachment 127090 [details] The two-lines patch for ui.ide
I'd prefer not to make this a public static method (especially since it simply calls Charset.isSupported(enc)) If it is not possible to use one of the concrete classes of org.eclipse.ui.ide.dialogs.AbstractEncodingFieldEditor then wouldn't it be better to subclass it can override doStore() and getStoredValue() to work with the launch config? PW
I've read some code and I think that AbstractEncodingFieldEditor should not extend FieldEditor, since an encoding editor is a general purpose control, but FieldEditor goal is about preference edits. I think better classes would be: - EncodingCombo: an encoding editor combo (including almost all the functionality from current AbstractEncodingFieldEditor, related to UI and encodings). - EncodingFieldEditor: a preference editor which would use EncodingCombo and contain the rest of the functionality from AbstractEncodingFieldEditor related to load/save preferences. - ResourceEncodingCombo: not related to preferences, would extend EncodingCombo to keep the combo in sync with a related resource. This approach would allow us to reuse EncodingCombo in debug.ui without the workaround of extending AbstractEncodingFieldEditor and overriding preferences behavior (just like ResourceEncodingFieldEditor is currently doing). In the meantime yes, I think we can extend AbstractEncodingFieldEditor and workaround it, but I think the above reorganization would improve the code quality.
Creating an API to expose one line "return Charset.isSupported(enc);" ? I would just copy it in your code, but I must be missing something here?
There's also an enclosing exception, and the standard error message. Regardless of simplicity, the idea was reusing code. However what I have currently in mind is that even more code can be reused, because debug.ui has its own raw combo which could be replaced by ui.ide combo with embedded encoding functionality, although such combo is tied to a preference editor. So forget about making isValidEncoding public. My actual request would be reorganizing encoding classes, as I said in comment 3. Split AbstractEncodingFieldEditor into two separate classes: EncodingCombo (UI logic) and EncodingFieldEditor (preferences logic, including an EncodingCombo). Also, rename ResourceEncodingFieldEditor to ResourceEncodingCombo, and make it inherit from EncodingCombo, not EncodingFieldEditor (current AbstractEncodingFieldEditor), since there's no properties logic in resource combo (actually, I don't understand why load/save methods from FieldEditor are abstract, since they're supposed to do it from a preference store, not anywhere else - as far as I could perceive from FielEditor's class documentation). It's just a suggestion to improve the code, so that debug.ui could reuse EncodingCombo out-of-the-box. If for any reason you can't reorganize the classes, then I'd ask only for having access to the standard error message for unsupported encoding (I guess it's in the same package as isValidEncoding). In debug.ui I'd just extend AbstractEncodingFieldEditor as something like EncodingEditor, workarounding the preferences functionality.
(In reply to comment #5) > Also, rename ResourceEncodingFieldEditor to ResourceEncodingCombo, and make it > inherit from EncodingCombo, not EncodingFieldEditor (current > AbstractEncodingFieldEditor), since there's no properties logic in resource The ResourceEncodingFieldEditor is an API, so renaiming it is a non-starter. Changing its inheritance chain can only be done in a way that preserves all public methods it currently inherits.
The other classes are also API. In the battle between fix poor code vs. break API, I'd prefer the migration effort to the new API and end up with fixed code for everybody. To change the API without hardly breaking compatibility, you can create the new classes into a new subpackage "encoding", and tag the old ones as deprecated, and possibly make a lobby for the migration. org.eclipse.ui.ide.dialogs - AbstractEncodingFieldEditor <deprecated, use encoding sub-package instead> - EncodingFieldEditor <deprecated, use encoding sub-package instead> - ResourceEncodingFieldEditor <deprecated, use encoding sub-package instead> org.eclipse.ui.ide.dialogs.encoding - EncodingCombo <since 3.5> - EncodingFieldEditor <since 3.5> - ResourceEncodingCombo <since 3.5>
(In reply to comment #7) > To change the API without hardly breaking compatibility, you can create the new > classes into a new subpackage "encoding", and tag the old ones as deprecated, > and possibly make a lobby for the migration. Right, if we were to introduce a new structure we are required to do it this way. We cannot break API. If I understand ResourceEncodingFieldEditor correctly, it takes advantage of the setup of FieldEditor but doesn't deal with preferences. We won't introduce new API unless there is a clear benefit ... reworking a hierarchy to make it cleaner is not enough, there must be more. Especially since it is currently used both with and without preferences, so it is adequate to the job. PW
> If I understand ResourceEncodingFieldEditor correctly, it takes advantage of > the setup of FieldEditor but doesn't deal with preferences. Actually what I've seen is that it takes advantage of the AbstractEncodingFieldEditor, not FieldEditor, because of the encoding combo (or group of controls containing the combo). Everything about EndodingEditor is preferences workaround. > We won't introduce new API unless there is a clear benefit ... reworking a > hierarchy to make it cleaner is not enough, there must be more. Especially > since it is currently used both with and without preferences, so it is adequate > to the job. Where 'without' in my opinion is a workaround. The main benefit would have a standalone, strictly-UI encoding combo, which could be reused without any workaround. A simpler solution (although not complete) would be at least create such EncodingCombo, extracting most of the functionality from AbstractEncodingFieldEditor, which would be kept with the same interface, but this time _using_ the combo control instead of _containing_ it, as well as the other classes. This way the API would be all the same, but with a new EncodingCombo class extracted from AbstractEncodingFieldEditor. This new class would be used in debug.ui (and anywhere else) to have a simple encoding combo with standard error message for invalid encodings. It's not so clean, but better at least.
Besides EncodingCombo could exist also an EncodingGroup, containing those two radios for the combo and for default option (they're pretty the same in both plugins, except for the labels and that in debug.ui default means what's selected for the project, which can be its default or not).
(In reply to comment #10) > Besides EncodingCombo could exist also an EncodingGroup, containing those two > radios for the combo and for default option (they're pretty the same in both > plugins, except for the labels and that in debug.ui default means what's > selected for the project, which can be its default or not). > Actually I've just checked and it seems that the default in debug.ui is not the project setting itself, but the same default value.
Created attachment 127593 [details] Proposed implementation (new ConsoleEncodingEditor) New class ConsoleEncodingEditor replaces the previous encoding editor area with an implementation of AbstractEncodingFieldEditor, but this last was modified because wasn't completely extensible (most of it was change stuff from private to protected). There's currently only one problem with this as far as I could see: selecting default encoding will later enable the combo itself, not keep the default. You'll need to checkout debug.ui and ui.ide to try the patch.
Comment on attachment 127593 [details] Proposed implementation (new ConsoleEncodingEditor) https://bugs.eclipse.org/bugs/show_bug.cgi?id=244708#c20
This bug hasn't had any activity in quite some time. Maybe the problem got resolved, was a duplicate of something else, or became less pressing for some reason - or maybe it's still relevant but just hasn't been looked at yet. If you have further information on the current state of the bug, please add it. The information can be, for example, that the problem still occurs, that you still want the feature, that more information is needed, or that the bug is (for whatever reason) no longer relevant.
