Skip to content
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

Closed
salmanarif opened this issue Nov 22, 2014 · 62 comments
Closed

Getting scroll position of CodeArea #98

salmanarif opened this issue Nov 22, 2014 · 62 comments

Comments

@salmanarif
Copy link

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!

@ghost
Copy link

ghost commented Nov 25, 2014

See http://stackoverflow.com/a/27108942/4136325 for a workaround using selectRange()

@ghost ghost mentioned this issue Nov 25, 2014
@TomasMikula
Copy link
Member

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:

  1. The implementation of controls in JavaFX is divided into two parts: the model (which is the Control subclass itself) and the visual representation (which is implemented by a Skin). The client code interacts only with the model. The model for CodeArea only contains the lines (composed of segments of styled text), the caret position and the selection range. Visual representation and interacting with CodeArea is the responsibility of the skin. CodeArea itself does not know things like the scroll position. It cannot even reliably get this information from the skin, since there might be no skin (e.g. when the CodeArea is not part of scene graph), or the user might replace the default skin by her own skin implementation (although unlikely for such a complex control like CodeArea, this is how JavaFX controls are designed). I can think of a few ways of addressing this, neither of which I really like:

    a) Make the scroll position part of the model. This pixel-based navigation doesn't work well at all with logical navigation based on the position in text.
    b) Implement the visual representation in the Control subclass itself, don't use any skins. This breaks the separation of concerns.
    c) Instead of double getScrollY(), have a property ObjectProperty<ChangeListener<Double>> onScrollYProperty() that the client could set to a listener and the skin would notify the listener (or not, if someone uses a custom skin that does not do that). This means you cannot ask for the current value of scrollY, but you can get notified when scrollY changes.

  2. Not even the skin, which implements the visual representation, knows the exact scroll position. This is because not the whole document is laid out. If you are at the end of a long document, the paragraphs (lines) that are not in the viewport are not in the scene graph at all, and thus their actual heights are unknown. This is an important optimization. The scrollbars that you see are just an estimate, based on the average height of currently visible lines. In case of CodeArea, this is actually accurate, since all lines have the same height, but that is not true in the general case of variable height paragraphs.

@salmanarif
Copy link
Author

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?

@TomasMikula
Copy link
Member

Hi,

a), b) and c) were possible ways to deal with 1. None of them is currently implemented.

@JordanMartinez
Copy link
Contributor

Hey Tomas,
Did anything ever come out of the conversation about the "Control" suggestion in The trouble with Skins?

@TomasMikula
Copy link
Member

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.

@JordanMartinez
Copy link
Contributor

Any update about this?
Also, considering the number of issues that are currently open, the other work you are doing elsewhere, the difficulty of fixing this issue, and the length with which this has been open, how important this is to you?

@TomasMikula
Copy link
Member

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.)

@TomasMikula
Copy link
Member

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.

@JordanMartinez
Copy link
Contributor

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:
As for the issue itself, I guess, assuming that line wrapping is on, the code would need to calculate how wide a line is, then calculate how many lines it should use to display that one line based on the control's width. After that, it would need to calculate the height of each of these lines probably by checking each style used in every segment of that line. Once calculated, this new value, along with every other height value and the constant line spacing value for every line minus one, would be summed up into the scroll value's max. Then the scrollbar's display would need to be adjusted to accordingly.
However, I'm not sure how the line's width could be calculated. The easy way is to display the line itself, but that can't be done in this case. Monospace could be used, but that would limit the font-family used. Besides, CodeArea would already do most of this anyways...

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?

@TomasMikula
Copy link
Member

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 width, TextFlow.prefHeight(width) computes that. The problem is that this can already be quite expensive for very large documents.

Anyway, the place to implement this would be Flowless.

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.

@JordanMartinez
Copy link
Contributor

I wouldn't bother computing the width and subsequently the height of the paragraph (wrapped line) myself. For a given width, TextFlow.prefHeight(width) computes that.

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 Flowless for this.

@TomasMikula
Copy link
Member

Yes, in that case the estimate would be accurate, so it is a matter of exposing totalBreadthEstimateProperty() and totalLengthEstimateProperty().

@JordanMartinez
Copy link
Contributor

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.

@JordanMartinez
Copy link
Contributor

@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.
Any ideas?

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) );

@TomasMikula
Copy link
Member

So I suppose that to make Strategy 2 compile, you updated virtualFlow.estimatedScrollXProperty() to return a Var so that it is settable, am I correct? And it somehow works, but is inaccurate? This looks like the difference between a position and pixel offset—they are not the same thing. Turns out virtualFlow.estimatedScrollXProperty() is not pixel offset, but an abstract position, the same kind of thing that scrollbars use. If your content is 1000px long and the position ranges between 0.0 and 1000.0 (could as well be 0.0-1.0, it is not directly related to pixels) and you scroll to the end, your position will be 1000.0 (because you are at the end), but your pixel offset will be, say 600, if your viewport is 400px long.

@TomasMikula
Copy link
Member

I found that confusing when implementing VirtualFlow, but had to do that, because that's how scrollbars work.

@JordanMartinez
Copy link
Contributor

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:

  • I assumed that Strategy 2 wouldn't ultimately work because if a user could set the estimatedScrollX value to an impossible value, it would screw up the visual (somehow). For example, if the user sets the value to -30.0 or 50 pixels beyond the width, wouldn't that screw up the visual? I didn't test that out personally to see what would happen, only assumed that there would be a better way to handle it somehow.

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 setValue(value) method is called, it applies some function to value first and then sets its actual value to the result of that function? So something like:

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;
}

@JordanMartinez
Copy link
Contributor

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...

@JordanMartinez
Copy link
Contributor

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.

@JordanMartinez
Copy link
Contributor

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.
However, the getEstimatedScrollX/Y value will return a false value. For example, if I set the estimatedScrollX value to 50,000 despite the actual width only being 200 pixels and then call getEstiamtedScrollX(), the returned value will be 50,000. If the value is limited by the totalWidthEstimate, it might be more accurate, but it also might prevent a user from fully scrolling to the right.

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.

@JordanMartinez
Copy link
Contributor

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:

  • area.estimatedScrollXProperty().bindBidirectional(scrollPane.hvaleProperty()); doesn't work because Var's bindBidirectional(Property<Double>) cannot be applied to DoubleProperty.
  • Likewise, scrollPane.hvalueProperty().bindBidirectional(area.estimatedScrollXProperty()); doesn't work because DoubleProperty's bindBidirectional(Property<? extends Number>) cannot be applied to Var.

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:

  • at SizeTracker's level
  • if possible, add another bindBidirectional() method to Var that accounts for this issue

@TomasMikula
Copy link
Member

For example, if I set the estimatedScrollX value to 50,000 despite the actual width only being 200 pixels and then call getEstiamtedScrollX(), the returned value will be 50,000.

It might be that VirtualFlow's estimatedScrollX does get updated to 200, but it is not propagated through the bidirectional binding. Can you test that by setting VirtualFlow's scrollX way beyond the width?

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 prefWidthProperty and prefHeightProperty to totalWidthEstimate and totalHeightEstimate, respectively.

@JordanMartinez
Copy link
Contributor

It might be that VirtualFlow's estimatedScrollX does get updated to 200, but it is not propagated through the bidirectional binding. Can you test that by setting VirtualFlow's scrollX way beyond the width?

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.
The goal was to create an area with some amount of text that requires the horizontal scrollbar to appear. So, if the actual width of the area was 200 pixels (or something higher), and I set the estimatedScrollX to 50,000, it would scroll just enough to the right so that the last character is visible, but wouldn't scroll any farther than that. However, if I later got estimatedScrollX, it would still hold the original 50,000 value. The same would go for any negative values: it would scroll so that the leftmost character was visible, but the getEstimatedScrollX() method would return a negative value.
So, I limited it like so:

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 totalWidthEstimate is inaccurate. Then, this limiter would actually prevent scrolling to the end. Such an inaccuracy would only arise if the code doesn't adhere to the two constraints (no line wrapping & same sized font used throughout entire area).

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 prefWidthProperty and prefHeightProperty to totalWidthEstimate and totalHeightEstimate, respectively.

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 -----|
|...... content ....... | .......... content ............ |

...... content ....... .......... content ............

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.

@JordanMartinez
Copy link
Contributor

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?

@TomasMikula
Copy link
Member

What I meant was to try it on VirtualFlow directly (i.e. without the proxy property on StyledTextArea):

virtualFlow.estimatedScrollXProperty().setValue(50000);
virtualFlow.estimatedScrollXProperty().getValue(); // does it return 50000 or 200?

@swpalmer
Copy link

swpalmer commented May 9, 2016

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.

@TomasMikula
Copy link
Member

TomasMikula commented May 9, 2016

This, too, is already possible with the snapshot release, where we abandoned the use of skin, and decoupled the scroll pane from the rest.

@swpalmer
Copy link

swpalmer commented May 9, 2016

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.

@alt-grr
Copy link

alt-grr commented May 9, 2016

scroll bars were broken. They didn't appear at all.

@swpalmer In snapshot version you must wrap text area into VirtualizedScrollPane to get scrollbars.

@TomasMikula
Copy link
Member

Or use the factory method AreaFactory. embeddedCodeArea().

I tried building the latest source

Snapshots are also published to the Sonatype repository and should work with a Maven/Gradle build.

@TomasMikula
Copy link
Member

Early feedback on this new API is very appreciated.

@JordanMartinez
Copy link
Contributor

@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 Consumer<AreaType> as a parameter, this wouldn't be as tedious.

Additionally, part of the idea behind AreaFactory was to clone an area. However, now that an EditableStyledDocument is public, the clone methods in the factory are somewhat irrelevant.

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.

@TomasMikula
Copy link
Member

Should we then remove the cloning methods and move the remaining ones to the respective classes (StyledTextArea, CodeArea, ...)?

@JordanMartinez
Copy link
Contributor

JordanMartinez commented May 9, 2016

Should we then remove the cloning methods and move the remaining ones to the respective classes (StyledTextArea, CodeArea, ...)?

AreaFactory has 3 methods for each of the area:

  1. Cloning methods
  2. Constructor methods
  3. Embedding methods (embed original / embed a cloned version of an area / embedded 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 CodeArea.newInstance(/* args */); as opposed to new CodeArea(/* args */);)? Those were really only there to conveniently provide a common place to construct an area.

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 StyledTextArea would have type issues with those methods.

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 StyledTextArea#embed.

class InlineCssTextArea extends StyledTextArea<String, String> {

    // Compiler: incompatible return types 
    //  "InlineCssTextArea" should be "StyledTextArea<PS, S>"
    VirtualizedScrollPane<InlineCssTextArea> embed() {
        return new VirtualizedScrollPane<>(this);
    }
}

@TomasMikula
Copy link
Member

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.

@JordanMartinez
Copy link
Contributor

static embedding methods to the respective classes and keep them static. Making them instance methods might work, too, if the signature is

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.
Since a node can only be in one other node's children at a time, if a developer uses embed() twice mistakenly, the first pane won't have the area anymore. In JavaFX, it's always the node's parent that removes a node from its children, but this API would allow a child to remove itself from its parent, which isn't done elsewhere in JavaFX (AFAIK).

@TomasMikula
Copy link
Member

I agree. So static factory methods it is?

@JordanMartinez
Copy link
Contributor

Sounds reasonable to me.

@JordanMartinez
Copy link
Contributor

Actually, is it?
There's two ways of writing the code:

// 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:

  1. It's shorter than the current API
  2. People are more likely to find the embed method since it's in the same class than VirtualizedScrollPane. We could link to it via javadoc, but we already do that with VirtualizedScrollPane.

Additionally, we haven't yet figured out the utility of these factory methods.

@TomasMikula
Copy link
Member

I thought we would just move the factory methods as they are, i.e.

class CodeArea {
    public static VirtualizedScrollPane<CodeArea> embeddedInstance();
}

@JordanMartinez
Copy link
Contributor

Ah... those factory methods. ok.

@JordanMartinez
Copy link
Contributor

I'm not sure whether you agree or not, but I still think the Consumer<AreaType> should be included. So how about this?

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);
}

@TomasMikula
Copy link
Member

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."

@JordanMartinez
Copy link
Contributor

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
)

@JordanMartinez
Copy link
Contributor

See #304. Looks like we'll be keeping things mostly the same due to the issues I found there.

@swpalmer
Copy link

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.

@TomasMikula
Copy link
Member

@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?

@swpalmer
Copy link

Inside VirtualizedScrollPane. No word-wrap. One-way binding may reduce the flickering a little but does not make the problem go away.

@TomasMikula
Copy link
Member

Does the behavior improve without VirtualizedScrollPane?

@JordanMartinez
Copy link
Contributor

@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?

@TomasMikula
Copy link
Member

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 VirtualizedScrollPane is used.

@TomasMikula
Copy link
Member

So this has been released in 0.7-M1. @swpalmer please open a new issue for the flickering problem.

@swpalmer
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants