Rewrite of the UI code
Moderator: Forum Moderators
Rewrite of the UI code
I'm in the process of rewriting some parts of the low-level user interface. Rationale: lots of code are currently duplicated through Wesnoth. For example, the "disappearing scroll buttons in the help system" problem can easily be fixed; but it shouldn't have to be fixed. Once the same bug was corrected for menus, the help system should have automagically benefited from the fix.
I won't commit the code at the moment. It is not 0.8.6 material (or 0.9, or whatever). It may not even be 1.0 material. However it will need to be done sooner or later. As a matter of fact, my current patch modifies around 500 lines of code, and removes as much! It is no small improvement. I may commit the code to a CVS branch once I'm quite satisfied with the current patch.
I'm creating this thread because some design decisions need to be taken. It will also serve as a pseudo-documentation in the meantime.
I'm working at two levels. First a rewrite of the event handling. In fact, it is not so much the event handling, than the removal/cleanup of all the "process" functions. For example, "menu::process" has eight parameters; but if you take a look you will see that none of them is actually useful. And the real "process" functions (the virtual inheritors of "handle::process") have been renamed "process_event" in order to avoid confusion with the fake ones. Rational for the name: these functions are called by "raise_process_event" and they match the "handle::handle_event" functions.
Now let's clarify what to do about "handle_event" and "process_event". The first one still receives an SDL_Event, so its behavior is quite clear: it handles mouse motion, keypress, and so on. I suggest the second one be used to deal with asynchronous events. That is: a widget should look at the status of another widget during a "process_event". For example, a scrollbar should look at the status of its scroll buttons during this time.
I think it may work quite well. However it will induce a lag when an information needs several call to "raise_process_event" to propagate along a widget chain. Maybe add a flag that would be set when a widget changes its state during "process_event", so that a second call to "raise_process_event" is invoked right after the first one so that any pending "process" are done. Thoughts?
I said I was working at two levels. Here is the second one: a rewrite of the widgets. The most important change is the scrollbar. It goes from a "rather stupid scrollbar widget" (doxygen documentation) to a full-featured one. Now the scrollbar owns the scroll buttons; it is able to spy on another widget for wheel scrolling events; and it completely hides the complexity of scrolling.
What I mean by the last point is: the scrollbar now manipulates abstract scroll positions. The owner of the scrollbar doesn't have to manipulate pixels distance anymore, it directly gets a meaningful result. For example, the loadgame menu will tell to its scrollbar that it has 20 entities, and it can display 5 of them at a time. Then the scrollbar will compute by itself the size of the grip, it doesn't have to be given anymore. It will also call the menu callback with a value between 0 and 15 whenever the user moves the scroll grip (with the wheel on the menu, or by clicking on the groove, or by dragging the grip, or by pressing the scroll buttons). It greatly simplifies the code.
However I have encountered a few problems with background restore in doing that. Before, when a scrollbar was only a groove, it was natural to restore the background once the scrollbar was dirty so that it could get redrawn upon a fresh background. Now that the scrollbar owns its scroll buttons, it doesn't work that well anymore. Once the user moves the grip, the scrollbar gets dirty, and the background is restored, destroying the scroll buttons; and since they were not dirty they are not restored.
A way to avoid this issue would be to forget about the default surface restorer of the widget class, and never calls "set_location" or "hide" so that it doesn't kick in. I don't think it is a good idea since it will once again lead to code duplication: each composite widget will need its own partial restorer so that it only restores the parts not owned by its child widgets.
A solution would be to change the background restorer so that it does not work on the whole widgets, but on a list of rectangles related to the widget. For example, a menu would have only one rectangle for the item lists; the scrollbar would restore the rest of the widget by itself. Moreover, the saved surfaces should be publicly accessible (or at least protected). Rationale: when a menu moves the selection, it does not want to rewrite the complete list, it only wants to restore and draw two items. Any thoughts on this?
I won't commit the code at the moment. It is not 0.8.6 material (or 0.9, or whatever). It may not even be 1.0 material. However it will need to be done sooner or later. As a matter of fact, my current patch modifies around 500 lines of code, and removes as much! It is no small improvement. I may commit the code to a CVS branch once I'm quite satisfied with the current patch.
I'm creating this thread because some design decisions need to be taken. It will also serve as a pseudo-documentation in the meantime.
I'm working at two levels. First a rewrite of the event handling. In fact, it is not so much the event handling, than the removal/cleanup of all the "process" functions. For example, "menu::process" has eight parameters; but if you take a look you will see that none of them is actually useful. And the real "process" functions (the virtual inheritors of "handle::process") have been renamed "process_event" in order to avoid confusion with the fake ones. Rational for the name: these functions are called by "raise_process_event" and they match the "handle::handle_event" functions.
Now let's clarify what to do about "handle_event" and "process_event". The first one still receives an SDL_Event, so its behavior is quite clear: it handles mouse motion, keypress, and so on. I suggest the second one be used to deal with asynchronous events. That is: a widget should look at the status of another widget during a "process_event". For example, a scrollbar should look at the status of its scroll buttons during this time.
I think it may work quite well. However it will induce a lag when an information needs several call to "raise_process_event" to propagate along a widget chain. Maybe add a flag that would be set when a widget changes its state during "process_event", so that a second call to "raise_process_event" is invoked right after the first one so that any pending "process" are done. Thoughts?
I said I was working at two levels. Here is the second one: a rewrite of the widgets. The most important change is the scrollbar. It goes from a "rather stupid scrollbar widget" (doxygen documentation) to a full-featured one. Now the scrollbar owns the scroll buttons; it is able to spy on another widget for wheel scrolling events; and it completely hides the complexity of scrolling.
What I mean by the last point is: the scrollbar now manipulates abstract scroll positions. The owner of the scrollbar doesn't have to manipulate pixels distance anymore, it directly gets a meaningful result. For example, the loadgame menu will tell to its scrollbar that it has 20 entities, and it can display 5 of them at a time. Then the scrollbar will compute by itself the size of the grip, it doesn't have to be given anymore. It will also call the menu callback with a value between 0 and 15 whenever the user moves the scroll grip (with the wheel on the menu, or by clicking on the groove, or by dragging the grip, or by pressing the scroll buttons). It greatly simplifies the code.
However I have encountered a few problems with background restore in doing that. Before, when a scrollbar was only a groove, it was natural to restore the background once the scrollbar was dirty so that it could get redrawn upon a fresh background. Now that the scrollbar owns its scroll buttons, it doesn't work that well anymore. Once the user moves the grip, the scrollbar gets dirty, and the background is restored, destroying the scroll buttons; and since they were not dirty they are not restored.
A way to avoid this issue would be to forget about the default surface restorer of the widget class, and never calls "set_location" or "hide" so that it doesn't kick in. I don't think it is a good idea since it will once again lead to code duplication: each composite widget will need its own partial restorer so that it only restores the parts not owned by its child widgets.
A solution would be to change the background restorer so that it does not work on the whole widgets, but on a list of rectangles related to the widget. For example, a menu would have only one rectangle for the item lists; the scrollbar would restore the rest of the widget by itself. Moreover, the saved surfaces should be publicly accessible (or at least protected). Rationale: when a menu moves the selection, it does not want to rewrite the complete list, it only wants to restore and draw two items. Any thoughts on this?