OpenTTD

1) please use "svn diff" or something equivalent that DOES NOT diff svn's metadata. I cannot really see the changes due to all the metadata diffs.
2) do not use tabs to align comments. Tabs will only be used up to the first non-tab character in a line. Spaces will only be used after tabs for aligning the * in a multiline /*n *n */ comment.
3) do not put multiple statements on a single line (except for for loops).
4) WinPtr and WinPtr_const are very misleading names.

Tnx for the quick response. VERY sorry about the svn meta data, that shouldn't have happened.

Removed all tabs for comment-alignment, moved all statements to their own line, Replaced WinPtr by WindowListItem, and WinPtr_{begin,end} by WindowListItem_{BOTTOM,TOP}.

Would it be possible to split the comments additions from the actual functional changes, because in a 100+ kilobyte patch with all kinds of scattered documentation additions it is hard to follow the real changes.

Furthermore large patches with lots of different changes are better presented in smaller chunks because that highly increases the chance to get the to trunk; none of the developers fancies to determine the changes in a 10-20+ kilobyte patch. Small and easy to understand changes go (much) quicker to trunk than huge patches. For me it is basically: can I understand what the complete patch does in a compile cycle (2-3 minutes), then the chance of inclusion is high. Otherwise it's pretty low, unless I am myself highly interested in the feature.

At a last note I must say that you remove an awfully lot of lines in the first diff, without readding them somewhere else.

Basically, in whatever form you like it, you can get it.
Attached you will find the doxygen changes, and a few code-refactorings
- moving declarations of variables to a better place
- moved some function doxygen stuff from .h to .cpp,
- replaced a for() loop with equivalent FOR_ALL_WINDOWS() macro,
- deleted unused struct SizeRect).
- The one line diff in station_gui.cpp is a space at the beginning of the line.

The reason for the large delete is that 'svn cp window.cpp window_list.cpp' copies about 65KB. Then in both files I throw out functions that I keep in the other file (to make the split between 'real' window functions (about 10K), and 'window-list' functions (about 55K)), so that's 65KB deleted lines without counting context.

first set code changes:
- Converted struct Window to class Window
- Added a base class BaseWindow, and move left/top/width/height, and viewport data members into it (and SVN is making a mess of it)
- Swapped 'void DrawOverlappedWindowForAll()' and 'static void DrawOverlappedWindow()' to get rid of the forward declaration of the latter
- memset() on objects crashes the program, so replaced/merged it with explicit initialization of all data members.
- removed memset() in InitWindowSystem too

What is the added benefit of the BaseWindow class besides not being able to memset the struct anymore?

In the current implementation, it is very difficult to do anything without the entire windowing system coming down on you. That makes fixes for eg resizable widgets as requested in  FS#1072 ,  FS#1781  difficult.
Also changes in setup of the WindowClassDesc and the event handler function (imho it should be a method in the Window class, and we should eliminate the magic widget index numbers) are quite impossible to implement in an incremental way (one window at a time).

In addition, the base class gives a more precise definition of what is part of the 'general' window interface, and what is 'internal' (to Window currently). Currently that distinction is very blurred, since much code accesses w data members directly.

An underlying light-weight base window class gives room to make modular changes/improvements and create a more explicit interface since all code is not in the base class but in a derived class.

It is one of the steps to allow deriving 'class NewWindow: public Basewindow' classes in the program that exists NEXT to the currently existing implementation (without the latter coming down).
(other steps necessary are use of new and delete for Window objects, elimination of the _windows array, and the ability to create window objects from different classes (discussed further below)).

That base class should be as light-weight as possible, forwarding calls to derived classes as they occur.


Unfortunately, introducing a base class in the system has quite an impact.

The biggest one is that ultimately the window-list _z_windows will need to speak in terms of BaseWindow* rather than Window*. All FOR_ALL_WINDOWS() code needs to be cope with that change.
Although I haven't yet looked at the details, I expect that functions using the window-list will do general (ie not for a specific window) operations that can either be implemented in the base class (eg using window-coordinates or other general information), or as a virtual method that forwards the call to the derived class (eg event handling). If all that fails, a fall-back to using dynamic_cast<Window*>() exists which will always work, since it is a type-safe way to obtain a pointer to a derived class.

Note that code for individual windows will not change very much (as far as I can see). They create a window of class X, and calls from the video drivers (mouse, timers, keys, etc) arrive back at that object.

Another costs to having different window classes is that the AllocateWindowDesc() should be able to create objects of different window-classes during its call. I briefly looked into this problem, and it seems that little is actually done with the window data itself. I currently think that it should be possible to move the 'new MyAlternativeWindow' call quite close to the AllocateWindowDesc() call, maybe even give the window object as parameter to it.
If not, then a templated form of WindowClassDesc can be used so the correct window object can be created by it.

So in short: advantages of a base class are a more explicit interface, and more flexibility to changes in the window system.

I do have plans to make the above changes, but currently I only have implemented replacement of _z_windows by a std::list<Window*> structure, and elimination of _windows and _viewports (a small and simple side-track to the global goal of OO-ing the windowing system, so  FS#925  can be trivially implemented). Ultimately I think ViewPort should be put in a widget rather than a Window.

At the moment, I am re-implementing those changes in small enough steps. I have given up on the idea that I can split a file in this way.

Don't know what you have decided about BaseWindow, but I can live without it for a few patches. Below the same patch as above but without BaseWindow

Apparently, initialization was the problem rather than the base class. Adjusted the patch accordingly. Changes:
- Window class instead of struct (postponed BaseWindow for later)
- Swapped DrawOverlappedWindow() and DrawOverlappedWindowForAll(), and removed static forward declaration
- introduced global _mouseover_last_w instead of local last_w, to prevent MOUSEOVER event calls after deletion of a window

Rather than patching the static array initialization, I removed the array, and replaced it by calls to new and delete with zeroed memory. Plz apply this patch on top of windowclass2b.diff.
Old memset() calls have been replaced by deriving from the MemInitialize base class that sets allocated memory to 0 prior to the call to the constructor.

As stated, this only works for dynamically allocated objects, so memory init in the constructor is still very important.

If this is not satisfactory, plz state what is missing.

Oops, mem_init.h is not included. Sorry

alloctype_warning: Added some warning text that the size paramter of the delete operator functions should not always be trusted as a note for memory management implementors.

viewport2: Moved the ViewPort data structure from _viewports static array to Window vp_d custom extension, and removed the former static array. At my machine, the compile assert on vp_d size still holds after adding the ViewPort data structure, at other machines this may not be the case.

Various quite small and harmless changes:
- Renamed 'AssignWindowViewport()' to InitializeWindowViewport()'
- Replaced hexadecimal width and height values of the calls to decimal values
- Doxygen additions & corrections
- Few comment corrections to coding style
- station_gui.cpp and vehicle_gui.cpp: Refactored almost duplicate FOR_ALL_WINDOWS loop to seperate function
- deleted double #include in vehicle_gui.cpp
- window.cpp: Moved several window ** loop variables into the for loop, DeleteWindow() now uses the NOT_REACHED() macro (I needed to get rid of the assert()), additional _mouseover_last_w initialization (not really needed)
- window_func.h: Deleted unneeded FindWindowZPosition() declaration (makes life easier when replacing _z_windows)
- window_gui.h: Moved a block declarations with that function below the _z_window declarations for the same reason

Replacing _z_windows stack with STL list<>

From my point of view, this change does NOT:
simplify code
make faster ot smaller binary
improve readability

- Window **wz;
+ WindowListItem wz;
- Window* const *wz;
+ WindowListItem_const wz;

Why?

In my view it would be preferable to have a std::list since it manages its memory by itself.
On the other hand, it is not a critical patch, so I can also live with the current _z_windows.

Doxygen additions for core/geometry.hpp

Splitting off top-left coordinate computation of new window from allocation of it.
Second file fixes a few comment code style issues, and moves some internal comment into the doxygen doc-string

Redirecting direct calls to the window event handler to a Window::HandleWindowEvent() method so we can have derived classes with their own handler methods.

2a introduces a wndproc compare function as a temporary solution to detecting the sub-type of a build toolbar window.
2b does the replacement of w->wndproc(w, e) to w->HandleWindowEvent(e).

Note: The 5 patches above are neither added nor commented atm.

The code changes of the window stack are coming to an end. The next phase will be more oriented towards individual windows. Before making changes there, an more global plan would seem useful. Below I attach a description of changes I'd like to do. Also attached is a demonstration of adding containers to widgets, although that is still far away (for more details, see the description).
The .py file needs pyGTK to run. As is, it will show the non-resizing news-settings window in an abstract form. By removing a comment at line 9, you can get the train-group window, which resizes horizontally in multiples of 1, and vertically in multiples of 12.
For people not having pyGTK, I attached 3 screen shots. At the bottom in the resized train-group window, you can see a small space of white, since the GTK window resizes in multiples of 1 verically.

Please provide some feedback on my plans.

AssignWidgetToWindow() was not consistent. If widgets were supplied, the existing w->widget pointer was extended with ReallocT, while if a NULL pointer to widgets was supplied, w->widget was simply reset without caring about previous contents.
attached fix1 fixes that problem by assuming it has to initialize window w at all times.

Attached fix2 makes the function static, as it is used in only 1 place.

CodeChange: Moving the responsibility of finding new window position *and size* to LocalGetWindowPlacement()

CodeChange: Make PointDimension objects more easily usable

Remove contra-productive shuffling of new window afterwards

At line 948 it says "Try to make windows smaller" which is simply untrue. In that code, the window is resized to def_width/def_height. In addition, complicated window/viewport position shuffling/movement is performed seemingly to keep the entire window visible.
If one gets here without using a WindowDesc, the coordinates are hard-coded. With a WindowDesc, LocalGetWindowPlacement() takes already care of this problem. For all cases, except AUTO it is quite easily decided that the delivered coordinates will not display a window off the screen.
For AUTO case, GetAutoPlacePosition() performs a search. It uses IsGoodAutoPlace1() and IsGoodAutoPlace2() functions. Both function will never allow a new window to be put over another window, including the windows/bars at the top/bottom.
In addition, IsGoodAutoPlace1() will not allow a new window off-screen at all. IsGoodAutoPlace2() does allow partly off-screen windows, but only after the first search failed.
In other words, a new window is never put outside the display, except when there is no room otherwise. In that case, it is even unwanted to have the code afterwards move the window entirely back onto the screen.

Also added an additional check that a resize event is only performed when the size actually changes after applying the step restrictions.

Replacement for the Autoplacement function, loosely based on the existing implementation.
I can also provide a long sequence of patches to slowly move the current implementation to it if that is desired.

Added a WindowNumber to all the WC_GAME_OPTION windows

It looks like I kinda horribly broke your del_aftershuffle.diff and with that probably the usefulness of pointdim_fn.diff and wpossize*.diffs too. Seems to me that the changed characteristics cannot be easily united with the new windows.

First of all, I'd like to thank you very much for all your (and your fellow developers!) hard work in making classes out of all the windows. It would have taken me months.

With respect to you breaking del_aftershuffle.diff:
Yeah, unfortunately, communication of a Dimension from LocalGetWindowPlacement() to FindWindowPlacementAndResize() seems pretty much dead, which is needed for allowing a more sophisticated window placement code such as in new_autoplace_position.txt.

I reduced my efforts to just documenting the current situation (r13332).

Attached some new doxygen comments. Also, in src/gfx.cpp, the "byte _stringwidth_table[FS_END][224];" global should be made local to the file, as it is not used elsewhere.

Adding some flag-methods for widgets

(forgot to mention, previous was a code-change)

This code-change adds a Widget::SetDirty(const Window *) method

Final code change builds partly on latter two, and introduces Window::RaiseButton() and Widget::RaiseButton()

Oops, here is the diff file.

Code change: Prepare for more flexible ways of obtaining window size

When one removes fixed window/widget position/sizes, this information will most likely need to be obtained from a method in a derived window class. These methods cannot be accessed from the Window::Window(const WindowDesc*, int) constructor. The first possible point is the first statement in the constructor of the derived class.
The current approach of obtaining size from WindowDesc, and using that in the Window base class constructor to obtain the best position will then fail to work. As a step towards allowing window size to be obtained from a derived window class, the part of getting and using the size/position of the window has been split off from the base class constructor, and is now called from the constructor of the derived class as a second step in the initialization. This second part has been moved into the new Window::InitializeSize() function.

There is a risk that a new window does not use the 2nd part. To reduce that risk, documentation strings have been added. The more rigorous check of making such a case fail is currently not possible it seems. Some windows perform only basic initialization, and these should continue to work. The next best solution is to make it work, but in a way that makes it obvious that something is wrong. In this case, failing to call the 2nd part will not break the program, but the window will simply appear at a fixed position with an incorrect size, so it is likely that the error is caught early.

In order not to break the windows that use fixed positions, the existing Window::Initialization() function has not been changed.

Note: the patch was created by modifying the Window::Window(const WindiwDesc *, int) signature, then inserting the Window::InitializeSize() call in each derived class that failed to compile. Afterwards, the signature has been changed back to the original. In this way, I know I got all calls covered.

Why don't you move the InitializeSize stuff to FindWindowPlacementAndResize? Seems much easier to me.

I'd like that, but it is not (yet) possible.

The FindWindowPlacementAndResize() function is called near the end of the constructor of the derived class (if it is called, not all windows do that). The code in front of it assumes initialized window data values (width, height, resize.* data members) which they read and/or update.
To merge both functions, first that code in-between must be eliminated or moved out of the way, ie you'd get a new FindWindowPlacementAndResize() function that looks like

{
InitializeSize();
IntermediateCodeFunction();
if (old_used_findplacement)
OldFindWindowPlacementAndResize();
}

together with just about all constructor code in derived classes moved to 'Derived::IntermediateCodeFunction()' functions.

I'd like that but I am afraid you don't :)
This is the less-invasive alternative.

Note that I expect that most intermediate code will disappear when switching to my hierarchical widgets (it would move to the widget structure construction code), so afterwards it may be possible to do what you want (thus unbreaking my del_aftershuffle.diff :D but that is way too far ahead).

Replaced the intro gui with hierarchical widgets as a first start. The code is roughly the same as posted at https://www.tt-forums.net/viewtopic.php?f=33&t=38747&start=0&st=0&sk=t&sd=a a few weeks ago, roughly stripped to reduce it in size.
It builds on the InitializeSize() function of the previous patch.

Forgot to add two new files. Here they are.

Another go at improving widgets. This time much smaller, and using the WindowDesc struct.

I'd suggest reading the PDF first to get an idea of the patch functionality and details.

01_nested_widgets.patch
Nested widget tree classes

02_nested_widget_parts.patch
Nested widget parts code

03_windowdesc_constructor.patch
Introducing a WindowDesc constructor. Patch is large due to the large number of windows of the game. The 'real' change is in window.cpp and window_gui.h. I thought this is a bit more resistant to future changes in the WindowDesc struct than adding 2 fields everywhere.
(especially since the next change is in the next patch :P )

04_read_widget_parts.patch
Allow for defining a window as nested widget parts.

05_news_window_parts.patch
Introduction of news type0 and news history widget parts.

06_intro_window_parts.patch
Introduction of intro-screen widget parts.

07_vehicle_window_parts.patch
Introduction of vehicle list widget parts.

08_nwidgets_docs.patch
The source of the nwidgets.pdf file.

Edit: Fixed typo.

I guess we will need another iteration with comments from the OpenTTD devs, so I will simply add the changes caused by developing the GUI replacements for the group and the network server windows.

09b_fixes: WWT_EMPTY is also a legal widget, comparision between arrays is now done exactly one time, improved compare output, renamed widget -> wids to keep unique names
10b_nwidget_defaults: Added default data+tip for scrollbars, documented default data+tips in nwidgets.tex document
(extension of nwidgets) 10_sizeptrs: In the future, we may want to load sizes and resize-steps from outside the NWidgetPart array. This patch makes that possible by loading through a pointer.

11_group_gui: widget array information gets duplicated.

12_network_server_gui: 'Interesting' is an adequate description :)
I got the array duplicated, except for the buttons that lay on top of each other. The resize setting in the 'name' column also causes changes in the display_flags of those widgets.
I left it there (rather than move it to the 'info' column or so) because in the nested widget setting, that would be the best place.
To fix the incorrect values, the derived constructor resets the modified fields to the original settings.

In a nested widget tree, a 'hidden' flag should be added to the widgets. Also, unless this behaviour happens at more places, one would probably want to have a custom container that handles the removal of the columns.

How to add such a container into the parts is an open question. One solution may be to add a part that allows calling a function to obtain a nested widget.


Edit: Added patches + descriptions

Review of the full diff you gave me today - whatever I already committed.

Processed all review comments.
Fixed all names and added explicit NULL comparison where needed.
Thrown out my beautiful prime factorization code in favour of an iterative lcm computation.
Used Point instead of my NWidgetDataPartXY.

Attached you will find a patch containing all source code changes from my fs_widgets5 directory.
To discuss is how to make the documentation available. A PDF is nice but difficult to maintain for others. LaTeX is also not common. Maybe some text format, or make a Wiki page from it?

Anyone who is not able to handle (i.e. install) LaTeX is not able to do serious OpenTTD coding ;)

You detect rtl in the following way:
+ const bool rtl = (_dynlang.text_dir == TD_RTL); // Direction of the language is left-to-right

You need to add strings_func.h to the includes to make that work.

However, when I applied that it didn't look like the GUI (that used the new widgets) was actually swapped after I reopened the windows, e.g. the close and pin button were still at the same side and the resize button didn't go to the other side in the group gui/vehicle list.

Aha, found why the GUIs didn't 'swap':
const Widget *wids = (this->widgets != NULL) ? this->widgets : this->new_widgets;

This shows the old widgets as long as you don't remove them, which will (I think) be a later step. Changing it to the following 'fixes' it:
const Widget *wids = (this->new_widgets != NULL) ? this->new_widgets : this->widgets;

Only issue is that the custom (text) drawing totally messes up everything and we need to find a way to 'solve' that before we can set RTL in the nightlies

Containers didn't fill properly in their direction (ie extending their content in the direction of the container to fill it completely).
The following patch fixes that problem.

Also set the assert a bit sharper (we should get at least as much space as we minimally require)
(29_container_length_filling.patch)

Also did some fixing / extending around widget arrays that you may want to throw in trunk
(30_fixes.patch)

I'd say it's safe to assume that we're now, thanks to primarily the work of Alberth, at at least 70% completion of the whole project.

Next steps:
- check each window with big fonts; are there holes etc?
- check each window with RTL; is stuff drawn in the wrong order, is stuff drawn over other stuff?
- check each window to see whether we can make OnPaint const