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

Added ScaledVirtualized class #16

Merged
merged 6 commits into from
Dec 4, 2015
Merged

Conversation

JordanMartinez
Copy link
Contributor

How's this?
I'm also not sure if there might be some unwanted implications of adding the Scale object to the ScaledVirtualized wrapper and not the Virtualized content itself

@JordanMartinez
Copy link
Contributor Author

One issue I came across was the lack of access to layoutChildren() after one of the scale's values changes.
For example, if I use ScaledVirtualized, it doesn't redisplay its content properly because, when the one of the scale's values changes, it's layoutChildren() method needs to be called, which isn't public.
However, if I create a class called VirtualizedScaler that extends ScaledVirtualized, and implement my code within VirtualizedScaler, I can now use layoutChildren() where needed.

With that being said, I think ScaledVirtualized should be made abstract.

@JordanMartinez
Copy link
Contributor Author

On second thought, it would be better to refactor ScaledVirtualized to call layoutChildren() whenever one of Scale's values changes.

@TomasMikula
Copy link
Member

I think the correct way is to call requestLayout() instead of layoutChildren() directly. layoutChildren() is then called from layout() during layout pass.

@TomasMikula
Copy link
Member

I didn't realize that we don't need an ObjectProperty<Scale>, because Scale's fields can be modified. Then the method is probably better named getScale() instead of scaleProperty().

@JordanMartinez
Copy link
Contributor Author

K, I've updated the code

@JordanMartinez
Copy link
Contributor Author

I wonder if the getter should be renamed. It is annoyingly close to Node#getScaleX/Y() in that a developer might use the wrong Scale object mistakenly.

@JordanMartinez
Copy link
Contributor Author

What about getVirtualizedScaler() or something like that?

@TomasMikula
Copy link
Member

You have a point. What do you think about calling it zoom instead of scale?

@JordanMartinez
Copy link
Contributor Author

Sounds good!

@JordanMartinez
Copy link
Contributor Author

I added some javadoc to help clarify the name as well

@TomasMikula
Copy link
Member

👍

TomasMikula added a commit that referenced this pull request Dec 4, 2015
Add ScaledVirtualized class.
@TomasMikula TomasMikula merged commit 98fdbb4 into FXMisc:master Dec 4, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants