News:

Simutrans.com Portal
Our Simutrans site. You can find everything about Simutrans from here.

[Project] GUI Theme

Started by Max-Max, May 31, 2013, 11:12:48 PM

Previous topic - Next topic

0 Members and 3 Guests are viewing this topic.

prissi

Different types of items in one list is confusing the user; this will not happen in order to keep things simple.

I am not way finished with the scrolled list, as there will be a derived sorted_filter_list_t will incorporate the sort controls from the list dialogues.

You seem to derivate already a lot from the target, when changing tab controls and the like ... Anyway, we warned you. ;) More serious, look at the finance window. A client area for the tabs would look ugly.

I will go on a project meeting tomorrow, so I will not be able to look at your code before Friday night.


Max-Max

#36
I told you I don't mind to dig into the GUI stuff  ;D

Well, if no one has the time to look at it until the weekend, I will see if I can send a larger patch then...
I have already some dialogues converted but things are connected and will work best with a larger patch.

Well, one of the goals are to target portable devices. For this to happen the GUI must be able to rearrange itself and to do that the concept of client areas is in need. To get the alignment to work, I found that I need in some cases implement the client concept already now.

So I would say, no, I'm still on the very same path as intended...
- My code doesn't have bugs. It develops random features...

Max-Max

#37
...regarding list items.

You can't say already now that it will be confusing for the user to mix different types of items in the same list, because we don't have anything yet where this will be applicable. We have to trust the developers that they have some sort of common sense when it comes to what to mix and not...

However, it is common praxis to use an ADT as a base item so further expansion will not impact on the already implemented system/behaviour.

Think of the scroll list as a scrollable container where each item is a container by itself. The scroll list can now serve as the list class independent of what it is displaying.

For example, you can make a vehicle list where you have one item type for each vehicle type showing different information. It could show a picture and some vital data in each list element. You can filter and mix these vehicle types in the same scroll list without the scroll list knows the type of each item.
- My code doesn't have bugs. It develops random features...

Max-Max

Some project progress updates...

The theme project has survived two trunk updates  :thumbsup:

One bigger impact of a theme is that we can't predict the size of each gui element. A check box might me larger than a button, or a edit field might be smaller than a button. When a dialogue/window builds it gui it can't rely on a fixed relation between the gui controls regarding size.

I have added a new function to the base gui class komponente_t that aligns a control to another control. The alignment can be made both vertical and horizontal in the same call. I will develop the alignment further as the project progress to have a control automatically realign itself if the alignment control moves or change.

I have also refined the number of control size constants and the list looks like this for now:

// theme system colours 
#define SYS_COL_HIGHLIGHT MN_GREY4
#define SYS_COL_SHADOW    MN_GREY0
#define SYS_COL_FACE      MN_GREY2

// default button width (may change with langugae and font)
#define D_BUTTON_WIDTH (gui_frame_t::gui_button_width)
#define D_BUTTON_HEIGHT (gui_frame_t::gui_button_height)

// default edit field height
#define D_EDIT_HEIGHT (LINESPACE+4)

// default square button xy size (replace with real values from the skin images)
#define D_BUTTON_SQUARE (LINESPACE)

// titlebar height
#define D_TITLEBAR_HEIGHT (gui_frame_t::gui_titlebar_height)

// statusbar bottom of screen
#define D_STATUSBAR_HEIGHT (16)

// gadget size
//#define D_GADGET_SIZE (gui_frame_t::gui_gadget_size)
#define D_GADGET_SIZE D_TITLEBAR_HEIGHT

// Arrow size (replace with real values from the skin images)
#define D_ARROW_WIDTH  (10)
#define D_ARROW_HEIGHT (10)

// Scrollbar params (replace with real values from the skin images)
#define KNOB_SIZE        (32)
#define D_SCROLLBAR_SIZE (scrollbar_t::BAR_SIZE)

// dialog borders
#define D_MARGIN_LEFT (gui_frame_t::gui_frame_left)
#define D_MARGIN_TOP (gui_frame_t::gui_frame_top)
#define D_MARGIN_RIGHT (gui_frame_t::gui_frame_right)
#define D_MARGIN_BOTTOM (gui_frame_t::gui_frame_bottom)

// space between two elements
#define D_H_SPACE (gui_frame_t::gui_hspace)
#define D_V_SPACE (gui_frame_t::gui_vspace)

// Divider element height
#define D_DIVIDER_HEIGHT (D_V_SPACE+2)

// The width of a typical dialoge (either list/covoi/factory) and intial width when it makes sense
#define D_DEFAULT_WIDTH (D_MARGIN_LEFT + 4*D_BUTTON_WIDTH + 3*D_H_SPACE + D_MARGIN_RIGHT)

// dimensions of indicator bars (not yet a gui element ...)
#define D_INDICATOR_WIDTH (gui_frame_t::gui_indicator_width)
#define D_INDICATOR_HEIGHT (gui_frame_t::gui_indicator_height)

#define TOOLTIP_MOUSE_OFFSET_X (16)
#define TOOLTIP_MOUSE_OFFSET_Y (12)


All these defines will be moved and replaced by the theme system later on, but are here for my convenience to test with different sizes.

I use the prefix D_ for global theme definitions and L_ if it is a local definition to tweak some dialogue specific properties. The L_ defines are in the dialogue source files where it is used.

Some controls will require new theme elements, such the tab control. This is why the client concept is so important. The hight of the tab will be decided by the theme, therefore it is important that, for example, a tab page's children (child controls) are positioned relative to the tab's client area. The client area is positioned relative to the tab control's position depending on the size of the tabs.

In these cases where a child control is placed outside the client area, it might also be an indication of that, maybe, it shouldn't be a child control of the tab to begin with. My goal is to maintain the the same structure and design of the current dialogues, but when we move away from a myriad of magic numbers, tweaking on pixel levels, it is inevitable that some dialogues will differ just a little in its layout when we replace them with the above constants.

Today I will prepare a patch with some dialogues and gui controls that I hope will go into the trunk soon.
- My code doesn't have bugs. It develops random features...

Ters

Aligning components of unknown size to each other is the biggest pain when laying out a GUI. I've seen far more unsuccessful solutions than successful. It might be better to tie all components to a common scale factor, like font size. However, even then there would still be problems matching scaled components to components that must be a fixed number of pixels no matter what (typically some raster image).

Max-Max

#40
There are some techniques to handle alignment of some what arbitrary sizes. It will probably not work with ALL theme visions, but giving a flexible framework and the tools to test a theme, I think the artists will be able to produce quite amazing designs.

We should not say people are stupid just because you CAN abuse the system and make an impossible theme... What we can do is to provide a framework for artists to work with.

The Torque2D game engine have solved it quite nice and it works pretty well.
- My code doesn't have bugs. It develops random features...

Ters

It's not the themes I'm worried about, it's coding the layouts that can be themed. I have spent hours trying to get what seems like a simple layout to work in HTML. I have tried it in Java Swing. If I positioned and sized things with pixels, it would have worked, but only with the same fonts, DPI and screen size I have.

Max-Max

I don't think we can compare this with HTML where a simple layout will look different in different browsers, no matter how x-compatible they claim to be :)

But I do see this as a great challenge to overcome... Rounding errors are always an issue that can accumulate in large and complex GUIs  ;)
- My code doesn't have bugs. It develops random features...

prissi

GEM (ok very obsolete by now) had its elements with two coordinates, one was font height/width dependent, and the other were just relative pixel (lets say one font height = 16 pixel). That would give your very much room for scaling without rounding errors (and I guess for most stuff the fine pixel will be zero). You might then change just get_pos() to something more elaborate and add a new set_pos() routine which takes two koords, with the second one defaulted to zero).

Max-Max

#44
Okay, here is the first Theme patch. Things should look almost the same as before, unless you star to fiddle with the params in frame_t. As I said before the Panel is causing some offsets, but I will continue to update dialogues... Next patch, next Friday?

In this patch:

* checkbox height set to button square
* divider default inset, added set_width()
* Label_t height set to LINESPACE
* New control: Preview_map (experimental, may be removed later on)
* Banner color ramp defined
* Buttons using default sizes through defines
* frame_t Fall back if skin is loaded but no background defined.
* Banner buttons set to half dialog width
* Fixed tab panel to offset children correct in client area
* gui_image_t
- Added enable_offset_removal() to draw the image without the offsets.
* gui.componente_t
- Added align_to() to align a control with another
* gui_frame_t - Added GUI system color
- SYS_COL_HIGHLIGHT
- SYS_COL_FACE
- SYS_COL_SHADOW
- Added D_EDIT_HEIGHT for edit controls
* Divider using sys colors
* Language Dialog updated (flags drawn without offset)
* Map generation dialog updated (control alignment)
* Added gui_componente.cc to project

* Display Dialogue
* Player color dialogue
* Options dialogue
* Savegame dialogue
* Banner dialogue
* Language dialogue
* New world dialogue
* Sound dialogue

*** EDIT ***
I have removed the patch. It was released way to early and should never have been released in the first place.
- My code doesn't have bugs. It develops random features...

TurfIt

Doesn't compile.

===> CXX gui/banner.cc
In file included from gui/savegame_frame.h:20:0,
                 from gui/loadsave_frame.h:12,
                 from gui/banner.cc:22:
gui/components/gui_divider.h:42:61: error: 'EDividerStyles' is not a class or namespace
gui/components/gui_divider.h: In constructor 'gui_divider_t::gui_divider_t()':
gui/components/gui_divider.h:40:36: error: 'EDividerStyles' is not a class or namespace
make: *** [/build/gui/banner.o] Error 1


EDividerStyles, fComponent, fAlignment, kALIGN_TOP, etc. Such camel case and strange prefixes haven't been the style used in Simutrans so far, not sure we want to go there...

Any dialogs where simply magic numbers were replaced could be committed straight away if they were seperated out...  A 8000 line patch takes quite a while to wade through - having 'simple' changes separate would help greatly.

There's many places where the TAB indents have been replaced with spaces. Please put the TABs back (and use TABs for added lines - initial indentation, spaces thereafter).

Max-Max

I merged, compiled and tested with 6534, non problems here... I'm not sure why you get compiler errors, did you do a clean solution and rebuild all? I have noticed that VC sometimes ignores a compile if only the header has changed.

Names such as EDividerStyles and prefix f is an old habit of mine, I just do it without thinking. However, having a parameter name that is the same as a member variable may cause hard to find bugs. The prefix f is just a way to make sure it isn't the same as a member variable.

Regarding tabs & spaces, I just followed what Prissi answered on the question
QuoteI use 2 and 4 also sometimes. The beginning of a line should be tabs; after that spaces (i.e. when continuing if brackets).

Definitions should rather use spaces.

But that point again at reworking the dokumentation.

...and since Prissi seems to be the one working on the documentation, I followed his advice (or did I miss interpret it?)

I will change these names for you and chop it up in smaller pieces. I'm on my way to a 2 days music festival starting today, so I'm not sure if I have the time this weekend...
- My code doesn't have bugs. It develops random features...

Max-Max

Quote"...and strange prefixes haven't been the style used in Simutrans so far, not sure we want to go there..."

What do you recommend to name params so they don't collide with member names?

A prefix is the easiest way to make it clear that it is a parameter and it will ensure it doesn't collide with a member. A parameter with a member name will silently override and cause hard to find bugs if you aren't familiar enough with the code.
- My code doesn't have bugs. It develops random features...

TurfIt

Scoped enums is C++11 (and apparently a non-standard MS extension). Simutrans still uses C++98 - best to set your compiler to that (and disable MS stuff).

Scroll bars should be placed at the edge - no margin. Just look at the convoi info, halt info, and minimap post patch...
Default scrolly heights (and minimums) should be set for an integral number of entries - prevents ugly clipping IMO.
Chart sizes - the patch has shrunk them all to uselessness.
The savegame dialog was vertically sized to fill the screen before...
Linemanagement - scroll area is overlapping the filter text box.
Depot - divide ris overlapping the append/sell/front button.
Player List -  what happened ??

prissi

IN order to have meaningful comments you need to make patch for one feature a time. I.e. one for new GUI elements, one dialogue at a time and so on. Otherwise we have a patch which turns a fully functional game into a very experimental agglomeration without much chances of testing. It means also taking everything or nothing; However, I feel that soem dialogues may be improved anyway. Thus any realistic realisation must be able to cherrypick things people like and will not like. Especially at this early stage, where we all have to find a route ourselves.

However, I am quite ill today, and will not have much time to comment on details until Monday I fear. Sorry, but TurfIt gave already some remarks.

Max-Max

Since everything is connected, it doesn't make much sense to make any patches at all at this stage because not adjusted Dialogues and controls will be misaligned. This is exactly what I predicted would happen.

I will not make any patches until at least stage 1 is finished, then we can discuss how we can implement a huge patch, probably in chunks. But it will not really be usable until all chunks are in place.

Markohs suggested patches after stage 1 & 2, Prissi for each dialogue... I think it would make sense first after stage 1, no themes at that stages, but many theme params would be able to adjust through the defines.

If the project file isn't set up correctly, please update it in the trunk so we all work with the correct settings. The current version for download is VC 2012 Express and the update works fine but someone familiar with VC Express and the Simutrans' build process, needs to see if the conversion is changing any vital VC settings.

It would be good if some one could fix all the compiler warnings. I could do it myself, but I don't have enough knowledge of the code to decide if a type should be updated or type converted. When the warnings are fixed, it is easy to spot and fix your own warnings.

I have a clear vision of this project and maybe I didn't share it enough. I have updated the project description to give more details.
If there are some unpublished document for GUI design, please add them to the trunk for everyone to read.

When it comes to coding style I do read the style document and when I can't find what I'm looking for, I check the code. I have some habits that I do without thinking and I do go back and correct them when I see it.

I think the coding style can be improved to remove potential bug traps. One, for example, is to put a prefix on function parameters.

Consider this example:

void set_groesse(koord groesse) {

  this->groesse = groesse;
  groesse += koord(10, 10);
}

The code will compile fine but create a bug where the 10 pixel offset will be applied to the param instead of the class member. By using a prefix, this can be avoided.

void set_groesse(koord fGroesse) {

  groesse = fGroesse;
  groesse += koord(10, 10);
}

In the current code, I have seen the use of _ as prefix at some places, but _ or __ are usually used by compiler extensions and might be misinterpreted by some one used to work with this nomenclature. I strongly recommend that we use a prefix for params. if it is an f or something else, doesn't matter, just pick one, but maybe not the _ or __.

enums are not mentioned in the coding guide, I just copied what I already found in the code. If that is the wrong way, you need to put it into the coding guidelines.


Yesterday I was listening to some great artists like, La Fleur, Steve Aoki and David Guetta to mention a few. Today it will be Albin Myers, Alesso and Axwell to mention a few :)
- My code doesn't have bugs. It develops random features...

Ters

I suggest sticking to (one of) the current coding style(s) and discuss the coding style separately (there might be a thread somewhere). Waiting for us to agree about a coding style, especially when there is constantly new input like this, will just bog you down.

prissi

Well, please break you patch down. Make one to get ride of the titlebar, and then we find about the rest.

About your overlaying groesse: You could easily name the function parameter gr. (Apart from the fact that your code will generate the warning of an assigned value which is not used.)

The f prefix in windows (or rather UPN?) stands for float. I would be deeply confused but this.

Max-Max

Prissi,

That was just an example to show the problem when a parameter have the same name as a member. I have seen plenty of this in the code. Yes a warning is issued, but since there already are plenty of unresolved warnings, I would have to scan through them after each compile, to see if one of them is mine. Wouldn't it be better to just stick to some rules rather than relying on a compiler warning (or even better; fix the warnings)?

I have seen gr as parameter name in the code to. But even if the coding document don't state that a variable has to be meaningful, as you do for function- and class- names, I assume the same thing goes for variables and this is why we see the use of the this keyword.

f was also only an example, as in
Quote...if it is an f or something else, doesn't matter, just pick one...
The message I tried to get through with, was to put a prefix or suffix so it will not collide with any member variables. Then we can actually call the params for what they are instead of cryptic short cuts and remove potential collisions.

Ters,
I did try to stick to one coding styles, but it was apparently the wrong one...  :-[

Okay, lets see if we can find a thread to discuss this, but it would be good to solve this ASAP so I don't have to rewrite everything again...
After a search on Coding style I found 2 of my own threads: Code indent and tab size and Coding style.

The first one has started the subject but maybe I can change the name of the thread to "Coding Style Document". The second one has a good name for the thread but it has very little to do with the subject. Should I rename the first one so we can continue in there?

So, TurfIt, Prissi and Markohs, what about the other comments?

Updated project file with the correct settings?
GUI design document?
Fixed compiler warnings?

When can I expect to get this so I don't make more unnecessary mistakes again?

Now regarding the Theme project.
What is the point to replace only one of many magic numbers? Then I will have to do the same work again... and again... and again... for each magic number.
What if I do step one, dispatch one patch for the GUI controls, that will mess up some stuff, then one patch for each dialogue. After each dialogue patch, that dialogue would be okay again. Quite simple to detect errors in the dialogues when they are one by one.

In the later case, I can fix all the magic numbers at once for each dialogue.
- My code doesn't have bugs. It develops random features...

TurfIt

Quote from: Max-Max on June 09, 2013, 02:08:59 AM
Then we can actually call the params for what they are instead of cryptic short cuts and remove potential collisions.
For your example, I'd probably call the param 'new_size'. More descriptive than simply 'size' and avoids the aliasing.


Quote from: Max-Max on June 09, 2013, 02:08:59 AM
After a search on Coding style I found 2 of my own threads:
Thread I started (in forum you don't see) to obtain a consensus on updating coding_styles.txt died a quick death... Guess it's a free for all.


Quote from: Max-Max on June 09, 2013, 02:08:59 AM
Updated project file with the correct settings?
GCC is the primary environment. MS files are I think a courtesy, I have no idea how one would update them.


Quote from: Max-Max on June 09, 2013, 02:08:59 AM
GUI design document?
No such hidden document exists.


Quote from: Max-Max on June 09, 2013, 02:08:59 AM
Fixed compiler warnings?
GCC currently only warns of unused variables in asserts - annoyingly. I've heard MSVC goes rather even more overboard with its warnings. If there's something serious contained in one, maybe post it up in a different thread...


Quote from: Max-Max on June 09, 2013, 02:08:59 AM
Now regarding the Theme project.
What is the point to replace only one of many magic numbers? Then I will have to do the same work again... and again... and again... for each magic number.
I think it was expected that the magic number removal in any particular dialog would have no effect on others. What is being changed in your patch that screws up dialogs you've not changed?

Max-Max

#55
Thank you TurfIt for your answers :)

There is not much use of continuing a discussion in a forum the whole community can't access. I have renamed the subject of my thread, maybe we can continue that thread or you can make your thread public to the community.

Regarding MS project file, this isn't my main development tool, so I'm not the right person to go through it.

If there is no GUI design document, there is no way I can know how things "should" be done. I guess, I'm free to use my own interpretation in this matter  :D

I will post the warnings in a new thread...

The magic numbers doesn't only contain the D_TITLEBAR_HEIGHT, it also contains all the Margins, pixel adjustments and other unknown dimensions. Depending on which container they are in, they also contains adjustment to its parent control, the tab control's children has the size of tab area hard coded. Some controls, like the chart, is drawing outside its bounding box. In short, it's an awful mess!

To fix all the magic numbers in each dialogue, some GUI components needs to get fixed so they a) draw inside their bounding box and b) reports the client area to its children so they can be laid out the same way even if the client area moves or change in size.

It doesn't matter how much we cut down the patches, in the end, as soon I change the GUI controls they will break the dialogues until each dialogue has been fixed.

To save work and not have to go through every dialogue multiple times for each stage in the patch creation, I would rather get rid of all magic numbers at once. This also gives me a chance to adjust and test my solutions before I submit the patches.

When stage 1 is finished I will en up with a huge patch. But I can create one patch for the GUI elements and then one smaller patch for each dialogue. After applying the GUI patch, things will be messed up, but for each dialogue patch, that particular dialogue will/should be normal again. In this way each patch will be much smaller and easy to test.

If you create a temporary branch to apply the patches, you can be several ppl working with them at the same time, dialogue by dialogue. Then when everything has been implemented and tested, merge it into the main trunk.
- My code doesn't have bugs. It develops random features...

prissi

QuoteIt doesn't matter how much we cut down the patches, in the end, as soon I change the GUI controls they will break the dialogues until each dialogue has been fixed.

Why changing GUI controls? This is the last thing to do, as we have working dialogues. Also comboboxes needs to draw outisde their bounding box, that is part of their way. For charts, well changing it so it uses predefined spaces from the borders of its own bounding box (you me the size of the graph) can be done, and may affect different dialogues. First thing needs to be to make everything scaleable. Then there can be patches to change charts etc. Doing everything at once will be much more work for you, for us and unlikely leads to something good enough to everyone that I can integrate it.

Moving stuff within dialogues will not affect the overall code. Thus first priority should be moving dialogues. Best to start with those which are not scalable, i.e. not use any of the D_MARGIN_... When all dialogues use this, removing D_TITLEBAR... is just assigning one number.

Max-Max

QuoteWhy changing GUI controls?
I think you guys are making this a bigger issue than it is. I have already updated the Chart control to move inside its bounding box, it was already scalable. The tab control has already been updated to work with a client area and are already transforming events to the client coordinates.

The biggest issue TurfIt mentioned was when a chart was located inside a tab. That gives a double effect on unmodified dialogues:
1. The chart coordinate includes the tab heights as a magic number, but are positioned relative to the client. Meaning they get to far down in the client area.
2. Because the chart now rescale and draws inside its bounding box, instead of outside, the available client area has shrunk. In dialogues where I have not yet adjusted the components locations will display a very small chart.

I have already updated the buttons to work with the predefined sizes like D_BUTTON_HEIGHT, D_ARROW_WIDTH, D_BUTTON_SQUARE etc... These behaive exactly the same as before.

The edit control has been updated to use the new D_EDIT_HEIGHT, but if you define D_EDIT_HEIGHT as D_BUTTON_HEIGHT they look the same as before.

If all D_MARGIN_XXXX are set to 10, it will look as before, except for the fact that nothing will be the margin area, scrollbars are moved inside the margin. This is because the window border will be drawn in the margin and the margin will be decided by that border later on. A more realistic value might be in the range 3 to 5 later on.

QuoteAlso comboboxes needs to draw outisde their bounding box
The edit control of the combobox is inside its bounding box, but as you know, the drop down list is placed on top of the GUI, as an independent layer. There are no controls that needs to be adjusted because of the drop down list. Therefore the bounding box is always only related to the edit control of a combobox. However the dropdown list has a bounding box (edit width and x items down) on its own "layer" on top of the GUI and a client area for the list items.

QuoteBest to start with those which are not scalable, i.e. not use any of the D_MARGIN_...
All dialogues I have seen so far are using margins as a magic number. Sometimes the magic numbers are separated and sometimes they are just a sum and I have to guess what's included.

I don't see the point in replacing one magic number with another to only remove one part of it. If I'm in each dialogue, why not fix all the magic numbers? When I see numbers that can be configurable I create a local define for easy configuration. For example, the file dialogue has a define deciding how many rows the dialogue should display. If you think they are to few, just increase the define and the dialogue is calculating the new dialogue height automatically.

Every time I notice a change in your trunk I merge it with my local theme branch, this means that the whole theme is already merged with the latest trunk code. If you prefer to get my, already merged, theme trunk I can zip the code and send it to you. No need for you to patch, only compile, run and test... If you want to see the changes, just run a diff against your trunk...

What you basically is asking of me is to revert 3 weeks of coding, replace a magic number with another magic number, then do the whole process again, and again, and again until all magic numbers are gone. If I notice that I should have done something different half down the road, I have to revert all code again and repeat the process and you will be frustrated that I'm send you new patches that makes the old ones obsolete. This only generates more work for all of us...

I see now that it was a huge mistake to send any patches at this early stage, before I had finished phase 1  :-X
- My code doesn't have bugs. It develops random features...

prissi

#58
Actually, patches as early as possible. In the end I have to integrate a patch, whcih leaves me with such large patches in the position to either take all or leave it. (Which both is not desirable. As the deed is done, I will wade through your code slowly and then commit first stuff one by one.

So some initial remarks:
In banner you are redefining LINESPACE as LINESPACE_EXTRA_2: Why add this confusion? Also you defines of LINESPACE_EXTRA_5 and ..._7 are not helpful if I see them later.

Imho you better keep the existing stuff there, and sum them up. It is a long term anyway. Or, even better, do the resizing during construction of the dialogue, as in some many dialogues.

The comments about the strange naming of constants was already given ... Simutrans uses Uppercase for those usually. (Actually my MSVC complains that "EDividerStyles::kIn" is non-standard, which is using in virtual init in gui_divider_t". Also, why is this a virtual function?)

Also the enum is dead ugly. Especially since it is shared, I would rather have something global like the ALIGN_LEFT|MIDDLE|RIGHT for display_... too.

Furthermore I fail to link:
1>gui_map_preview.obj : error LNK2001: Nicht aufgelöstes externes Symbol ""public: virtual class koord __thiscall gui_komponente_t::align_to(class gui_komponente_t *,enum gui_komponente_t::EControlAlignments)" (?align_to@gui_komponente_t@@UAE?AVkoord@@PAV1@W4EControlAlignments@1@@Z)".

You were also patching the MSVC project file to a number not working any more for me ...

I also explicitely reserved the list windows several time for rework. Really, no need to touch them at all, I am planning to write them more or less new from scratch (same for line management window).

Max-Max

As I said, it was an mistake to release that patch so early. The banner.cc was one of the first I "converted" and after a while I kind'a found e better work methodology.

QuoteIn banner you are redefining LINESPACE as LINESPACE_EXTRA_2: Why add this confusion? Also you defines of LINESPACE_EXTRA_5 and ..._7 are not helpful if I see them later.
LINECPACE isn't redefined, LINESPACE is still defined as (large_font_total_height).
The banner used a lot of magic numbers instead of just LINESPACE. If I had replaced them all with LINESPACE the layout of the banner would have been different. I figured that I would probably get a load of complaints if it did, so I tried to map them to already defined dimensions. This is kind'a bad idea too because these distances might change in the theme later... But as I said, they are there for elaboration...

LINESPACE_EXTRA_2 replace the magic LINESPACE+2
LINESPACE_EXTRA_5 replace the magic LINESPACE+5
LINESPACE_EXTRA_7 replace the magic LINESPACE+7

The easiest would be to just use LINESPACE in all these places... But since I made defines for them you can experiment yourself. Just LINESPACE makes the dialogue a bit awkward to read because of the shadow text being larger than the ordinary LINESPACE.
QuoteThe comments about the strange naming of constants was already given...
I did try to find information about the enum and I also asked about them without getting a straight answer.
QuoteSimutrans uses Uppercase for those usually.
Usually?!? It either do or don't. Usually means I can use whatever standard I like... The big fuzz about the enum naming made clear it was wrong, even if I had copied some of it from the source. But still not a straight answer on how it should have been done. Please decide how enum should be named and implemented.
QuoteActually my MSVC complains that "EDividerStyles::kIn" is non-standard, which is using in virtual init in gui_divider_t". Also, why is this a virtual function?
In the early stage I had a vision to separate the gui controls a bit more, to derive and create a bevel/panel control. But when I realised that the whole gui would need an overlook in its oo design, I thought that it might be another project in itself.
QuoteAlso the enum is dead ugly.
Yes, there is almost a whole thread telling me this, but no straight answers...
QuoteEspecially since it is shared, I would rather have something global like the ALIGN_LEFT|MIDDLE|RIGHT for display_... too.
I thought about it too, but I had more functions in mind. The enums looks like this now:

enum control_alignments_t {
align_none       = 0x00,

align_top        = 0x01,
align_v_center   = 0x02,
align_bottom     = 0x03,
align_v_interior = 0x00,
align_v_exterior = 0x10,

align_left       = 0x04,
align_h_center   = 0x08,
align_right      = 0x0C,
align_h_interior = 0x00,
align_h_exterior = 0x20
};


Except for the dead uggly enum style I don't think it's such a good idea to combine them. The other are for text justification and should probably be in their own namespace so it can be developed further without interfering with other areas. These are for control alignments and not text justification.

If you want all enums to be public, sure no problems just tell me how to name them. If you want them all upper case, how do you tell a #define from a constant? A #define has no type checking, unless you include it in the definition, while const and enum does. The Taligent coding standard, developed by IBM and Apple suggests the prefix k infront of const and enums to show that they are just that. Here is a quick table from Taligent's Guide.
QuoteFurthermore I fail to link:
1>gui_map_preview.obj : error LNK2001: Nicht aufgelöstes externes Symbol ""public: virtual class koord __thiscall gui_komponente_t::align_to(class gui_komponente_t *,enum gui_komponente_t::EControlAlignments)" (?align_to@gui_komponente_t@@UAE?AVkoord@@PAV1@W4EControlAlignments@1@@Z)".
You have probably forgot to add gui_komponente.cc to the project.
QuoteYou were also patching the MSVC project file to a number not working any more for me ...
I don't know what version you are using, I'm using the latest available for download, VC 2012 Express. Maybe time to upgrade, free of charge! ;)
I included it because I had added some files to the project... If I'm not allowed to use that version or add new files to a project, or if there are any special procedures for this, you have to document this as well.
QuoteI also explicitely reserved the list windows several time for rework
I stopped the work immediately when you said so... Again this patch should never have been uploaded in the first place...

I really appreciate that you took the time to have a look, but I feel sorry for you that you have been looking at a patch that shouldn't have been uploaded in the first place. As I also mentioned earlier, some code will be rewritten and changed a number of times and become outdated.

I also appreciate critique and feedback, but I don't care much for comments that tells me; this and that is sooo wrong and stupid, without telling me how it should be done instead. It isn't easy to know what is considered good or bad by looking at a source code written in many different styles...

I think I know how we can proceed in smaller chunks, I just need to figure out in what order to release each part. I think the gui_button_t, gui_frame_t, gui_numberinput_t and gui_komponente_t can be quite safe to implement. I have updated these to use the predefined dimensions. I just need to do a test merge with your trunk to see if it creates any strange behaviours first...

After that I think I can send you some dialogues that has no, tab or chart controls in them. Ohh... and simwin + werkzeug_waehler_t where the main handling of D_TITLEBAR_HEIGHT modification are.
- My code doesn't have bugs. It develops random features...

prissi

Now discussion started. Usually there are not rules in stone, we try to be flexible. And you have already lots of code, so very good so far. I do not want to have put it as a bashing. I would just suggest to ask about some tiny bit first, as you may get negative feedback early on.

For naming convention:
Simutrans uses all uppercase for constant stuff (read for stuff you do not put into the left side of an assignment). It does not care whether is is a define or const.

If you are worried about assignments to font_height because of #define LINESPACE (fontheight) use simply #define LINESPACE (fontheight+0) You cannot assign this or pass it by reference.

About reusing alignments:
The only thing that currently uses alignments are labels. All other stuff does its alignment itself, which is fully intended. But ok, assume we need alignments.

Then one could easily define (or enum in the global! namespace)

H_ALIGH_LEFT=0, H_ALIGN_CENTER=1, H_ALIGN_RIGHT=2, V_ALIGN_TOP = 0, V_ALIGN_CENTER = 4, V_ALIGN_BOTTOM=8
and for convenience people may want (although I am not sure this should be done)
ALIGN_H_CENTER_V_CENTER = 5

Voila, free alignment for almost any stuff. (also for images and the low level display routines, I personally would ignore the vertical part.)

I still have to look through the code, because at the moment, between making thing skinnable and introducing a lot of changed gui_components is a difference, which probably confused me. Therefore, I still have to find out about you actual aim.

The gui_numberinput is anyway somewhat tainted, as it does not trigger sometimes instant renaming action and other times not. If you want to work on gui_numberinput_t, it would be a good time to have it consistent.

Max-Max

#61
When naming a group of constants I always try use a name convention that follow a group order. Like the D_MARGIN_XXXX defines do (I guess the D_ stands for dimension/distance?).

ALIGN_LEFT     = 0,
ALIGN_CENTER_H = 1,
ALIGN_RIGHT    = 2,
ALIGN_TOP      = 0,
ALIGN_CENTER_V = 4,
ALIGN_BOTTOM   = 8


The alignment bits can be combined, just as you assumed.

I'm not sure if we should put H and V in every alignment. There is no doubt what LEFT, RIGHT, TOP, BOTTOM means, while CENTER needs to distinguish horizontal from vertical. Personally I'm always confusing horizontal and vertical because different software interpret them different (like moving along the axes or rotating around the axes).

When I make these global, where should I put them? simtypes.h?
- My code doesn't have bugs. It develops random features...

prissi

Since they need to be read by display_xxx, simgraph.h seems reasonable.

Max-Max

All right, considered it done  :)
- My code doesn't have bugs. It develops random features...

Max-Max

Hmm, ALIGN_LEFT is already defined but as text justification.

A suggest the following mod to the text justification enum:

enum text_justifications_t {

  JUSTIFY_LEFT   = 0 << 0,
  JUSTIFY_MIDDLE = 1 << 0,
  JUSTIFY_RIGHT  = 2 << 0,
  JUSTIFY_MASK   = 3 << 0,
  DT_DIRTY       = 1 << 2,
  DT_CLIP        = 1 << 3
};
typedef uint8 text_justification_t;


I renamed them to justification instead of alignment. So far when I looked at it it was only used with text justification.

I added a namespace because some IDE will show this in the watch window and it gives you the information of where it comes from right away.

I also created a type for them of the following reason.

void foo(text_justifications_t  justification_par) {

  // do something
}


As long you use one of the values in the enum it compiles fine, but if you combine them, you will get an error/warning because the combined value isn't a member of the enum.

void foo(text_justification_t  justification_par) {

  // do something
}


(The difference is in text_justifications_t vs text_justification_t)
Now all combinations will be accepted and the IDE will show you that the param is of the text_justification_t type. No doubt of what you are expected to put there as a parameter.

This is only a simple renaming of the ALIGN_XXXX to JUSTIFY_XXXX in your current trunk. I can do the replacement and conversion as a separate patch and send you to update.
- My code doesn't have bugs. It develops random features...

prissi

Why should text alignment do something else? My suggestion was just use the existing modifiers.

I would also advise against falgeritis. A justify flag should not carry the clip or dirty meaning. I know is is done for embedded and so on, but this is neither memory critical nor will an additional parameter impact performance (compared to the drawing). Simutrans tarted as an excercise to make clear OOP code. As such we tried (not always successful) to keep the code as simple as possible. It results in easier understandable code, and less errors and usually helps often even the compiler's optimizer.

But back to the project as it is: Why do you feel the UI needs the alignment modifier? Any Object has already a bounding box and could decide itself how to scale with size changes. The labels have text, color and alignment, but those are reather put through to the actual drawing.

Forcing thing to scale from the "outside" feels a little against the thing of OOP, i.e. gui stuff as very independent objects.

Max-Max

Why do we need alignment?
I'm not sure if you understand what I have done. I'm not talking about text justification. I'm talking about object alignments, aligning bounding boxes (controls) relatively to each other. There are tons of places where a label is centred around an edit field for example.

What is most clear to you?

foo.align_to(&bar,ALIGN_CENTER_V);

...or...

foo.set_pos(foo.get_pos() + koord( 0, (bar.get_pos().y - foo.get_pos().y)/2))

There are already a lot of cryptic formulas for control alignment. By having the alignment function built in to the object you can just simply tell it just align with another object. I do plan to create a group control that can distribute child controls in various ways.

Strange enums?
If you are referring to DT_DIRTY and DT_CLIP, that isn't my enum, it is the existing one and my proposal to rename it to JUSTIFY_XXX instead of ALIGN_XXX. Those flags where merged in by tron in version 1050. The comment say @author Volker Meyer, prissi So don't blame me for that enum :)

Sure I can change the numeric values of the existing to the ones in my enum, but I have no idea of what impact that might have on the current code. I don't know what to do with the DT_CLIP, DT_DIRTY and DT_MASK.

The alignment values I have are selected in such a way that you can combine them. You can in a single operation align object Foo's right side to object Bar's left side (exterior) with an offset of x and at the same time align Foo's vertical center to Bar's vertical center. That would look like this:

Foo.align_to(&bar, ALIGN_EXTERIOR_H | ALIGN_RIGHT | ALIGN_CENTER_V, koord(10,0) );

I plan to make it possible to have Foo now always follow Bar with this alignment relation. Whenever Bar moves, Foo will follow.

Why rename current ALIGN_XXX?
Because current ALIGN_XXX is only used to justify text within a text control, while my new ALIGN_XXX is aligning objects to each other. These are quite two different functions and it would be wise to separate them. The current ALIGN_XXX that is text related could be expanded with, bold, italic, column, underline etc... Maybe it should be called TEXT_ALIGN_LEFT, TEXT_ALIGN_RIGTH etc.. Then you could expand it with TEXT_BOLD, TEXT_SHADOW etc...

So to make it absolutely clear what my intention is:

CURRENT TRUNK CODE

/*
* len parameter added - use -1 for previous behaviour.
* completely renovated for unicode and 10 bit width and variable height
* @author Volker Meyer, prissi
* @date  15.06.2003, 2.1.2005
*/
enum
{
ALIGN_LEFT   = 0 << 0,
ALIGN_MIDDLE = 1 << 0,
ALIGN_RIGHT  = 2 << 0,
ALIGN_MASK   = 3 << 0,
DT_DIRTY     = 1 << 2,
DT_CLIP      = 1 << 3
};



PROPOSED CHANGE

/*
* len parameter added - use -1 for previous behaviour.
* completely renovated for unicode and 10 bit width and variable height
* @author Volker Meyer, prissi
* @date  15.06.2003, 2.1.2005
*/
enum text_attributes_t {

TEXT_ALIGN_LEFT   = 0 << 0,
TEXT_ALIGN_MIDDLE = 1 << 0,
TEXT_ALIGN_RIGHT  = 2 << 0,
TEXT_ALIGN_MASK   = 3 << 0,
DT_DIRTY   = 1 << 2,
DT_CLIP   = 1 << 3
};
typedef uint8 text_attribute_t;

/**
* Alignment enum to align controls against each other
* Vertical and horizontal alignment can be masked together
* Unused bits are reserved for future use, set to 0.
*
* @author Max Kielland
*/
enum control_alignments_t {

ALIGN_NONE       = 0x00,

ALIGN_TOP        = 0x01,
ALIGN_CENTER_V   = 0x02,
ALIGN_BOTTOM     = 0x03,
ALIGN_INTERIOR_V = 0x00,
ALIGN_EXTERIOR_V = 0x10,

ALIGN_LEFT       = 0x04,
ALIGN_CENTER_H   = 0x08,
ALIGN_RIGHT      = 0x0C,
ALIGN_INTERIOR_H = 0x00,
ALIGN_EXTERIOR_H = 0x20
};
typedef uint8 control_alignment;


I don't know what to do with the MASK and DT flags...
In this way you can continue to add stuff that are text related to the text_attribute_t.

Forcing thing to scale from the "outside" feels a little against the thing of OOP...
The alignment isn't resizing anything, it is moving the whole object. It is very OOP when you just tell an object to follow another with a set relation. But again, i think you have misunderstood the use of it. My ALIGN_XXXX is used in a call to a member function align_to(gui_komponente *target_object, control_alignment alignment, koord offset )
- My code doesn't have bugs. It develops random features...

Ters

Quote from: Max-Max on June 11, 2013, 10:50:41 PM
I'm not sure if you understand what I have done. I'm not talking about text justification. I'm talking about object alignments, aligning bounding boxes (controls) relatively to each other. There are tons of places where a label is centred around an edit field for example.

What is most clear to you?

foo.align_to(&bar,ALIGN_CENTER_V);

...or...

foo.set_pos(foo.get_pos() + koord( 0, (bar.get_pos().y - foo.get_pos().y)/2))

I think the idea is that the labels controls shouldn't be centered on the edit fields, the label should match the edit field. The text inside the label would instead center within the label control, and therefore appear centered on the edit field as well. The result looks the same, but only one level of alignment is needed, not two.

I am familiar with GUI frameworks with two levels of alignments, first of controls within the layout grid and then of text within the controls, but where it makes a difference, I think I have always used the fill alignment.
Besides, the align_to function seems to assume that the edit field is larger, which may not be the case. It is perhaps unlikely for an edit field to be smaller than just plain text, but what if it was a check box or an image instead of an edit field.

prissi

About alignment: If you have control x aligned relative to control y, you need to align control y at a certain point.

And control_x.set_pos( control_y.get_pos() - koord(foowifth+D_SPACE_X,0) ), makes it easy to add stuff. (At a certain point you still have to set_pos():  If however, control x depends control a which depends on c and which finally is from control f: then how on earth adding stuff to it in a controlled position? Also rigth aligning stuff does not obey left borders at all. The D_MARGIN were specially introduced to have a uniform border.

So, I am not fully convinced. Such alignment will fail badly either in other languages (when width is taken automatically from the strings) or you have to specify a bounding box. But then it is trivial to align it anyhow.

So my idea for Labels next to controls
label.init( koord( D_MARGIN_LEFT, current_y ), koord( BUTTON_WIDTH,BUTTON_HEIGHT), text, color, alignment  )
button.init( koord( button_pos2, current_y), ...

If there is a very common control (which is not imho) then maybe the label and the next control should  be rather merged. Then also the controls tooltip (if any) could cover both.

Furthermore, using fixed X coordinates for buttons (as most the dialogues do now) means that a button is always at the same position relative to a dialogue.

I have to add, that I would only expect text to align itself within the bound box. As such an inner Alignment, which otherwise centers single line text inside the bounding box, and put multi line text into to left corner seems enough for simutrans.

But as this is my view I appreciate further discussion on this.

Max-Max

The question wan't about the existence of an align_to() function. It was about naming and renaming enums. I wanted to distinguish between object alignment and text justification.

None of you have seen the function align_to(), how it is used, where it is used or what it can do.

My point is that text justifications are attributes to a text, more than only justifications. In that category you can also put shadow, bold, italic, underline etc..

My alignment enum has nothing to do with text.
- My code doesn't have bugs. It develops random features...