Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 369558

Summary:

[build path] Source Attachment dialogs should leave encoding combo empty if none is set

Product:

[Eclipse Project] JDT

Reporter:

Markus Keller <markus.kell.r>

Component:

Assignee:

Markus Keller <markus.kell.r>

Status:

VERIFIED FIXED

QA Contact:

Severity:

normal

Priority:

CC:

daniel_megert, deepakazad, markus.kell.r

Version:

3.8

Target Milestone:

3.8 M6

Hardware:

All

OS:

All

Whiteboard:

Attachments:

Description	Flags
Fix	none

Description Markus Keller

2012-01-24 12:24:16 EST

follow-up to bug 368015

We currently show the default workspace encoding if no encoding is set, but this is not always correct. We should leave the encoding combo empty if none is set.

SourceMapper.findSource(String) line: 1030 calls IFile.getCharset(true), which also inspects the contents of the file if no encoding was specified. If the source file has a BOM, then the encoding is determined from the BOM. This makes sense.

In my test, I had a UTF-16 file that was read correctly, although the source attachment dialog said I used MacRoman (which is the default on my system).

Comment 1 Markus Keller

2012-01-24 15:37:14 EST

Important point is that my source attachment was a folder (external in my case, but that doesn't matter).

Another issue is that for internal folders, the workspace encoding is consulted first, so there are cases where we don't even have a single default encoding.

I find it confusing to have information that is not reliable, and in case the encoding is editable, showing the default is even information loss (explicit encoding is not the same as default encoding).

Once we don't show the default any more, then the encoding field should be completely hidden if it's not editable.

See IClasspathAttribute.SOURCE_ATTACHMENT_ENCODING and bug 369595 for the lookup sequence.

Comment 2 Deepak Azad

2012-01-25 01:58:28 EST

- Even when we show the encoding attribute because it is explicitly set in the .classpath file, for an external folder containing many files with different encodings what we show (in the source attachment dialog) would still be wrong...

- Showing the default encoding has the benefit that when the user is adding a source attachment for the first time, he sees the default encoding already filled in - which should be good enough in most cases.

- Showing the default encoding has another benefit - it is already implemented :-)

Comment 3 Markus Keller

2012-01-25 10:08:51 EST

Moving to M6, since the solution is not yet clear and the current code is acceptable.

Another idea to solve the problem: Fill the combo with "Default (CpWhatever)" when no encoding has been set. That makes an explicitly set encoding distinguishable from the default, but still shows the default, without causing even more UI bloat.

Comment 4 Deepak Azad

2012-02-01 06:52:57 EST

Ping. Do we have an agreement here?

My vote is to let things be.

Comment 5 Markus Keller

2012-02-01 10:17:10 EST

Created attachment 210376 [details]
Fix

> My vote is to let things be.

-1, the current solution has even more problems. E.g. in a UTF-8 workspace, I cannot set up a project where I want a source attachment to be UTF-8 for everyone, since choosing UTF-8 removes the attribute (i.e. sets the default). So we really need a distinction between default and explicit state.

This patch implements comment 3. Any objections?

Comment 6 Deepak Azad

2012-02-01 10:25:03 EST

(In reply to comment #5)
> This patch implements comment 3. Any objections?
None from me.

Comment 7 Markus Keller

2012-02-01 15:03:03 EST

Pushed as commit 31088ad8e4ac707ee63170f1e619e48ddd723915.

Comment 8 Dani Megert

2012-02-02 05:16:59 EST

(In reply to comment #5)
> Created attachment 210376 [details] [diff]
> Fix
> 
> > My vote is to let things be.
> 
> -1, the current solution has even more problems. E.g. in a UTF-8 workspace, I
> cannot set up a project where I want a source attachment to be UTF-8 for
> everyone, since choosing UTF-8 removes the attribute (i.e. sets the default).
> So we really need a distinction between default and explicit state.
> 
> This patch implements comment 3. Any objections?

(In reply to comment #7)
> Pushed as commit 31088ad8e4ac707ee63170f1e619e48ddd723915.

Looks good to me.