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

Add FlowGroup - a generalization of HorizontalFlowGroup and VerticalFlowGroup #333

Merged
merged 1 commit into from
Sep 23, 2020

Conversation

ccmb2r
Copy link
Contributor

@ccmb2r ccmb2r commented Aug 15, 2020

This commit adds the class FlowGroup - a more versatile generalization of HorizontalFlowGroup and VerticalFlowGroup.

While I like the algorithm of both HorizontalFlowGroup and VerticalFlowGroup, it is distracting to me that the layout direction would influence the inheritance hierarchy this heavily, which causes problems when designing controls based on one of the flow groups but where the later user of the subclass should be able to specify the layout direction. The new class uses essentially the same algorithm as the existing classes but makes the layout direction configurable at runtime and adds a few tweaks.

Key differences are:

  • FlowGroup can be configured to have a layout direction as either horizontal (horizontal=true) or vertical (horizontal=false).
  • FlowGroup layout direction can be changed programmatically during runtime.
  • If available, FlowGroup uses up all necessary space in the layout direction, i.e., when horizontal=true, attempts to expand horizontally and, when horizontal=false, attempts to expand vertically (instead of using the specified width/height as before).
  • FlowGroup adds spacing only between children, but not after the last element.
  • When even the first child does not fit FlowGroup's row/column, space is no longer placed before it.

P.S.: Read and followed the contribution guidelines to the best of my ability. :)

@kotcrab
Copy link
Owner

kotcrab commented Aug 17, 2020

Given how this makes current flow groups deprecated, how about replacing uses of those group in the code base? They are used at least in drag pane, tabbed pane and tests.

This looks like a good improvement but I haven't looked at it closely yet.

@kotcrab kotcrab added the ui label Aug 17, 2020
//Let the ancestors know in hopes that they resize the group.
if (getHeight() != layoutedHeight) {
invalidateHierarchy();
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this really work in a case like this? Is it done somewhere in libGDX layouts?

private boolean horizontal;
private float spacing;

//Internal
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary comment

@ccmb2r
Copy link
Contributor Author

ccmb2r commented Sep 5, 2020

Hey kotcrab,

lost track of this for a bit, still interested in contributing though!

Regarding your questions/comments:

1a. Will the resizing logic really work?

Yes, I'd say it will. What it does is take one attempt at resizing and either use the extended size (e.g., if within a ScrollPane that allows scrolling in the layout direction and, thus, can accommodate more space) or live with the existing size as the calculation will yield the same result then.

1b. Is this done somewhere in libGDX?

Yes, the HorizontalGroup uses a similar procedure in its layoutWrapped () method to determine at which width to wrap.

2. Unnecessary comment.

I'll remove it.

3. Further integration

I was not sure how invasive contributions may be but would be happy to integrate this further into VisUI. Adapting the test would certainly make sense if you are happy with, perspectively, using FlowGroup as a replacement for the current layouts. Here are a few further thoughts on how to integrate the new changes while trying to maintain backward compatibility:

a) FlowGroup as parent of HorizontalFlowGroup and VerticalFlowGroup

One could make FlowGroup the parent of HorizontalFlowGroup and VerticalFlowGroup getting rid of their specific implementations and, perspectively, removing the specialized classes altogether. The behavior is very similar so that there would only be minor edge cases where it would affect existing applications, namely that FlowGroup does not add excess space in unfavorable resizing scenarios and that it uses up available space in the layout direction by expanding if possible. FlowGroup behaves similarly enough in a ScrollPane I'd say.

One caveat though: Both HorizontalFlowGroup and VerticalFlowGroup would inherit the methods isHorizontal () and setHorizontal (boolean), which do not make sense in their context. As a workaround, one could overwrite these methods and have them throw an UnsupportedOperationException with a pointer to the new FlowGroup class.

In this particular use case, one could leave the current classes in DragPane and TabbedPane until, ultimately, HorizontalFlowGroup and VerticalFlowGroup are removed from VisUI (at which point, FlowGroup would be used in their place).

b) Replacing HorizontalFlowGroup and VerticalFlowGroup in existing VisUI code

It would certainly be possible to replace the use of HorizontalFlowGroup and VerticalFlowGroup in existing VisUI code, i.e., in DragPane and TabbedPane (which seem to be the only internal uses apart from tests). The caveat might be if existing users depend on these particular classes in their extensions. However, it seems that the concrete classes are mostly encapsulated in existing controls. TabbedPane should be relatively easy to adapt without effects on existing users. DragPane still has some deprecated methods that expose the concrete implementation classes HorizontalFlowGroup and VerticalFlowGroup as well as their superseding methods that determine whether the DragPane uses a horizontal or vertical flow. I could certainly adapt the latter but keeping the prior would pose a problem unless the solution under a) was used as a temporary workaround.

I'd be happy to hear your thoughts on this and proceed accordingly.

@kotcrab kotcrab merged commit 69c2d50 into kotcrab:master Sep 23, 2020
@kotcrab
Copy link
Owner

kotcrab commented Sep 23, 2020

I gave this some testing and looks okay! I merged it and renamed horizontal to vertical. vertical parameter is used for other widgets too.

I did change the test to use our new flow group but left DragPane/TabbedPane alone as that could cause breaking changes. TabbedPane exposes DragPane so it's not entirely internal implementation detail.
I'm okay with deprecating HorizontalFlowGroup, VerticalFlowGroup leaving them as is and removing them in next major version.

@ccmb2r
Copy link
Contributor Author

ccmb2r commented Oct 1, 2020

Sounds great, thanks!

@ccmb2r ccmb2r deleted the flowgroup branch October 1, 2020 19:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants