Tile inspector element type

[amdoku]

should fix #12444

introduced a convenience method to typesafe convert from the enum class to the underlying integer type, as the enum is used quite a lot as a lookup value for tables.

[tupaschoal]

Can you please rebase and do two things:

• Drop the merge commit
• Fold the other commits into a single one, named:
  Close #12444: Refactor TILE_INSPECTOR_PAGE to use strong enum

Here's a page explaiing it: https://github.com/OpenRCT2/OpenRCT2/wiki/Rebase-and-Sync-fork-with-OpenRCT2
You'll also need to fix the formatting issues pointed out by the CI

[duncanspumpkin]

This would go in contrast to every other window. I don't think we should make this change.

[amdoku]

The page variable - this one - is accessed very often, and reused across different windows with different value ranges.

i.e. in Cheats.cpp it is used to get the value from a lookup table, which contains only four entries, TileInspectorPage containing 10.

This seems to be a reoccurring pattern in OpenRCT2, since there are a lot of union types already defined in Window_internal.h
My current approach is to replicate the union pattern for page as well.

I assume this is what @duncanspumpkin meant - please correct me if I am wrong.

[duncanspumpkin]

The problem is that every window type has its own number of pages/tabs. If we use an enum class for each of them we will need a union of every window type page type. This would be annoying to use. Alternatively we could make a generic enum class for pages 1-X and have aliases that translate a page specific name to an enum class page number. But then it's not really giving us anything useful protection wise.

I think we need to ask ourselves what are we trying to achieve and what does the final product look like. What do we want developers not to do and what do we want them to do. Off the top of my head we want to reduce macro usage to reduce name pollution. Reduce enum usage to reduce name pollution and allow for strong typing. Reduce code duplication. Prevent setting pages that don't exist. We have talked for a while about moving windows to individual classes and that will give us more tools to tackle this sort of thing but we still need to think about what the end goal should be.

[Broxzier]

We have talked for a while about moving windows to individual classes and that will give us more tools to tackle this sort of thing but we still need to think about what the end goal should be.

Pages could be lists of widgets in a vector stored with the window. Page indices or IDs are not needed, if you want to access a specific page, a pointer to the item in the vector could be used instead (which can be stored separately for better naming, of course).

[amdoku]

Some thoughts:

• A blank pointer into a std::vector, which might reallocate, would result in a use after free bug, a fixed sized std::array would have the same storage layout and very similar API - but avoids the potential use after free.
• Any container approach would have to introduce polymorphism, otherwise you would not be able to store different window types in a container - I am not sure if this is something you would be opposed to (ignoring the std::vector<std::variant<A,B,C,D,E,F> > approach for now)
• Going down the road to remove macro usage and have the undesirable union type of all panes, might not be a bad way to go forward. All the logic for the panes is already in their own class, reusing the union member variable for individual state storage. First converting to strong enums would allow for all state sharing bugs to be found and/or fixed. Then a gradual transition on a per window basis is possible without having to worry about side effects in other windows.

This is from a first time contributor to this project, so I am very much aware that all of this has been talked about by people who have much more knowledge about this project already and I just could not find it.

[duncanspumpkin]

Currently the big unions in window_internal are for accessing variables that should be private variables of the window if it was turned into class. In general writing to the page variable is done only from the specific window. Reading of the page value is also generally from the specific window but is at times also read in other locations. I'm not really suggesting anything here just writing down thoughts.

[tupaschoal]

I'm not really suggesting anything here just writing down thoughts.

Well we do need a consensus or a general direction at some point for the PR to be merged/closed ahahah

[tupaschoal]

First of all, sorry for getting back to this after a while.

Going down the road to remove macro usage and have the undesirable union type of all panes, might not be a bad way to go forward. All the logic for the panes is already in their own class, reusing the union member variable for individual state storage. First converting to strong enums would allow for all state sharing bugs to be found and/or fixed. Then a gradual transition on a per window basis is possible without having to worry about side effects in other windows.

I'm very convinced about that too. The union will look ugly, but decoupling will be nice when we have windows split up. I'm ok with merging this, are you @duncanspumpkin ?

(If so, we'll need merge conflicts to be resolved @amdoku )

[duncanspumpkin]

okay but please keep in mind this is not the end goal

[tupaschoal]

Decoupling is not the end goal? I thought ultimately we wanted a polymorphic window infra

[duncanspumpkin]

I meant that the union we have isn't the end goal.

[tupaschoal]

I know (?), for me it's a means to an end, we get a big ugly union but then we slowly move the corresponding variable to its actual window class.

[amdoku]

No problem. Discussions take time.

I'll try to find time around new years eve.