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

Generic UI Framework - basic implementation #56126

Closed
wants to merge 1 commit into from

Conversation

mqrause
Copy link
Contributor

@mqrause mqrause commented Mar 16, 2022

Summary

Infrastructure "Generic UI Framework - basic implementation"

Purpose of change

Allow easier UI development by providing some default classes.
Mostly done to further #55503, so it only contains things I need for that.
Also see #56026 for a previous version and discussion.

Describe the solution

ui_element

  • an interface for drawable classes

absolute_layout

  • basic layout that draws ui_elements at the given locations

text_block

  • a single line text element that trims the text to fit inside the available width and interprets color tags in the given text

list_view

  • a complex element to display a list of optionally selectable elements
  • how the elements are displayed is handled by a template function that returns a ui_element

todos:

  • sort out how the list_view gets the category of its elements (probably by turning the template function into a class and handling it there)

Describe alternatives you've considered

I'm not fully convinced by the rectangles for locations. I might instead go back to a point and two ints for the start and width/height, but that means more parameter juggling. Using rectangle is kind of unintuitive, though, because it's going by terminal cells and not by pixels. So a rectangle with a height of zero has one line.

Testing

It's used in #55503
See these 200 lines: https://github.com/CleverRaven/Cataclysm-DDA/pull/55503/files#diff-323b6d7ca56f6c7fb3d8f3fd71b66870cd60b31f6f9aa39e06e6583304ae69b5R290-R490

Additional context

This is slighly beyond my actual C++ abilities, so it might contain stupid design choices. And the constant scope creep is driving me insane.

@github-actions github-actions bot added [C++] Changes (can be) made in C++. Previously named `Code` json-styled JSON lint passed, label assigned by github actions labels Mar 16, 2022
@mqrause mqrause marked this pull request as draft March 16, 2022 01:41
@Maleclypse Maleclypse added Info / User Interface Game - player communication, menus, etc. Controls / Input Keyboard, mouse, keybindings, input UI, etc. labels Mar 16, 2022
@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Mar 16, 2022
@andrei8l
Copy link
Contributor

With the new absolute_layout you're taking the code back to the days before ui_adaptor by unconditionally redrawing elements even if they're obscured behind windows, etc. IMO ui_adaptor is the best thing that happened for cdda UI and you shouldn't throw it away...

If I understand your goal correctly, you could implement absolute_layout as a wrapper that calls resize()/reposition() on its children in ui_adaptor's on_screen_resize() callback.

@Qrox
Copy link
Contributor

Qrox commented Mar 16, 2022

@andrei8l Currently, ui_adaptor is used on a per-UI basis, that is, the subcomponents of a UI are unconditionally redrawn unless the whole UI is obscured. This is a design choice based on the status of the CDDA codebase where many UIs use ad-hoc implementations of subcomponents which are hard for obscuration calculation. The ui_adaptor system can of course be extended to UI subcomponents but that's a lot of further work, and the ui_adaptor system itself may need to be upgraded (for example, currently you can't adjust the z-order of the ui_adaptors, which are redrawn in the order they are created). #55503 still uses ui_adaptor for the V menu so I think it's good enough for the time being.

@mqrause My concern though is that even if someone creates generic UI subcomponents, other contributors are often disinclined to use them simply because they are unaware of the new subcomponent; and unless the whole codebase is migrated to the new subcomponents, they tend to copy and use the legacy code simply because more UIs use the legacy code than the new code.

@andrei8l
Copy link
Contributor

andrei8l commented Mar 16, 2022

The ui_adaptor system can of course be extended to UI subcomponents but that's a lot of further work

You can just have one ui_adaptor for each subcomponent that handles the drawing (on_redraw() callback) but leave the resizing to an upper level (such as this absolute_layout). I've already done something like this for the trade_ui or the failed AIM rework so I know it's doable easily.

But if we're breaking down the UI to subcomponents that are just individual lines of text, I'm not sure the effort is worthwhile.

#55503 still uses ui_adaptor for the V menu so I think it's good enough for the time being.

I see that it defines an ui_adaptor then never uses it. What am I missing?

@mqrause
Copy link
Contributor Author

mqrause commented Mar 16, 2022

With the new absolute_layout you're taking the code back to the days before ui_adaptor by unconditionally redrawing elements even if they're obscured behind windows, etc. IMO ui_adaptor is the best thing that happened for cdda UI and you shouldn't throw it away...

If I understand your goal correctly, you could implement absolute_layout as a wrapper that calls resize()/reposition() on its children in ui_adaptor's on_screen_resize() callback.

No, the way you would use it currently is to have one top level element, most likely an absolute_layout, and call it's draw method within ui_adaptor::on_redraw. In that way it does nothing different than what's currently there. I didn't completely rework #55503 to use this, but here https://github.com/CleverRaven/Cataclysm-DDA/pull/55503/files#diff-323b6d7ca56f6c7fb3d8f3fd71b66870cd60b31f6f9aa39e06e6583304ae69b5R1030-R1050 you have the list.draw because currently the list_view in there is the only thing that uses this framework, so that's the top level element.

Ultimately that should move inside the framework, too, but that's a thing for future improvements, not for a first pass.

@mqrause My concern though is that even if someone creates generic UI subcomponents, other contributors are often disinclined to use them simply because they are unaware of the new subcomponent; and unless the whole codebase is migrated to the new subcomponents, they tend to copy and use the legacy code simply because more UIs use the legacy code than the new code.

Yes, that is generally an issue I don't have a good solution for. I just copied what inventory_ui did for #55503 myself. Adding documentation is an obvious step to address it as is updating current UIs to use the new code.

But if we're breaking down the UI to subcomponents that are just individual lines of text, I'm not sure the effort is worthwhile.

That is not the point of this. You can create any kind of subcomponent you want. Individual lines of text was just the first thing I needed.

@Qrox
Copy link
Contributor

Qrox commented Mar 16, 2022

I see that it defines an ui_adaptor then never uses it. What am I missing?

It's used in src/surroundings_menu.cpp. Maybe you didn't find it because the file is by default folded.

@KorGgenT
Copy link
Member

KorGgenT commented Mar 16, 2022

@mqrause My concern though is that even if someone creates generic UI subcomponents, other contributors are often disinclined to use them simply because they are unaware of the new subcomponent; and unless the whole codebase is migrated to the new subcomponents, they tend to copy and use the legacy code simply because more UIs use the legacy code than the new code.

This is definitely a problem we have and have had in this code base with things like item_location and generic_factory. The solution, as it were so far, is to just update as much legacy code as you can stomach and to make documentation easily understandable and available; and to keep an eye, at least for a little while, on new prs that implement up stuff so that they use the new system. I certainly would not use this concern as a reason not to implement something new, but as a word of caution that unfortunately the work isn't done just by writing this new framework.

@andrei8l
Copy link
Contributor

I don't think I'm getting my points across very well so let me try some code instead:

andrei8l@afe221a this is how I think this absolute_layout should work. It only handles resizing/repositioning and leverages ui_adaptor to let children draw themselves when they are not obscured.
This is still grossly incomplete as a "generic UI framework" and I don't think we should be pushing it just yet.

andrei8l@5d34b54 this is a wrapper over uilist that serves the same purpose as your list_view<>, but without reinventing the wheel. It includes a callback forwarder so there is no loss of functionality.

andrei8l@648124c and this is a (very!) barebones and dirty re-implementation of game::list_items_monsters() using this new uilist_generic. It's only meant as a demo so it's missing a lot of functionality, but it can all be added fairly easily in (or around) the callback.

Let me know what you think.

@mqrause
Copy link
Contributor Author

mqrause commented Mar 18, 2022

andrei8l@afe221a this is how I think this absolute_layout should work. It only handles resizing/repositioning and leverages ui_adaptor to let children draw themselves when they are not obscured. This is still grossly incomplete as a "generic UI framework" and I don't think we should be pushing it just yet.

Yes, that is pretty much what I meant with "Ultimately that should move inside the framework". But keep in mind what Qrox said: "The ui_adaptor system can of course be extended to UI subcomponents but that's a lot of further work, and the ui_adaptor system itself may need to be upgraded (for example, currently you can't adjust the z-order of the ui_adaptors, which are redrawn in the order they are created)"

andrei8l@5d34b54 this is a wrapper over uilist that serves the same purpose as your list_view<>, but without reinventing the wheel. It includes a callback forwarder so there is no loss of functionality.

andrei8l@648124c and this is a (very!) barebones and dirty re-implementation of game::list_items_monsters() using this new uilist_generic. It's only meant as a demo so it's missing a lot of functionality, but it can all be added fairly easily in (or around) the callback.

I'll check it out over the weekend.

I already gave up on doing only as much as I need for the other PR btw. This needs to get all of the basic structure implemented at least. So a window with a layout with elements and all the drawing logic being internal to it. The point is that developers can just stitch together the basic elements without having to worry about any of the low level stuff. And there's no point in merging it before that's achieved.

@mqrause
Copy link
Contributor Author

mqrause commented Mar 20, 2022

@andrei8l What you did with the wrapper over uilist doesn't really fit too well into the concept (as I imagine it). The uilist class itself would need to be reworked/expanded. A simple stringify is not enough for more complex (possibly multiline) layouts of list entries. And all the drawing it does should be updated to use the framework itself. I'm really not confident enough to do that for such a widely used class, so I tend to using the new list_view and expanding it to do everything we need from uilist when replacing the uses of that. However the first pass of the framework doesn't necessarily have to contain a list-ui_element, so merging the framework and then updating uilist to use it and be a part of it is also an option, if you (or someone else) would be willing to take over that part.

@github-actions github-actions bot added the astyled astyled PR, label is assigned by github actions label Apr 1, 2022
@mqrause
Copy link
Contributor Author

mqrause commented Apr 1, 2022

Expanded it to roughly how I think it should be structured. Still heavily WIP, so any details can and will change. But the baseline is there, and as usual #55503 has a (currently very basic) demonstration of how to use it.

@stale
Copy link

stale bot commented Jun 12, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. Please do not 'bump' or comment on this issue unless you are actively working on it. Stale issues, and stale issues that are closed are still considered.

@stale stale bot added the stale Closed for lack of activity, but still valid. label Jun 12, 2022
@leemuar
Copy link
Contributor

leemuar commented Jun 25, 2022

I came from a ERP framework that uses a declarative UI. You describe what form contain and how it's elements should behave - and renderer does the rest based on this information. It hides lots of low level calculation and drawing from programmer.
It takes huge inspiration from Windows UDF

Every UI window is Form.
Form is a tree of different FormElements.
FormElements can be Button, TextInput, text label, Table, Picture, ElementGroup (can contain other elements), etc
Every Form and FormElement has properties, changing property changes the behavior of that element: MaxWidth, FitToWidth, TextColor, etc

Form (bg_color: red, text_color: white, fullscreen: true)
   Group
      TextInput (label: "first name")
      TextInput (label: "last name")
  Button (label:"Submit" OnClick:"OnButtonClick")

Generally Im thinking about this kind of UI: set of abstract elements and ability to create forms using them. Every window extends base class and defines it's default content in the constructor.

class MissionForm extends FormAbstract
  MissionForm() {
     AddElement( TextInput );
     AddElement( TextInput );
     AddElement( Button )
     Elements[Button].OnCLick = [](){ ... }
  }

Since it's declarative - it can be JSONized and changed on the fly. And since it is abstract - it can use any "driver" rendered: catacurses, IMGui, SDL, etc

@stale stale bot removed the stale Closed for lack of activity, but still valid. label Jun 25, 2022
@mqrause
Copy link
Contributor Author

mqrause commented Jun 25, 2022

Yes, that is more or less what I'm trying to do here.

@leemuar
Copy link
Contributor

leemuar commented Jun 26, 2022

@leemuar
Copy link
Contributor

leemuar commented Jun 26, 2022

Scrollbar class is doing something interesting, it uses declarative approach:

image

@stale
Copy link

stale bot commented Aug 13, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. Please do not 'bump' or comment on this issue unless you are actively working on it. Stale issues, and stale issues that are closed are still considered.

@stale stale bot added the stale Closed for lack of activity, but still valid. label Aug 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions [C++] Changes (can be) made in C++. Previously named `Code` Controls / Input Keyboard, mouse, keybindings, input UI, etc. Info / User Interface Game - player communication, menus, etc. json-styled JSON lint passed, label assigned by github actions stale Closed for lack of activity, but still valid.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants