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

Bug 356262

Summary: Make default auto edit strategy configurable via preferences
Product: [Modeling] TMF Reporter: Henrik Lindberg <henrik.lindberg>
Component: XtextAssignee: Project Inbox <tmf.xtext-inbox>
Status: NEW --- QA Contact:
Severity: enhancement    
Priority: P3 CC: dtn-eclipsebugs, moritz.eysholdt, sebastian.zarnekow, sven.efftinge
Version: unspecified   
Target Milestone: ---   
Hardware: All   
OS: All   
Whiteboard:
Attachments:
Description Flags
preference page
none
constants
none
helper
none
overriding edit strategy provider none

Description Henrik Lindberg CLA 2011-08-30 20:18:44 EDT 
It is useful to be able to control the auto edit strategies via user preferences. I made such an implementation in the Geppetto development environment for Puppet and would like to contribute it to Xtext.

Before doing so, I have some questions regarding the approach. My current implementation makes use of the DefaultEditStrategyProvider (and naturally its superclass AbstractEditStrategyProvider). The result would have been cleaner if I had replaced both of these with my own implementation (but i did not know this when I started).

In my implementation, I have delegated some of the preference responsibilities to a preference helper class. This is not needed - it could just as well go directly to the preference store.

I have attached the classes for review, and like some feedback / discussion. (Or just take what I did and change it...).
(Attachments to follow).
Comment 1 Henrik Lindberg CLA 2011-08-30 20:27:02 EDT 
Created attachment 202467 [details]
preference page
Comment 2 Henrik Lindberg CLA 2011-08-30 20:27:32 EDT 
Created attachment 202468 [details]
constants
Comment 3 Henrik Lindberg CLA 2011-08-30 20:27:56 EDT 
Created attachment 202469 [details]
helper
Comment 4 Henrik Lindberg CLA 2011-08-30 20:31:01 EDT 
Created attachment 202470 [details]
overriding edit strategy provider

The PPEditStrategyProvider is the "messy" part, that wraps strategies in a wrapper that is aware of preferences. The complexity is because I wanted to reuse the default implementation. Taking over all the responsibility would be much cleaner.
Comment 5 Henrik Lindberg CLA 2011-08-30 20:38:17 EDT 
Cleanest API compatible implementation is probably to:
- use separate preference keys for the different strategies
- use a wrapper that knows about the preference key per strategy
- make the abstract edit strategy aware that strategies may be wrapped (like in the overriding implementation that is attached).

Or (breaking API)
Add an Supplier<Boolean> enabler parameter to the edit strategies themselves (then there is no need for a wrapper). The strategies simply need to call get() on the Supplier to know if they are enabled or not.
Comment 6 Sven Efftinge CLA 2011-08-31 01:48:24 EDT 
Since some auto edit strategies depend on others I'd expect surprising behavior if I disable some of them. Didn't you face such situations?
Comment 7 Henrik Lindberg CLA 2011-08-31 07:24:59 EDT 
(In reply to comment #6)
> Since some auto edit strategies depend on others I'd expect surprising behavior
> if I disable some of them. Didn't you face such situations?

No, seems to work just fine - I disable all strategies for a particular character pair. Anything in particular you think should be tested to verify that there is no negative effect?
Comment 8 Sebastian Zarnekow CLA 2011-10-17 16:40:38 EDT 
Unfortunately we didn't find the time to review the attached implemetation for 2.1. 

Is that a worthy addition for 2.1.1?
Comment 9 Sven Efftinge CLA 2011-10-18 02:07:48 EDT 
I think so. We should at least review it.
Comment 10 Christian Dietrich CLA 2016-08-17 09:40:09 EDT 
*** Bug 391639 has been marked as a duplicate of this bug. ***