mabe02 / lanterna

Java library for creating text-based GUIs
GNU Lesser General Public License v3.0
2.29k stars 243 forks source link

Components added after calling removeAllComponents become non-interactive #360

Open xazap opened 6 years ago

xazap commented 6 years ago

Hi,

I ran into an issue where I am unable to get focus or keyboard navigation when adding buttons to a Panel after calling removeAllComponents on that same Panel. The code below reproduces this with Lanterna 3.0.0.

The use-case for this is where my (MVC) controller updates a view implemented with Lanterna. I remove all components from a panel and then initialize all components again with updated model values.

Experimenting a bit, I found that calling setFocusedInteractable on the Window that contains the panel sets focus. This is a very expensive workaround as it would require me to keep track of which interactable component had focus before calling removeAllComponents. The component having focus is usually some other panel that may even be in some other window.

I observed this: When I add buttons to a panel the first time, the first button automatically gets focus. When you clear all components and add another set of buttons, none of them get focus. Even worse: interactable components on other panels also have no focus, resulting in a stuck GUI. Is this a bug?

import com.googlecode.lanterna.*;
import com.googlecode.lanterna.graphics.*;
import com.googlecode.lanterna.gui2.*;
import com.googlecode.lanterna.screen.*;
import com.googlecode.lanterna.terminal.*;

public class Reproduction {

    public static void main(String[] args) {
        try {
            Terminal terminal = new DefaultTerminalFactory().createTerminal();
            Screen screen = new TerminalScreen(terminal);
            screen.startScreen();

            WindowManager windowManager = new DefaultWindowManager(TerminalSize.ONE);
            WindowPostRenderer renderer = new WindowPostRenderer() {
                @Override
                public void postRender(ThemedTextGraphics textGraphics, TextGUI textGUI, Window window) {
                    System.out.println("postRender");
                }
            };

            MultiWindowTextGUI gui = new MultiWindowTextGUI(new SeparateTextGUIThread.Factory(), screen, windowManager, renderer, new EmptySpace(TextColor.ANSI.MAGENTA));
            ((AsynchronousTextGUIThread) gui.getGUIThread()).start();

            Panel panel = new Panel(new GridLayout(2));
            Button button1 = new Button("Button 1");
            panel.addComponent(button1);

            Window window = new BasicWindow();
            window.setComponent(panel);
            gui.addWindow(window);

            Thread.sleep(3000); // wait a bit so we can see it losing focus...
            panel.removeAllComponents();
            Button button2 = new Button("Button 2");
            panel.addComponent(button2);

            // workaround:
            // window.setFocusedInteractable(button2);
        } catch (Exception e) {
            e.printStackTrace();
        }
    }
}
avl42 commented 6 years ago

My observation is that for an Interactable to get initially focused it must already have been added to the structure of (maybe nested) Panels before the main Panel is set as the Component for the Window. Later adding of further components will not automatically set focus.

I have rearranged and modified your code:

package com.googlecode.lanterna.issue;

import com.googlecode.lanterna.gui2.*;
import com.googlecode.lanterna.screen.*;
import com.googlecode.lanterna.terminal.*;

public class Issue360 {

    public static void main(String[] args) {
        try {
            Terminal terminal = new DefaultTerminalFactory().createTerminal();
            Screen screen = new TerminalScreen(terminal);
            screen.startScreen();
            MultiWindowTextGUI gui = new MultiWindowTextGUI(screen);
            Window window = new BasicWindow();

            Button button1 = new Button("Button 1");
            Panel panel = new Panel(new GridLayout(2));

            panel.addComponent(button1);
            window.setComponent(panel);
            gui.addWindow(window);

            gui.updateScreen(); // shows focused Button 1
            Thread.sleep(1000);

            panel.removeComponent(button1);

            gui.updateScreen(); // shows empty Window
            Thread.sleep(1000);

            Button button2 = new Button("Button 2");
            panel.addComponent(button2);

            gui.updateScreen(); // shows non-focused Button 2
            Thread.sleep(1000);

            // mainly for demonstration purpose:
            window.setComponent(null); // don't let a draw() happen while null ...
            window.setComponent(panel);

            // alternatively, you can ask the Panel for any focusable child components:
            //window.setFocusedInteractable(panel.nextFocus(null));
            // (that's what window.setComponent() does, when called with a  Container like a Panel.)

            gui.updateScreen(); // now shows focused Button 2
            Thread.sleep(1000);

            System.exit(0);
        } catch (Exception e) {
            e.printStackTrace();
        }
    }
}
mabe02 commented 6 years ago

When you assign a component to the window, it will find the first interactable component and focus on it. But if the component is a panel like above, when you remove and add components to it, we can't get the window to automatically adjust the focus (not easily at least) because it doesn't know what you are doing with the composite component's content.

You could solve it by calling window.setFocusedInteractable(panel.nextFocus(null)); which will (re-)focus the first interactable in the panel.

xazap commented 6 years ago

Thanks for pointing out window.setFocusedInteractable(panel.nextFocus(null));. I have now implemented this in my project. I still get a stuck GUI sometimes though. Digging some more I found that there are actually two causes for my frozen GUI: Not having focus on an interactable component is one cause, which is now solved. Another cause is that the call to removeAllComponents(); doesn't always complete. Looking at the code for the Panel class, I see that it synchronizes internally on "components":

    public Panel removeAllComponents() {
        synchronized(components) {
            for(Component component : new ArrayList<Component>(components)) {
                removeComponent(component);
            }
        }
        return this;
    }

When I call removeAllComponents from my Controller thread it often waits (forever?) on getting the lock for "components". By suspending all threads before the synchronized block, I have found that a thread named "LaternaGUI" own the lock on "components" and is apparently not releasing it. This happens at a 50% ratio so it's very likely a thread-contention issue.

What puzzles me though, it seems to wait forever on release of the lock. Worst case I would expect a stalled completion when the LanternaGUI thread does its thing with the components and then releases its lock allowing my Controller thread to do its thing.

Any ideas on this?

avl42 commented 6 years ago

I find those locks a bit suspicious, too. Most likely you're sometimes facing a dining philosophers problem some thread holds one lock and tries to aquire another lock, and a second thread holds the second lock and tries to aquire the first one. java can give some information if you can send it signal 3 (on unix) or maybe with some tools in $JDK_HOME/bin/... it would be helpful if you could find out which thread is holding which locks (aka monitors) and waiting for which other monitor. if these threads are within lanterna code, then a stackdump would also help...

xazap notifications@github.com schrieb am So., 20. Mai 2018, 17:43:

Thanks for pointing out window.setFocusedInteractable(panel.nextFocus(null));. I have now implemented this in my project. I still get a stuck GUI sometimes though. Digging some more I found that there are actually two causes for my frozen GUI: Not having focus on an interactable component is one cause, which is now solved. Another cause is that the call to removeAllComponents(); doesn't always complete. Looking at the code for the Panel class, I see that it synchronizes internally on "components":

public Panel removeAllComponents() {
    synchronized(components) {
        for(Component component : new ArrayList<Component>(components)) {
            removeComponent(component);
        }
    }
    return this;
}

When I call removeAllComponents from my Controller thread it often waits (forever?) on getting the lock for "components". By suspending all threads before the synchronized block, I have found that a thread named "LaternaGUI" own the locks on "components" and is apparently not releasing it. This happens at a 50% ratio so it's very likely a thread-contention issue.

What puzzles me though, it seems to wait forever on release of the lock. Worst case I would expect a stalled completion when the LanternaGUI does its thing with the components and then releases its lock allowing my Controller thread to do its thing.

Any ideas on this?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/mabe02/lanterna/issues/360#issuecomment-390491396, or mute the thread https://github.com/notifications/unsubscribe-auth/AFAwsve8BQB3T-KOGm_u4FGp--l-b16vks5t0Y82gaJpZM4UFVCa .

xazap commented 6 years ago

Ok, I made stacktraces along with lock/monitor information. Quick summary:

Proof below:

Dump for thread "LanternaGUI":

Thread [LanternaGUI] (Suspended)    
    owns: ArrayList<E>  (id=95) 
    owns: Panel  (id=96)    
    owns: ArrayList<E>  (id=93) 
        waited by: Thread [Controller-1] (Suspended)    
            owns: ArrayList<E>  (id=94) 
                waited by: Thread [LanternaGUI] (Suspended) 
    owns: Panel  (id=97)    
    owns: AbstractBasePane$ContentHolder  (id=98)   
    owns: MultiWindowTextGUI  (id=99)   
    owns: SeparateTextGUIThread  (id=100)   
    waiting for: ArrayList<E>  (id=94)  
        owned by: Thread [Controller-1] (Suspended) 
            waiting for: ArrayList<E>  (id=93)  
                owned by: Thread [LanternaGUI] (Suspended)  
    HandComponent(Panel).isInvalid() line: 222  
    HandComponent(Panel).calculatePreferredSize() line: 214 
    HandComponent(AbstractComponent<T>).getPreferredSize() line: 178    
    LinearLayout.doVerticalLayout(TerminalSize, List<Component>) line: 186  
    LinearLayout.doLayout(TerminalSize, List<Component>) line: 163  
    Panel.layout(TerminalSize) line: 350    
    Panel.access$4(Panel, TerminalSize) line: 348   
    Panel$1.drawComponent(TextGUIGraphics, Panel) line: 195 
    Panel$1.drawComponent(TextGUIGraphics, Component) line: 1   
    Panel(AbstractComponent<T>).draw(TextGUIGraphics) line: 218 
    Panel$1.drawComponent(TextGUIGraphics, Panel) line: 205 
    Panel$1.drawComponent(TextGUIGraphics, Component) line: 1   
    Panel(AbstractComponent<T>).draw(TextGUIGraphics) line: 218 
    AbstractBasePane$ContentHolder$1.drawComponent(TextGUIGraphics, Container) line: 340    
    AbstractBasePane$ContentHolder$1.drawComponent(TextGUIGraphics, Component) line: 1  
    AbstractBasePane$ContentHolder(AbstractComponent<T>).draw(TextGUIGraphics) line: 218    
    PlayerWindow(AbstractBasePane<T>).draw(TextGUIGraphics) line: 73    
    PlayerWindow(AbstractWindow).draw(TextGUIGraphics) line: 128    
    MultiWindowTextGUI.drawGUI(TextGUIGraphics) line: 261   
    MultiWindowTextGUI(AbstractTextGUI).updateScreen() line: 120    
    MultiWindowTextGUI.updateScreen() line: 223 
    SeparateTextGUIThread(AbstractTextGUIThread).processEventsAndUpdate() line: 83  
    SeparateTextGUIThread.mainGUILoop() line: 114   
    SeparateTextGUIThread.access$1(SeparateTextGUIThread) line: 100 
    SeparateTextGUIThread$1.run() line: 55

From the stacktrace we see that thread LanternaGUI is waiting to acquire a lock on components in Panel::isInvalid line 222:

    @Override
    public boolean isInvalid() {
        synchronized(components) { // This is line 222
            for(Component component: components) {
                if(component.isInvalid()) {
                    return true;
                }
            }
        }
        return super.isInvalid() || layoutManager.hasChanged();
    }

Dump for thread "Controller-1":

Thread [Controller-1] (Suspended)   
    owns: ArrayList<E>  (id=94) 
        waited by: Thread [LanternaGUI] (Suspended) 
    waiting for: ArrayList<E>  (id=93)  
        owned by: Thread [LanternaGUI] (Suspended)  
            waiting for: ArrayList<E>  (id=94)  
                owned by: Thread [Controller-1] (Suspended) 
    Panel.invalidate() line: 340    
    AbstractBasePane$ContentHolder(AbstractComposite<T>).invalidate() line: 110 
    PlayerWindow(AbstractBasePane<T>).invalidate() line: 66 
    PlayerWindow(AbstractBasePane<T>).setFocusedInteractable(Interactable, Interactable$FocusChangeDirection) line: 252 
    PlayerWindow(AbstractBasePane<T>).setFocusedInteractable(Interactable) line: 232    
    HandComponent(Panel).removeComponent(Component) line: 120   
    HandComponent(Panel).removeAllComponents() line: 136    
    HandComponent(CardPanel).updateView(List<Card>, Locale) line: 229   
    HandComponent.updateView(PlayerGameState) line: 48  
    PlayerWindow.onNext(ModelUpdateEvent) line: 105 
    PlayerWindow.onNext(Object) line: 1 
    SubmissionPublisher$BufferedSubscription<T>.consumeNext(Subscriber<? super T>, Object) line: 1305   
    SubmissionPublisher$BufferedSubscription<T>.takeItems(Subscriber<? super T>, long, int) line: 1294  
    SubmissionPublisher$BufferedSubscription<T>.consume() line: 1251    
    SubmissionPublisher$ConsumerTask<T>.run() line: 929 
    ThreadPoolExecutor.runWorker(ThreadPoolExecutor$Worker) line: 1135  
    ThreadPoolExecutor$Worker.run() line: 635   
    Thread.run() line: 844

From the stacktrace we see that thread "Controller 1" is waiting to acquire a lock on components in Panel::invalidate line 340:

    @Override
    public void invalidate() {
        super.invalidate();

        synchronized(components) { // This is line 340
            //Propagate
            for(Component component: components) {
                component.invalidate();
            }
        }
    }
avl42 commented 6 years ago

Well, that's bad. I don't yet see an easy fix, but some approaches maybe towards a fix (all targetting lanterna, not your user code):

From your user code, however you could consider doing your GUI updates using invokeLater().

mabe02 commented 6 years ago

Nice one! While we come up with a fix for this, you could do a call to invokeAndWait(..) on the GUI thread object and perform your state mutations there. That will avoid the issue you're seeing now.

xazap commented 6 years ago

For the benefit of anyone else running into the same issues as I did, the code below fixed both the issue of the components becoming inaccessible and the deadlock issue:

getTextGUI().getGUIThread().invokeLater(() -> {
    removeAllComponents();
    initComponents(model, locale); // This is a non-Lanterna call which recreates the removed components with new model values
    setFocusedInteractable(contentPane.nextFocus(null));
});

Note that the above assumes that the LanternaGUI was started in its own thread:

gui = new MultiWindowTextGUI(new SeparateTextGUIThread.Factory(), screen);
((AsynchronousTextGUIThread) gui.getGUIThread()).start();

Thanks for helping out @avl42 and @mabe02!