Skip to content

Fix WorkspaceRoot encoding preference lookup and persistence - #2907

Merged
MohananRahul merged 1 commit into
eclipse-platform:masterfrom
deepika-u:use_ieclipsepreferences_instead
Sep 24, 2026
Merged

MohananRahul merged 1 commit into
eclipse-platform:masterfrom
deepika-u:use_ieclipsepreferences_instead

Conversation

@deepika-u

@deepika-u deepika-u commented Sep 3, 2026 •

Copy link
Copy Markdown
Contributor

Fixes two issues in how WorkspaceRoot reads and writes the workspace default encoding preference.

  1. getDefaultCharset() - now reads the encoding via Platform.getPreferencesService() instead of the instance-scope node
    directly, so it correctly falls back through the scope search order instead of only checking one scope.

  2. setDefaultCharset() - removed the custom override. Since WorkspaceRoot's path is always IPath.ROOT, it now inherits
    Container#setDefaultCharset(String), which already delegates to CharsetManager and reports save failures as a CoreException instead of silently logging them.

Partially fixes
#497

@deepika-u

Copy link
Copy Markdown
Contributor Author

@HeikoKlare - When you get some time, can you take a look at this please?

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Preference lookup and persistence error handling must preserve existing behavior and API contracts.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Modernizes WorkspaceRoot by replacing legacy preferences with IEclipsePreferences.

Changes:

  • Uses scoped preferences for workspace encoding.
  • Explicitly flushes updates and logs persistence failures.
File summaries
File Review
resources/bundles/org.eclipse.core.resources/src/org/eclipse/core/internal/resources/WorkspaceRoot.java Two moderate issues (2 votes each): default-scope encoding values are no longer resolved, and flush failures are logged instead of reported as CoreException per the API contract.
Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 2
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@github-actions

github-actions Bot commented Sep 3, 2026 •

Copy link
Copy Markdown
Contributor

Test Results

    54 files  ±0      54 suites  ±0   56m 9s ⏱️ -40s
 4 823 tests ±0   4 801 ✅ ±0   22 💤 ±0  0 ❌ ±0 
12 360 runs  ±0  12 207 ✅ ±0  153 💤 ±0  0 ❌ ±0 

Results for commit d1af8cb. ± Comparison against base commit a439595.

♻️ This comment has been updated with latest results.

@deepika-u
deepika-u force-pushed the use_ieclipsepreferences_instead branch 3 times, most recently from 62e824c to 3e60230 Compare September 9, 2026 11:04
@iloveeclipse
iloveeclipse requested a balanced review from Copilot September 9, 2026 11:10

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The reviewed change has no unresolved approval-blocking issues.

Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

@deepika-u deepika-u changed the title Update WorkspaceRoot to use IEclipsePreferences. Fix WorkspaceRoot encoding preference lookup and persistence Sep 9, 2026
@deepika-u
deepika-u force-pushed the use_ieclipsepreferences_instead branch from 3e60230 to fb9edf5 Compare September 10, 2026 09:37
@deepika-u

Copy link
Copy Markdown
Contributor Author

Do you think any other actions are still pending? If not, can this be approved and merged now?

@deepika-u

Copy link
Copy Markdown
Contributor Author

@iloveeclipse : Do you want me to check anything else on this pr? or can this be merged?

@deepika-u
deepika-u force-pushed the use_ieclipsepreferences_instead branch 2 times, most recently from 2eef6c6 to 68638c3 Compare September 22, 2026 05:20
@deepika-u

Copy link
Copy Markdown
Contributor Author

@iloveeclipse : can this be merged now?

@deepika-u

Copy link
Copy Markdown
Contributor Author

Looks like iloveeclipse is busy.
@merks : can you take a look at this when you get some time please?

@merks merks left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks okay. I trust you've done your homework.

@deepika-u
deepika-u force-pushed the use_ieclipsepreferences_instead branch from 68638c3 to d1af8cb Compare September 24, 2026 07:03
@MohananRahul
MohananRahul merged commit 6448359 into eclipse-platform:master Sep 24, 2026
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants