Lanterna 3 AbstractListBox

[Vyacheslav Blinov]

I can see AbstractListBox as a gui element which shows some sort of the list of elements. In current implementation the list to be displayed is held by AbstractListBox privately and can only be manipulated using it's modifying methods. I can addItems to it, but my model of data is such that sometimes some items can be replaced by another ones, or just be removed. Unfortunately currently only addItem method exists, but no updateItem/removeItem and so on.

I feel inconvenience with current AbstractListBox it in that it is:

- feels a bit too complex as it maintains both graphical aspect and the list of data itself

- manipulated data as a list of Objects limiting your data to something implementing toString in a special way

I volunteer for implementing separation of concepts here, and propose to factor out data model from it. It will allow to make data container more type-safe, more flexible and adapt any data it has to the strings shown on screen. I already had experience doing such ListBox & ListModel for Lanterna 2, and it was a pleasure to use. https://gist.github.com/dant3/4a4249a886e4ec5625e6 - this is the simplest version I came with, as a beginning point. Of course it should be ported to Java.

WDYT?

[Vyacheslav Blinov]

Seeing that addItem is synchronised I imagined it will be thread-safe to add items from different thread, but I got exception soon enough.

java.util.ConcurrentModificationException
at java.util.ArrayList$Itr.checkForComodification(ArrayList.java:901)
at java.util.ArrayList$Itr.next(ArrayList.java:851)
at com.googlecode.lanterna.gui2.AbstractListBox$DefaultListBoxRenderer.getPreferredSize(AbstractListBox.java:245)
at com.googlecode.lanterna.gui2.AbstractListBox$DefaultListBoxRenderer.getPreferredSize(AbstractListBox.java:209)
at com.googlecode.lanterna.gui2.AbstractComponent.calculatePreferredSize(AbstractComponent.java:128)
at com.googlecode.lanterna.gui2.AbstractComponent.getPreferredSize(AbstractComponent.java:117)
at com.googlecode.lanterna.gui2.LinearLayout.getPreferredSizeVertically(LinearLayout.java:87)
at com.googlecode.lanterna.gui2.LinearLayout.getPreferredSize(LinearLayout.java:76)
at com.googlecode.lanterna.gui2.Panel$1.getPreferredSize(Panel.java:110)
at com.googlecode.lanterna.gui2.Panel$1.getPreferredSize(Panel.java:106)
at com.googlecode.lanterna.gui2.AbstractComponent.calculatePreferredSize(AbstractComponent.java:128)
at com.googlecode.lanterna.gui2.Panel.calculatePreferredSize(Panel.java:132)
at com.googlecode.lanterna.gui2.AbstractComponent.getPreferredSize(AbstractComponent.java:117)
at com.googlecode.lanterna.gui2.AbstractBasePane$ContentHolder$1.getPreferredSize(AbstractBasePane.java:238)
at com.googlecode.lanterna.gui2.AbstractBasePane$ContentHolder$1.getPreferredSize(AbstractBasePane.java:231)
at com.googlecode.lanterna.gui2.AbstractComponent.calculatePreferredSize(AbstractComponent.java:128)
at com.googlecode.lanterna.gui2.AbstractComponent.getPreferredSize(AbstractComponent.java:117)
at com.googlecode.lanterna.gui2.AbstractWindow.getPreferredSize(AbstractWindow.java:114)
at com.googlecode.lanterna.gui2.MultiWindowTextGUI.updateScreen(MultiWindowTextGUI.java:141)
at com.googlecode.lanterna.gui2.AbstractTextGUIThread.processEventsAndUpdate(AbstractTextGUIThread.java:61)
at com.googlecode.lanterna.gui2.MultiWindowTextGUI.waitForWindowToClose(MultiWindowTextGUI.java:275)
at com.googlecode.lanterna.gui2.AbstractWindow.waitUntilClosed(AbstractWindow.java:164)
at com.googlecode.lanterna.gui2.MultiWindowTextGUI.addWindowAndWait(MultiWindowTextGUI.java:252)
at com.github.dant3.catlog.Main$$anonfun$main$1.apply(Main.scala:9)
at com.github.dant3.catlog.Main$$anonfun$main$1.apply(Main.scala:7)
at com.github.dant3.catlog.GUI$.run(GUI.scala:21)
at com.github.dant3.catlog.Main$.main(Main.scala:7)
at com.github.dant3.catlog.Main.main(Main.scala)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.lang.reflect.Method.invoke(Method.java:497)

I suppose I should always add items to display in gui component on gui thread, but then synchronised appears to be unneeded overhead I can’t get rid of currently.

> --
> Det här meddelandet skickas till dig eftersom du prenumererar på gruppen "lanterna-discuss" i Google Grupper.
> Om du vill sluta prenumerera på den här gruppen och inte längre få någon e-post från den skickar du ett e-postmeddelande till lanterna-discu...@googlegroups.com.
> Fler alternativ finns på https://groups.google.com/d/optout.

Regards,
Vyacheslav Blinov
https://github.com/dant3

[AvL42]

The problem seems to be, that two methods of AbstractListbox getPreferredSize() and drawComponent()  both iterate the list of items without having obtained it using the already available getItemsIterable().

It would probably be best to change the calls to getItems() to  getItemsIterable(), but I fear there might be further concurrency problems waiting to show up...

Concurrency unfortunately isn't my strong side, either. Will your announced model/view separation for Listbox (once ported from Scala to Java) be thread-safe?

[Vyacheslav Blinov]

It seems to be just a single problem, but I think there would be lots of more. AbstractListbox has data implementation at hand and thus error prone around data access, makes two tasks at the same time. I can’t say that just refactoring it to using model/view pattern will solve concurrency issues, but I can for sure say that it will make lots easier to make list data access more thread-safe, thus Listbox will only have to deal with it’s own concurrency issues.

Thread safety of Lanterna seems to be far bigger topic then just AbstractListbox discussion.

Java Concurrency in Practice says that there are no successful thread-safe/multithreaded gui frameworks. In fact if you’ll look around at gui frameworks that are most popular in the industry you’ll find things like Android framework, JavaFX, Swing, SWT, Qt and so on - all not being thread-safe and single-thread-centric. This happens because thread-safety makes development a lot more complex and gui are complex on their own already. Having synchronisation bits in it makes it slower in redrawing stuff, dead-lock danger then extending and all lots of related problems. By just serialising all the gui access to a single thread and removing all synchronisation frameworks gains lots of drawing speed, responsiveness and simplicity.

[Vyacheslav Blinov]

I’ve made early version of model changes here: https://github.com/dant3/lanterna/commit/cf9a70793d466c22a58d7746a63416b9b6b434a9

All ListBox tests are passing currently, and now my plan was to work on a better implementation of basic data model. In particular I’m ready to pay attention to concurrency/ui state problems as well as play a bit with extendability.

Any feedback is welcome.

[Martin Berglund]

The main reason for not separating data and view in the listbox hierarchy, and elsewhere in the library too, was just for simplicity. I didn't want to make it more complicated than necessary.

One thought is, what's wrong with just adding extra synchronization? If the concurrency exceptions are happening because of data model manipulations while the GUI thread is drawing, this should be solvable through more synchronization, no? Adding extra methods for removing and replacing items in the list box should be easy enough.

Martin

Det här meddelandet skickas till dig eftersom du prenumererar på gruppen lanterna-discuss i Google Groups.

[Vyacheslav Blinov]

My proposal to separate those was to make a good abstraction layer that will give additional flexibility in both how your data gets passed to view, how it’s thread-safety guarded and in how this data is rendered on screen. As time goes list views and list models tend to become more complex, and keeping them together makes things even more complex, as I see it.

Adding more synchronisation doesn’t sounds like a nice idea to me in long term.
Did you ever had this sad waiting of button which just stucked in pressed state awaiting for other thread to release lock? In UI development making two rendering loops always feels better then making one, since your ui reflects user interaction faster being more responsive. Not only synchronisation makes ui rendering thread slower (which is not so much noticeable in terminal frame since your ui is not 1024x768 points but ~100x40 chars, so less work to do in rendering loop), it also makes you face those weird situations then data mutates while you render it, making all fancy variants of problems happening as wrong lines count, bloated strings, partially-updated content drawn and so on and so forth. Where are safe methods of how you can really deal with those problems, but they all are based on data copying and thread-based serialisation of delivery of those copied updates. While this can be implemented inside of ListBox as well, I think it will became too complex to be easily maintained.

I think the best way to preserve simplicity here is to implement some SimpleListBox component which will have some simple model hidden inside of it, exposing all sorts of mutation methods of it, somewhat like it does today. As example you can refer to implementation of CheckBoxList. It now has “checkability” implementation inside of it’s model (on top of any other user’s model), so if you want some other “checkable” list implementation you can just reuse it’s model, but create custom view (i.e. grid-based). Another example would be ActionListBox which completely hides model implementation, allowing you to interact with it like before.

[Martin Berglund]

Yes, you are not wrong. Again, I'm concerned about how much more difficult, or maybe non-straight-forward is the right word, the API will be if we extract out all data containers and make model classes. That being said, I looked through the code you linked to above and I think it generally looks good. I'll try to merge it into a separate branch and have a look at it again.

Martin