kotcrab / vis-ui

libGDX UI toolkit
Apache License 2.0
716 stars 128 forks source link

[VisUI] Root group parameter for ToastManager #279

Closed metaphore closed 6 years ago

metaphore commented 6 years ago

ToastManager is parametrized with Stage and adds new toasts into stage's root group. But unfortunately this leads to the overlapping problem when there is another kind of generic widgets that are added to the stage just as into the hip. I usually experience it when new dialog appears on the stage (e.g. LML Autumn creates them on the stage's root).

From the ToastManager code I see this should not be a problem to use Group field instead of Stage to host toasts into it. Like we can keep old constructor, but just extract root group from the stage for an old code compatibility:

private final Group root;

public ToastManager (Group root) {
    this.root = root;
}

public ToastManager (Stage stage) {
    this.root = stage.getRoot();
}

I can prepare PR if you would like, but I just feel like it's better to discuss first.

kotcrab commented 6 years ago

ToastManager is using Stage to get width and height of it using: stage.getWidth() / getHeight() so alignment to edges can be done. stage.getRoot().getWidth() returns 0 because Group does not implement calculating it if I remember correctly.

Not sure how to handle this.

metaphore commented 6 years ago

Oh, you're right stage's root always has zero size, my mistake. Hm, the shortest solution I can think of:

public ToastManager (Stage stage) {
    WidgetGroup toastHost = new WidgetGroup();
    toastHost.setFillParent(true);
    stage.addActor(toastHost);
    this.root = toastHost;
}

This should work for both compatibility and resize sakes.

kotcrab commented 6 years ago

Yep, I think this will work. I also thought about using root.getStage() but yours allows to do alignment within group bounds properly. However the alignment code will need to be changed, it now must use Group X/Y position.

PR would be greatly appreciated. Please also update ui\CHANGES.md and feel free to add yourself to CONTRIBUTORS.md

metaphore commented 6 years ago

@kotcrab good point, will do!

metaphore commented 6 years ago

It turned out that we don't need to pay attention to root group's X/Y because toast positions are relative to a parent and it's totally fine when root is located not in stage's zero or just moving.

kotcrab commented 6 years ago

That's great, merged PR.

jojorabbit commented 6 years ago

Hi, just a small comment. This does not seem the perfect solution. In case of adding toastManager after main root/actor to a stage, setFillParent will cover main root and events will not work on the main root since it is covered with widgetGroup from the ToastManager. The workaround is to add toastManager root to stage e.g. create toastManager first and add main root/actor to a stage but this would probably cause other problems e.g. toast root covered by main.

You can add widgetGroup.setTouchable(Touchable.childrenOnly); so events don't trigger on the toast widget group just on children.

Let me know what you think. Cheers.

metaphore commented 6 years ago

@jojorabbit oh you're right, I missed that one. Thanks!