Summary: | TextViewer.selectionChanged(int, int) not called anymore since bug 466532 | ||
---|---|---|---|
Product: | [Eclipse Project] Platform | Reporter: | Sebastian Ratz <sebastian.ratz> |
Component: | Text | Assignee: | Platform-Text-Inbox <platform-text-inbox> |
Status: | NEW --- | QA Contact: | |
Severity: | normal | ||
Priority: | P3 | CC: | mistria, sebastian.zarnekow, thatnitind |
Version: | 4.22 | ||
Target Milestone: | --- | ||
Hardware: | All | ||
OS: | All | ||
See Also: | https://bugs.eclipse.org/bugs/show_bug.cgi?id=466532 | ||
Whiteboard: |
Description
Sebastian Ratz
2021-10-01 12:45:24 EDT
It's the grey zone of breaking change: API is not broken because code compiles, but the expectations built on top of them are, although they are not specified... But in practice, some clients are broken without them getting a clear sign that they "abuse" the API. I'm willing to fix that decently, keeping the method invoked. I imagine we can add if (regions.length == 1) { selectionChanged(regions[0].offset, regions[0].range); } either at the beginning or the end of the new selectionChange(IRegion[]) method. What do you think? I'm not sure this would be a good idea. Usually, overwriting the selectionChanged() method, is not about being notified of such an evet (that's what we already have the listeners for), but to change behavior, e.g.: @Override protected void selectionChanged(int offset, int length) { fireSelectionChanged(offset, length); firePostSelectionChanged(offset, length); // call directly (don't delay); } Just calling the method if length == 1 makes things more complicated and less consistent and modifying superclass behavior would not be possible. Not sure what the best way forward is here. We can probably get that behavior above by some other means. This broke a feature in our product after upgrading the target platform. Not sure I'd consider this a grey zone. The product targets all Eclipse versions since 4.8 and so far that worked nicely but on the latest platform, there doesn't seem to be a great way to change the implementation of selectionChanged anymore. (In reply to Sebastian Ratz from comment #2) > Just calling the method if length == 1 makes things more complicated and > less consistent and modifying superclass behavior would not be possible. I'm thinking TextViewer, around line 2334, should decide to call selectionChanged(int, int) if there is only 1 range to keep calling the possible customizations, and call selectionChanged(IRegion[]) otherwise. (In reply to Mickael Istria from comment #4) > (In reply to Sebastian Ratz from comment #2) > > Just calling the method if length == 1 makes things more complicated and > > less consistent and modifying superclass behavior would not be possible. > > I'm thinking TextViewer, around line 2334, should decide to call > selectionChanged(int, int) if there is only 1 range to keep calling the > possible customizations, and call selectionChanged(IRegion[]) otherwise. Please also consider making the equivalent to protected void selectionChanged(int offset, int length) based on ranges selectionChanged(IRegion[]), protected, too. In our case, we'd like to avoid the queuing of PostSelectionChanges in some case, so it'd be great if all three methods would be accessible in addition. updateSelectionCache(); <- currently private queuePostSelectionChanged(true); <- currently private fireSelectionChanged(widgetRanges); <- this one is already protected |