Bug 172018 - [painting][preferences] configurable alpha level for whitespace character rendering
[painting][preferences] configurable alpha level for whitespace character ren...
Status: VERIFIED FIXED
Product: Platform
Classification: Eclipse
Component: Text
3.3
All All
: P3 enhancement with 13 votes (vote)
: 3.7 M3
Assigned To: Deepak Azad CLA Friend
:
: 172076 238117 262916 (view as bug list)
Depends on:
Blocks:
  Show dependency tree
 
Reported: 2007-01-29 10:07 EST by Markus Keller CLA Friend
Modified: 2010-10-26 07:53 EDT (History)
21 users (show)

See Also:
daniel_megert: review+


Attachments
fix (20.89 KB, text/plain)
2010-10-19 11:03 EDT, Deepak Azad CLA Friend
no flags Details
fix (22.46 KB, patch)
2010-10-20 09:02 EDT, Deepak Azad CLA Friend
no flags Details | Diff
fix (23.75 KB, patch)
2010-10-20 09:42 EDT, Deepak Azad CLA Friend
no flags Details | Diff
last fix + mnemonic (23.75 KB, text/plain)
2010-10-20 10:03 EDT, Deepak Azad CLA Friend
daniel_megert: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Markus Keller CLA Friend 2007-01-29 10:07:31 EST
Follow-up to bug 22712 comment 73:

>It looks like this is currently specified by "gc.setAlpha(100);" at line 138 in
>my patched InvisibleCharacterPainter.java (against 3.3M3).  I could see this
>fitting quite well on the preference page, right next to the "Show whitespace
>characters" checkbox, just like the "print margin column" and "hyperlink style
>navigation key modifier" textboxes.

In contract to Mark's original comment, I personally think it should be a bit lighter than 100 by default.
Comment 1 Mark A. Ziesemer CLA Friend 2007-01-29 11:20:38 EST
I was actually attempting to work on a patch for this.  Adding another property and removing the hard-code is easy.  Presenting it as an option on the GUI is the difficult part...

For one, the "General / Editors / Text Editors" preferences page is already filling up.  That, and it's not currently scrollable, such that resizing the window can easily cause options to apparently disappear.  I had considered adding an "alpha" box below "Show whitespace characters", just as "Print margin column" is currently below "Show print margin".

I'm also scared of using text boxes to accept a numeric example.  The validators that are currently being used to display invalid input at the top of the dialog are cool, but easily fooled.  For example, enter "2a" into "Displayed tab width".  This will correctly display an error.  Enter "200a" into "Undo history size" - another correct error.  Change "200a" back to "200".  All errors are "gone", even though "2a" is still incorrect.  The page even will "Apply" or "OK" without any warnings or errors.

Also, can we reasonably expect all Eclipse users to also be familiar with alpha channels?  I'm thinking a slider would be better than a text box, which would restrict properly restrict the input to a number within the valid range.  Even better would be to have a paired slider / text box such that the numeric value can still be viewed/entered directly - with proper checks to only accept numeric values, etc.  Such a dialog could also display a sample of the selected value.

I've been waiting for visible whitespace character support in Eclipse for a long time.  One of my biggest annoyances while working in code is inconsistent formatting - tabs vs. spaces, etc.  This is a feature that I normally leave on, but depending upon the type of file / type of coding I'm working on, I may want the whitespace characters a little more or less visible - and may want to change it quite often.  And as Markus stated, I think they're a little dark by default (though this effect is definitely dependent upon the display and other color settings being used.)  I'm thinking it'd be cool to have an "alpha selection dialog" openable from the preferences page, and displayable as a drop-down from the toolbar icon.  Just dreaming...  :-)

Just some thoughts!
Comment 2 Dani Megert CLA Friend 2007-01-29 12:13:54 EST
What you mention regarding the error state not updating correctly is just a bug. Can you please file this against Platform Text? Thanks.
Comment 3 Mark A. Ziesemer CLA Friend 2007-01-29 12:51:43 EST
(Posted bug 172031 to address comment #2.)
Comment 4 Dani Megert CLA Friend 2007-01-30 02:23:53 EST
*** Bug 172076 has been marked as a duplicate of this bug. ***
Comment 5 Dani Megert CLA Friend 2007-01-30 02:26:12 EST
Is the alpha level enough or would we also want to set the color?
Comment 6 Markus Keller CLA Friend 2007-01-30 04:04:09 EST
> Is the alpha level enough or would we also want to set the color?

I don't care too much. But adding an entry to the "Appearance color options" list on the preference page would give full flexibility and also avoid more widgets on the page. The "System Default" for "Whitespace color" then be based on the Foreground color (with a light alpha).
Comment 7 Dani Megert CLA Friend 2007-01-30 04:17:51 EST
The problem is that we cannot extend the 'Colors and Fonts' preference page i.e. we'd need to separate alpha and color.
Comment 8 Ahti Kitsik CLA Friend 2007-01-30 05:06:59 EST
To fight with overbloated Text Editors pref page (provided transparency pref patch adds one extra row) one could split it into two tabs: "General" and "Appearance".
Current situation would result in 8 rows + appearance color option for Appearance" and 5 rows for "General". But tab itself takes one row so not very huge benefit :)

PS. Should I move my patch from "DUPLICATE" bug 172076 to here?
Comment 9 Markus Keller CLA Friend 2007-01-30 05:12:17 EST
(In reply to comment #7)
I would add the setting to the color list on the Text Editor preference page. I would just add it as another color setting (no explicit alpha). Only the default setting would use an (implicit) alpha level of the current foreground color.
Comment 10 Dani Megert CLA Friend 2007-01-30 05:13:44 EST
The best solution is to
1. add it to the "Appearance color options" list
2. add an alpha entry field to the right which is only visible when the
   whitespace color list item is selected

Markus, I would not abuse the "system default" as there isn't such a color in the system.

>PS. Should I move my patch from "DUPLICATE" bug 172076 to here?
No, as I assume an improved patch will be attached here ;-)
Comment 11 Markus Keller CLA Friend 2008-06-26 05:34:00 EDT
*** Bug 238117 has been marked as a duplicate of this bug. ***
Comment 12 Haw-Bin Chai CLA Friend 2008-10-05 11:10:08 EDT
I'm very much looking forward to this feature.
Comment 13 C. Kuhlmann CLA Friend 2008-10-18 18:13:58 EDT
I'm too waiting for this feature. 

To save space in the preferences, why not use a percentage selector:
"Whitespace characters: [0%-100%] visible"
where [0%-100%] is a control that allows values from 0% (do not render any whitespaces) to 100% (like it is now) in some reasonable steps. I'm thinking of a drop-down or something. This would replace the seperate checkbox to enable the rendering of whitespaces.
Comment 14 Dani Megert CLA Friend 2009-01-30 01:25:59 EST
*** Bug 262916 has been marked as a duplicate of this bug. ***
Comment 15 Rogan Creswick CLA Friend 2009-07-20 19:26:41 EDT
Couldn't "Visible Whitespace" just be added to the Colors & Fonts list, in the "Basic" category?

*any* solution would be preferable to waiting another two years :).  Has anyone ported the Ahtik patch to 3.4/3.5? (http://ahtik.com/blog/2007/01/30/new-feature-whitespace-coloralpha-blending-transparency/ )
Comment 16 Dani Megert CLA Friend 2009-07-29 09:29:02 EDT
Ahti, would you be willing to tweak your feature according to comment 10 and attach a patch here?
Comment 17 Ahti Kitsik CLA Friend 2009-07-30 18:53:45 EDT
Dani, thanks for the reminder! I'll put it to my TODO list and make it happen at one point asap.
Comment 18 Adrian Dvergsdal CLA Friend 2010-01-14 05:15:17 EST
Hi, what is the status on this feature? 

I would like to dim down the contrast on whitespace characters now, does an alternative solution exist? :-)
Comment 19 bugzilla CLA Friend 2010-05-13 13:39:21 EDT
I'm also interested if there is a current patch to allow a more tolerable whitespace character color in Eclipse.
Comment 20 Andrea Dessì CLA Friend 2010-08-05 09:23:43 EDT
Hi all,

It would be great to have the possibility to choose foreground and background color to use for whitespace characters.

It's very difficult to recognize them now. :)

And yes, I think users don't care so much where that options will be placed.
Comment 21 Markus Keller CLA Friend 2010-10-07 08:39:54 EDT
Now that we have the Show Whitespace Characters dialog, we could just add a text field there to configure the alpha level. Add something like
"(0 is transparent and 255 is opaque)" to the label.

Alpha level is currently hardcoded in WhitespaceCharacterPainter#handleDrawRequest(..) and needs a new preference property.
Comment 22 Deepak Azad CLA Friend 2010-10-18 09:50:14 EDT
(In reply to comment #21)
> Now that we have the Show Whitespace Characters dialog, we could just add a
> text field there to configure the alpha level. Add something like
> "(0 is transparent and 255 is opaque)" to the label.
Agree, a single text field should do the job.
Comment 23 Deepak Azad CLA Friend 2010-10-19 11:03:14 EDT
Created attachment 181188 [details]
fix
Comment 24 Deepak Azad CLA Friend 2010-10-19 11:05:45 EDT
(In reply to comment #0)
> In contract to Mark's original comment, I personally think it should be a bit
> lighter than 100 by default.
I think a default of 80 would be better (though the patch still has the default as 100)
Comment 25 Andrea Dessì CLA Friend 2010-10-19 11:27:47 EDT
(In reply to comment #24)
> I think a default of 80 would be better (though the patch still has the default
> as 100)

yes, I agree. 80 would be better as default value.

--
Andrea
Comment 26 Dani Megert CLA Friend 2010-10-20 07:01:01 EDT
Looks good and works. Some issues I found:

You can't just delete an API method, even if it was added recently. Mark it @deprecated with a comment that it will go away right after M3.

There's a problem in the two actions in case the store is null: the painting   will not work (this is not just a bug with the new setting)
==> suggest to use the old constructor in case the store is 'null'

See http://en.wikipedia.org/wiki/Alpha_compositing
==> alpha value (not alpha level)

Dialog:
- remove "Configure ".
- entry field should be smaller (leave space for 4 or 5 digits).

Change the default to 80.
Comment 27 Deepak Azad CLA Friend 2010-10-20 09:02:49 EDT
Created attachment 181284 [details]
fix

(In reply to comment #26)
> Looks good and works. Some issues I found:
> 
> You can't just delete an API method, even if it was added recently. Mark it
> @deprecated with a comment that it will go away right after M3.
hmm.. it was added in M3, anyway deprecated it for now.
 
> There's a problem in the two actions in case the store is null: the painting  
> will not work (this is not just a bug with the new setting)
> ==> suggest to use the old constructor in case the store is 'null'
Because the store is null, we do not know if the whitespace characters were turned on or not. Hence we disable the painter which is also the default preference. Or am I missing something?
Comment 28 Dani Megert CLA Friend 2010-10-20 09:25:28 EDT
> Because the store is null, we do not know if the whitespace characters were
> turned on or not. Hence we disable the painter which is also the default
> preference. Or am I missing something?
Yes, the action can be used/run without a store. Please prepare a new patch.
Comment 29 Deepak Azad CLA Friend 2010-10-20 09:42:26 EDT
Created attachment 181286 [details]
fix
Comment 30 Dani Megert CLA Friend 2010-10-20 09:57:28 EDT
Looks good. Please add a mnemonic to:
TextEditorDefaultsPreferencePage_transparencyLevel=Transparency level (0 is transparent and 255 is opaque):
Comment 31 Deepak Azad CLA Friend 2010-10-20 10:03:08 EDT
Created attachment 181289 [details]
last fix + mnemonic

Dani, please commit this to HEAD.
Comment 32 Dani Megert CLA Friend 2010-10-20 10:10:43 EDT
(In reply to comment #31)
> Created an attachment (id=181289) [details] [diff]
> last fix + mnemonic
> 
> Dani, please commit this to HEAD.

Done.
Comment 33 Rajesh CLA Friend 2010-10-26 07:52:43 EDT
Verified in I20101025-1800.