Future GUI

[miniME89]

Sure, I agree and I will be more careful in the future.

Some other annoying conflicts will also appear if the "Fixing Checkstyle" pull request is merged in.

On the other hand we just need the classes MenuDefinitionUnserializer.groovy, UIElementVisualManager.groovy and MenuDefinitionLoader.java of your branch, which shouldn't be a problem to resolve the import/renaming stuff. And two additional changes in the UIWindow class.

Or just add the 3 additional classes in a first step and drop your other changes, which isn't that much. This could than be implemented again in a second step. We don't need it fully functional yet.

 

[x3ro]

Sure, I could just reconstruct what I did and just add it as a single commit on top of develop, but I'd really like to preserve my commit history because it turns out commit history can be really useful.

 

[Cervator]

IMHO I too enjoy detailed history including more commits if it makes sense

To cut down on conflicts I might be backing down on the Checkstyle stuff for a bit, going back to just fixing warnings if functional changes are being made to an area only. I hadn't quite fully considered how many conflicts it could cause in active branches, in particular local ones that don't show up on GitHub yet.

We certainly need to be careful with renames/moves, in particular if it is an area of active collaboration

 

[x3ro]

So I've been working on this some more, and I'm currently trying to "re-build" the main menu using a JSON definition. This is what I've got so far:

By using the following menu definition:

{
    "id": "main-menu",
    "controller": "UIMainMenu",
 
    "children": [
        {
            "id": "version-number",
            "component": "UIText",
            "style": {
                "text": "Pre Alpha",
                "position": [0, 230],
                "halign": "center",
                "valign": "top"
            }
        },
 
        {
            "id": "terasology-logo",
            "component": "UIImage",
            "style": {
                "backgroundImage": "engine:terasology",
                "size": [512, 128],
                "position": [0, 128],
                "halign": "center"
            }
        }
    ],
 
    "style": {
        "backgroundImage": "engine:menubackground"
    }
}

You may check out the code over on the GitHub and provide me with some feedback if you like

miniME89: There are currently some "problems" with the alignment setters (and probably other properties I haven't gotten to yet ), please read the comment I've linked to and tell me what you think

 

[miniME89]

Yea i saw that problem coming

But actually i don't think we should only implement methods in our widgets which take string/int/float/bool values as parameters, just because we can easily instance them from the JSON parser. That's why i haven't implemented a method for alignment, which takes a string as a parameter, in the first place.

I don't think that we should adjust the interfaces of the widgets themselves, just because the parser needs this. The interface of the widgets should be neat and logical. A method for alignment which takes a string as a parameter would be kinda wrong, in my opinion. So that's why it would be part of the parser to find a way to instance different objects and pass them to the widget interface as parameter.

I see the parser, to parse JSON files and instancing the UI elements, as an addon and it shouldn't really affect the interfaces of the widgets.

Since its not only the alignment methods which have this problem. I still can't see for example how the instancing of a Layout object, for the UIComposite widget would work. We need to find a way to instancing the right object and assign specific values (somehow).


By the way:
Still working on a multi line input widget, which in my first estimation would take 1 workday, but actually its not (hopefully it will be finished tomorrow)

 

[x3ro]

I see your point, however doing all the conversion in the unserializer would entail an ass-load (sorry) of "problems". The problem is that the JSON file has no clue about types. The only thing it knows are Strings, Numbers, Arrays and Maps, nothing else. Because of this, if we'd want to put the type conversion in the unserializer, we'd need to provide it with type information of _every single attribute_ there is, either through the JSON definition itself (which would be horribly from a "usability" perspective or in the unserializer itself.

While the latter would be feasible perhaps, I strongly discourage that solution, because it breaks a very important principle: Single point of control. The UIElement implementation, nothing else, should know about all its properties and how they need to be handled. If we split that information up into two separate parts we end up with a "oh I changed something here, lets remember where else I need to change stuff" kind of solution, and I really want to prevent that. In the current state, the unserializer is pretty generic, with about 100 lines of code total for the entire thing. If we'd but all the conversion/type information in there as well, it could easily grow to 4 times as much or more

So IMHO we should put those methods in the elements, because it has far better maintainability and the UIElements shouldn't be touched by hand anyway after the JSON system is in place.

 

[Immortius]

I have to side with miniME89. Having methods that consume strings when they really are dealing with enums is an ugly design, and it isn't the responsibility (as per the Single Responsibility Principle) of the UIElements to provide what is essentially persistence methods.

Once again I'll point out that GSON is designed to solve the problem you are describing. You can give it a JSON Element and the class to convert it to and it will handle the mapping of the primitive JSON types to the needed types for the properties the class contains - without requiring any dependency on the specific properties. In particular this includes enums (case-sensitive, although it is simple to add support for case-insensitive conversion). Try it, you might like it! This removes property-specific code, and leaves you to worry about the types that need to be handled and the overall structure of the JSON.

Another thing you may consider is using the entity/component system for UI as well, as that already has JSON loading sorted with support for a wide variety of types, and can be extended to support new types pretty easily.

 

[x3ro]

Immortius: Groovy does just that by default, i.e. assigning a string to a property that takes an enum and automatically converting, but that is case-sensitive as well. How do you propose making it case insensitive with GSON?

 

[Immortius]

It's actually the code example in the GSON documentation for TypeAdapterFactorys. In Terasology there is an implementation of it in the BlockLoader class called CaseInsensitiveEnumTypeAdapterFactory - feel free to move this into some flavour of GSON util package and reuse it (note to self - use less inner classes).

Basically the type adapter factories allow overriding GSON's handling of families of types (for single types a TypeAdapter will do) - so in this case you create a factory that, when GSON encounters an enum type, will produce a type adapter that uses the lowercase of the JSON string/enum for the purpose of mapping. To use it you register it with a GsonBuilder before producing the Gson object itself.

 

[pencilcheck]

Is there any significance to make GUI management different from entity framework?
I think entity framework is fantastic, where you can have some singleton managers, and components, system to handle different parts of system. Why can't we just apply this model and slap it on GUI?
For example, make GUIManager the singleton for adding,removing windows. And then components and system to handle events and take actions for particular UI components? Then perhaps use XML for specifying texture such as positions, dimension used. And devs could just put all UI into a UI folder with its texture components. What do you guys think?

It's also ridiculous to have two similar classes (UIScreenContainer, UIScreenInventory) that has similar variables as well (toolbar, inventory, etc)