eclipse.platform icon indicating copy to clipboard operation
eclipse.platform copied to clipboard

Using UTF_8 as default in core.resources.PreferenceInitializer

Open vogella opened this issue 1 year ago • 9 comments

https://github.com/eclipse-platform/eclipse.platform.resources/pull/51/ implemented logic to set UTF-8 as standard. I suggest to also set the default preference value to UTF-8.

vogella avatar Jul 03 '24 06:07 vogella

Test Results

 1 653 files   -  81   1 653 suites   - 81   1h 21m 19s :stopwatch: - 6m 21s  3 819 tests  - 153   3 793 :white_check_mark:  - 157   22 :zzz: ±0   4 :x: + 4  12 054 runs   - 459  11 883 :white_check_mark:  - 469  161 :zzz: ±0  10 :x: +10 

For more details on these failures, see this check.

Results for commit d4d794a9. ± Comparison against base commit 0f64336d.

This pull request removes 153 tests.
AllCompareTests org.eclipse.compare.tests.AsyncExecTests ‑ testCancelOnRequeue
AllCompareTests org.eclipse.compare.tests.AsyncExecTests ‑ testQueueAdd
AllCompareTests org.eclipse.compare.tests.AsyncExecTests ‑ testWorker
AllCompareTests org.eclipse.compare.tests.CompareFileRevisionEditorInputTest ‑ testPrepareCompareInputWithNonLocalResourceTypedElements
AllCompareTests org.eclipse.compare.tests.CompareUIPluginTest ‑ testFindContentViewerDescriptorForTextType_StreamAccessor
AllCompareTests org.eclipse.compare.tests.CompareUIPluginTest ‑ testFindContentViewerDescriptor_TextType_NotStreamAccessor
AllCompareTests org.eclipse.compare.tests.CompareUIPluginTest ‑ testFindContentViewerDescriptor_UnknownType
AllCompareTests org.eclipse.compare.tests.ContentMergeViewerTest ‑ testFFFX
AllCompareTests org.eclipse.compare.tests.ContentMergeViewerTest ‑ testFFTX
AllCompareTests org.eclipse.compare.tests.ContentMergeViewerTest ‑ testFFXF
…

github-actions[bot] avatar Jul 03 '24 07:07 github-actions[bot]

@iloveeclipse

Sorry for the reviewer bounce.

I have a feeling this is not correct. Looking at the logic here, the default depends on a bunch of things such as the system property or the encoding preference:

https://github.com/eclipse-platform/eclipse.platform/blob/0f64336d2bc4c785643f38d429bd36bdbae0337e/resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/Workspace.java#L2339-L2383

But the initializer logic of this PR hard codes the value.

merks avatar Jul 03 '24 07:07 merks

@merks I do not get what is different in your review request? Can you explain?

image

vogella avatar Jul 03 '24 08:07 vogella

@merks I do not get what is different in your review request? Can you explain?

I accidentally removed him and then added him back.

merks avatar Jul 03 '24 08:07 merks

Thanks Ed! Lars, it os not as trivial as one could think, I recommend to read the code Ed referenced and related tickets. I will be able ti review this later, hopefully on Friday, as I'm two days mostly not on my workstation.

iloveeclipse avatar Jul 03 '24 08:07 iloveeclipse

@merks I do not get what is different in your review request? Can you explain?

I accidentally removed him and then added him back.

Thanks for the clarification

vogella avatar Jul 03 '24 08:07 vogella

Thanks Ed! Lars, it os not as trivial as one could think, I recommend to read the code Ed referenced and related tickets. I will be able ti review this later, hopefully on Friday, as I'm two days mostly not on my workstation.

See also https://github.com/eclipse-platform/eclipse.platform/issues/1458, currently the reset of the preference sets the encoding to an undesired value. This is NOT changed by this PR, I could not find the responsible preference initializer.

vogella avatar Jul 03 '24 08:07 vogella

@vogella

My concern is that if the preference is set, then the logic at line 2357 will find a non-blank value and will not longer consider the explicit file.encoding system property:

https://github.com/eclipse-platform/eclipse.platform/blob/0f64336d2bc4c785643f38d429bd36bdbae0337e/resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/Workspace.java#L2354-L2357

When I debugged this I see we do get to org.eclipse.core.internal.resources.PreferenceInitializer.initializeDefaultPreferences() before we get to org.eclipse.core.internal.resources.Workspace.setExplicitWorkspaceEncoding() so this is definitely of concern.

In the preferences page, these things are all of interest:

  • org.eclipse.ui.ide.dialogs.ResourceEncodingFieldEditor.findDefaultEncoding()
  • org.eclipse.ui.WorkbenchEncoding.getWorkbenchDefaultEncoding()

When pressing the Restore Defaults:

  • org.eclipse.ui.ide.dialogs.ResourceEncodingFieldEditor.loadDefault()

Unfortunately when debugging it sets it to UTF-8 which is not what I see in actual IDE when I do that...

image

Anyway, more debugging is required. We need to be cautious that this seemingly innocent change doesn't actually break the logic that @iloveeclipse has carefully implemented and tested...

I see that this test failed which appears to be the effect I describe above:

image

merks avatar Jul 03 '24 12:07 merks

Thanks @merks for spending so much time on it. I also tried to find the place in which the preference page flips back to the wrong value. Maybe @iloveeclipse may find it, as he implemented the changed in the past.

vogella avatar Jul 03 '24 13:07 vogella