-
Notifications
You must be signed in to change notification settings - Fork 236
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Getting scroll position of CodeArea #98
Comments
See http://stackoverflow.com/a/27108942/4136325 for a workaround using selectRange() |
Hi Salman, this is not supported and I'm not sure whether and how this should be supported. There are at least two things to consider:
|
Hi, Thanks for the detailed answer. In your answer, method (c) seems like something I might be able to use. Does CodeArea have such a property that I can exploit? |
Hi, a), b) and c) were possible ways to deal with 1. None of them is currently implemented. |
Hey Tomas, |
None that I know of. I plan to remove the whole skin thing from RichTextFX whenever I can get around to it. There just doesn't seem to be any benefit to using a skin. No one is ever going to write a custom skin for RichTextFX, anyway. |
Any update about this? |
Refactoring to get rid of the skin, and thus opening the way to view-specific features like this one (but also many others) is definitely something I want to do. At the same time, as you noticed, I have other work to do elsewhere, and this isn't exactly a pressing issue for me. Regarding open issues, vast majority of them are feature requests. I try to resolve bugs quickly. I must admit that so far I haven't done a very good job of steering the community to get more involved with the RichTextFX codebase and make substantial contributions. (There were some nice contributions like xml highlighting demo and text background color, but that's about it.) |
Regarding difficulty, both finally getting rid of the skin architecture and then subsequently exposing api for scroll position (given that it will only be an estimate, anyway - see point 2. of my first comment) are relatively easy tasks (although tedious). The fact that we would only get an estimate limits the usefulness of this feature, though, and is the reason why I don't find it very important. |
Yes, thank you for fixing the bugs so quickly! One reason I haven't contributed code before is that I'm still learning many things about how JavaFX works and how to properly program something (If I was smarter, I would have probably bought a book that explained it in more depth than just the tutorials on oracle). Since this control is complex, I also haven't studied it in-depth and become familiar with how it works. So, you want to and are able to expose the scroll API relatively easily, but you don't see the point in doing so since its functionality would be limited by the font size and line wrapping. Beside that, you don't personally need the feature, and the world isn't going to end if you don't do this. :-) The following is me thinking outloud: Even if the code were to display the entire document once, calculate its height, remove the document and just display whatever fits inside Flowless, any modifications to the document programmatically wouldn't be accounted for. Does that sound about right? |
That sounds about right, except I wouldn't bother computing the width and subsequently the height of the paragraph (wrapped line) myself. For a given Anyway, the place to implement this would be One could come up with some sophisticated algorithms, where the scroll position starts with an estimate (as it does now), and then in the background gradually improves the estimate. This would result in some flickering of the scrollbars before reaching the exact value, but that might be acceptable. |
Lol... I should have known... In my program's specific use case, I don't use line wrapping, and the font size is the same for the entire document. Although I might use different font families for the font, the estimate for the scroll value would be accurate in that case, right? I'll open an issue in |
Yes, in that case the estimate would be accurate, so it is a matter of exposing totalBreadthEstimateProperty() and totalLengthEstimateProperty(). |
Just an update about this issue: the next version of Flowless (to be released soon) now exposes its scrolling API. Once released, it shouldn't take too long to further expose the API in RichTextFX. |
@TomasMikula In my first draft of exposing the scrolling API, I've come across a problem that I'm not quite sure how to resolve. I'm guessing you can further point me in the right direction. I think this might be another Skin-related problem. Exposing totalHeight/WidthEstimate isn't an issue. However, I'm not sure how to handle the estimatedScrollX/Y values. Getting their values is easy using properties, but I'm not sure how to set them. Strategy 1: using properties & action method cannot work due to no VirtualFlow reference. Adding that reference would break Separation of Concerns.StyledTextArea source code example: private final Val<Double> estimatedScrollX
public Val<Double> estimatedScrollXProperty() { return estimatedScrollX; }
// ... other code ...
public scrollXToPixel(pixel) {
// would be "virtualFlow.scrollXToPixel(pixel);" because setting the value of
// length/breadth correctly requires calling "setLength/BreadthOffset()"
// However, reference to virtualFlow is not available..
} StyledTextAreaView source code example: // during constructor process
area.estimatedScrollXProperty().bind(virtualFlow.estimatedScrollXProperty()); Strategy 2: Use just 1 property and bind bidirectionally. Getting value works fine, but no longer able to call the right setter method.StyledTextArea source code example: private final Val<Double> estimatedScrollX
public Val<Double> estimatedScrollXProperty() { return estimatedScrollX; }
public double getEstimatedScrollX() { estimatedScrollX.get(); }
public void setEstimatedScrollX(double value) { estimatedScrollX.set(value); } StyledTextAreaView source code example: // during constructor process
area.estimatedScrollXProperty().bindBidirectional(virtualFlow.estimatedScrollXProperty());
// whenever program sets area's estimatedScrollX property with new value,
// virtualFlow's scrollXToPixel(pixel) method is no longer called,
// which corrupts breadthOffset and makes its value inaccurate. Strategy 3: Use 2 properties: a getter property & a setter property. Seems like smelly code to me.StyledTextArea source code example: private final Val<Double> estimatedScrollX
public Val<Double> estimatedScrollXProperty() { return estimatedScrollX; }
private final Var<Double> scrollXSetter
Var<Double> scrollXSetterProperty() { return scrollXSetter; }
public void setScrollXToPixel(double pixel) { scrollXSetter.set(pixel); } StyledTextAreaView source code example: // during constructor process
// getter is bound to actual value
area.estimatedScrollXProperty().bind(virtualFlow.estimatedScrollXProperty());
// setter's changes updates the actual value
area.scrollXSetterProperty().values().subscribe( v -> virtualFlow.scrollXToPixel(v) ); |
So I suppose that to make Strategy 2 compile, you updated |
I found that confusing when implementing VirtualFlow, but had to do that, because that's how scrollbars work. |
These strategies were proposed, not tested. I didn't know about the pixel offset, but I can see why that would be confusing. Knowing that now, I think my assumption is a bit mistaken:
Now that you've explained the pixel offset part, I guess the horizontal scrolling position (if greater than 0) would only show extra white space. However, if set to some negative value, what would happen then? I'm not sure if the following idea has been created/implemented before in some other form (or even if the idea below is useful in this context), but what if there was a Property class that, when its class FunctionalDoubleProperty {
private double value
// some functional interface thing (not as familiar with lambdas, sorry...)
private function
public double getValue() { return value; }
public void setValue(double newValue) {
double oldValue = getValue();
double toBeSetValue = function.apply(newValue);
// fire value change events
fireValueChangeEvent(oldValue, toBeSetValue);
value = toBeSetValue;
} |
I guess the FunctionalDoubleProperty wouldn't be particularly helpful in this context because the function to call would be an instance method from VirtualFlow. Furthermore, the property, when constructed, wouldn't have access to VirtualFlow to assign such a method anyways... |
Well, to get around the negative value (duh...): public final void setEstimatedScrollX(double pixelOffset) {
if (pixelOffset < 0) pixelOffset = 0;
estimatedScrollX.setValue(pixelOffset);
} I'm going to test out Strategy 2 by changing Flowless locally according to what you thought I had already done, and see what happens when the scrollX value becomes bigger than the actual width of VirtualFlowContent. |
So, Strategy 2 actually does work to an extent. Setting the values negatively or beyond the actual width/height displays the scrolling correctly: if negative, it visually scrolls to 0.0; if positive and beyond its actual w/h, it visually scrolls to the bottom/right fully. In some ways, it might be better to create another area class that can set/get the now-exposed scrolling API. The area would be configured to adhere to the no-line-wrapping and same-font-size restraints. Then, such checking and handling would make more sense, and other users of the code that don't need this wouldn't be bothered by it. |
So, the scrolling API is basically exposed, but I was testing out what it would be like to bind a StyledTextArea to a scrollPane. When all values (vvalue, hvalue, vmax, hmax) were bound unidirectionally to area's corresponding values, the scrollPane's scrollBar flickered multiple times very rapidly (though this could have been due to the way I was testing this out). Then I tried something else and encountered a problem. If a user scrolls in either VirtualFlow or the scrollPane, both hvalue & estimatedScrollX need to be updated to the same value (the same goes for vvalue & estimatedScrollY). Unfortunately, I cannot bind these corresponding properties bidirectionally:
When I tested what changing area's estimatedScrollX from type Var to DoubleProperty would look like, I discovered that it would carry this type issue down into Flowless' Sizetracker. So, a few possible ways this could be fixed:
|
It might be that If you want to use StyledTextArea in a ScrollPane, then you just want the StyledTextArea laid out to its full size (totalWidthEstimate x totalHeightEstimate) and let ScrollPane handle the scrolling. As a start, I would just try binding StyledTextArea's |
I'm not sure I understand what you mean because that was what I was trying to do with the example you quoted. Perhaps I wasn't very clear in describing it. public void setEstimatedScrollX(double value) {
if (value < 0) value = 0;
if (value > getTotalWidthEstimate()) value = getTotalWidthEstimate();
estimatedScrollX.set(value);
} The only issue with the above solution is if
My idea is to have a ScrollPane besides the StyledTextArea where the contents of the left scrollPane line up horizontallly with the lines of StyledTextArea. The content in the left ScrollPane is too complex to use a ParagraphGraphicFactory to do the same thing: |--- ScrollPane ----|---- StyledTextArea -----|
If worse comes to worse, I could encapsulate the StyledTextArea inside a ScrollPane and bound it like the way you suggested; however, it would unnecessarily remove the memory optimization that VirtualFlow provides. |
BidirectionalBinding.bindNumber(area.estimatedScrollXProperty(), scrollPane.hvalueProperty());
BidirectionalBinding.bindNumber(area.estimatedScrollYProperty(), scrollPane.vvalueProperty()); This works, but it's via the com.sun.javafx package, which if I remember correctly will be unavailable in java 9's modularization, right? |
What I meant was to try it on virtualFlow.estimatedScrollXProperty().setValue(50000);
virtualFlow.estimatedScrollXProperty().getValue(); // does it return 50000 or 200? |
I also would like some way to get access to the scrollpane (or equivalent). My use case is a visual diff program where I would like to have two CodeAreas side by side and synchronize the scrolling between the two. |
This, too, is already possible with the snapshot release, where we abandoned the use of skin, and decoupled the scroll pane from the rest. |
I tried building the latest source, scroll bars were broken. They didn't appear at all. I believe the issue is already reported. My CodeAreas are in a SplitPane. Perhaps I didn't look hard enough for access to the scrolling after discovering that. |
@swpalmer In snapshot version you must wrap text area into |
Or use the factory method AreaFactory. embeddedCodeArea().
Snapshots are also published to the Sonatype repository and should work with a Maven/Gradle build. |
Early feedback on this new API is very appreciated. |
@TomasMikula After using this API for a while on demos and whatnot, I'm not sure if the API for AreaFactory is that useful largely because I can't modify the underlying area before it gets embedded in the VirtualizedScrollPane. For example, this has more boilerplate... VirtualizedScrollPane<CodeArea> pane = AreaFactory.embeddedCodeArea();
// tedious boilerplate just to modify the area
CodeArea area = pane.getContent();
/* modify area here.... */
Scene scene = new Scene(pane, /* dimensions */); ... than this: CodeArea area = new CodeArea();
/* modify area here.... */
Scene scene = new Scene(new VirtualizedScrollPane<>(area), /* dimensions */); Perhaps if we added a Additionally, part of the idea behind AreaFactory was to clone an area. However, now that an IMHO, I no longer think AreaFactory should exist. My only concern with going in that direction is it's not immediately obvious to new users why the ScrollBars don't appear (unless of course they read the javadoc). This concern will weaken over time as the newness of the idea becomes more of the norm. |
Should we then remove the cloning methods and move the remaining ones to the respective classes ( |
AreaFactory has 3 methods for each of the area:
There's no longer any point to having the cloning methods. I'm assuming we'd remove the constructor methods (what's the point of writing In that case, we'd only have embedding methods and I don't see the point of doing that. Subclasses (specifically, the three flavors in RTFX) of For example, I'm guessing the API would look like class StyledTextArea<PS, S> {
public VirtualizedScrollPane<StyledTextArea<PS, S>> embed() {
return new VirtualizedScrollPane<>(this);
}
} In which case, there would be a type conflict with class InlineCssTextArea extends StyledTextArea<String, String> {
// Compiler: incompatible return types
// "InlineCssTextArea" should be "StyledTextArea<PS, S>"
VirtualizedScrollPane<InlineCssTextArea> embed() {
return new VirtualizedScrollPane<>(this);
}
} |
I was thinking we would move the static embedding methods to the respective classes and keep them static. Making them instance methods might work, too, if the signature is public VirtualizedScrollPane<? extends StyledTextArea<PS, S>> embed(); and overridden in subclasses to return a more specific type. You have a point the the utility of these methods is questionable, though. |
I was wondering if using a generic like that would prevent such an issue. So, that could work. There's another subtle factor to consider. |
I agree. So static factory methods it is? |
Sounds reasonable to me. |
Actually, is it? // Given an area...
CodeArea area = //;
// Current API: wrap in a VirtualizedScrollPane
new VirtualizedScrollPane(area);
// New API ? : use a static factory method ?
CodeArea.embed(area); I guess using static factory methods would only have 2 benefits:
Additionally, we haven't yet figured out the utility of these factory methods. |
I thought we would just move the factory methods as they are, i.e. class CodeArea {
public static VirtualizedScrollPane<CodeArea> embeddedInstance();
} |
Ah... those factory methods. ok. |
I'm not sure whether you agree or not, but I still think the public static <PS, S> VirtualizedScrollPane<StyledTextArea<PS, S>> embeddedInstance(
Consumer<StyledTextArea<PS, S>> modifier,
PS initialParagraphStyle, BiConsumer<TextFlow, PS> applyParagraphStyle,
S initialTextStyle, BiConsumer<? super TextExt, S> applyStyle,
boolean preserveStyle
) {
StyledTextArea<PS, S> area = new StyledTextArea<PS, S>(
initialParagraphStyle, applyParagraphStyle,
initialTextStyle, applyStyle, preserveStyle
);
modifier.accept(area);
return new VirtualizedScrollPane<>(area);
} |
IDK, I probably wouldn't try to get too sophisticated here. For me, the main reason for these factory methods is to provide a simple and straightforward path for migration from 0.6.10 to 0.7, while preserving the behavior. Like "replace this constructor with this factory method and your area will keep having scrollbars as before." |
Why not do both? For example, InlineCssTextArea could have 4 static factory methods: // 2 normal constructors, no modifications
public static VirtualizedScrollPane<InlineCssTextArea> embeddedInstance()
public static VirtualizedScrollPane<InlineCssTextArea> embeddedInstance(String initialText)
// 2 normal constructors, with modifications post area creation
public static VirtualizedScrollPane<InlineCssTextArea> embeddedInstance(
Consumer<InlineCssTextArea> modifier
)
public static VirtualizedScrollPane<InlineCssTextArea> embeddedInstance(
String initialText,
Consumer<InlineCssTextArea> modifier
) |
See #304. Looks like we'll be keeping things mostly the same due to the issues I found there. |
I tried binding estimatedScrollY of two CodeAreas together so they would scroll together. Both had the same number of lines. It worked very poorly, lots of flickering and jumping. At the ends of the scroll range things weren't in sync but would snap to the right position if somehow another scroll event happened or something like that. |
@swpalmer Is it for CodeAreas with scrollbars (i.e. inside VirtualizedScrollPane) or without? Is word-wrapping on or off? Does the behavior improve when synchronizing (i.e. binding) only uni-directionally? |
Inside VirtualizedScrollPane. No word-wrap. One-way binding may reduce the flickering a little but does not make the problem go away. |
Does the behavior improve without |
@swpalmer I had a situation where I wanted to sync the scrolling between a VirtualizedScrollPane and a regular ScrollPane. I couldn't bind them bidirectionally because of a type conflict (VSP: Double; SP: Number). So, the solution Tomas suggested then was to use a simulated recursive binding (e.g. feeding values to one another). If his options don't work, perhaps doing something similar might: VirtualizedScrollPane<CodeArea> pane1 = //;
VirtualizedScrollPane<CodeArea> pane2 = //;
// simulated recursive binding
Subscription sub = Subscription.multi(
pane1.estimatedScrollYProperty().values().feedTo(pane2.estimatedScrollYProperty()),
pane2.estimatedScrollYProperty().values().feedTo(pane1.estimatedScrollYProperty())
); If that doesn't work, @TomasMikula would we be able to fix this by adding some sort of syncing option in Flowless? |
Since the issues remain with just one-directional syncing, I don't think that is the core of the problem. Let's wait for @swpalmer's answer whether the problem remains when no |
So this has been released in 0.7-M1. @swpalmer please open a new issue for the flickering problem. |
Well without VirtualizedScrollPane the testing becomes more difficult without scrollbars... but basically we are seeing nonsensical values for the scroll positions. They flicker all over the place while dragging a scrollbar, and when we set the scroll position of one pane in response to the change in the other, the change doesn't take right immediately anyway. I've tried bindings, listeners, and the Subscription stuff mentioned above. I will open a new issue. |
Hi!
I'm using this API for a project, and have added a CodeArea in a StackPane on top of a Canvas. I want to scroll the Canvas with the CodeArea. Is it possible to get the scroll position of the CodeArea to let me do this?
Thanks!
The text was updated successfully, but these errors were encountered: