Feat/view event and names#5974
Conversation
Reorder initializers in constructor to fix compiler warning.
A distance variable was not initialized which led to a compiler warning. In a worst-case scenario, this could also lead to indeterminate behaviour when calculating distances to a bound.
|
No complaints here - this is a generally good change, my only nitpick is that we don't actually have a SettingsView and that name isn't used anywhere. You could take it one step further in fact, and merge all of PiGuiView into the View class - PiGuiView was only separate for incremental migration of screens which used the legacy Old/NewUI systems (circa 2020) and is not intended as a permanent thing. |
Remove an unused variable in CityOnPlanet.
Merge the functionality from PiGuiView into the base-class View and remove the now obsolete PiGuiView. This is in preparation of refactoring the View names and simplifying how the Lua side accesses Views.
mwerle
force-pushed
the
feat/view_event_and_names
branch
from
cea0b81 to
a35da43
Compare
Hmm, literally replaced what was there to match the new naming - perhaps it was a leftover from previous refactors.. I should've checked a bit closer. Ok, killed |
Make all string representations of the views follow the same pattern, and give views which did not have a name a name. Following this, use that string representation in the Lua side as well instead of using a separate string. This allows for unique and easy referencing of views across the entire codebsae using 'grep' or Visual Studio Code's search function. This also allowed for some simplification of getting/setting views.
Use the new 'onViewChanged' event parameters to only add/remove the radar input frame if the view changes to/from "WorldView".
mwerle
force-pushed
the
feat/view_event_and_names
branch
from
a35da43 to
68b5b4c
Compare
sturnclaw
left a comment
sturnclaw
left a comment
There was a problem hiding this comment.
Good work, thanks for tackling this! I'm glad to reduce the bug surface when it comes to identifying views between C++ and Lua. Overall it looks good and I don't see any fixup commits in the commit list, so this is getting merged!
2 participants
This PR is a proposal to be discussed prior to merge.The first couple of commits fix a few outstanding compiler warnings (gcc/Linux).
The next commit merges
PiGuiViewintoView, as suggested by @Web-eWorks. I moved this commit ahead as it is preparatory work for the next commit, and could be used as-is even without the actual features of this PR.The first "real" commit sanitized the various view names, both between all the views, as well as between the C++ side and the Lua side. Perhaps there is a good reason to have a different name between the Lua and the C++ side, but if there is, it escapes me. Personally I find the proposed naming to be much more self-documenting as well as easier to find and it reduces overloading of some of the strings which were used in multiple places with different meanings.
C++:
Lua:
This is the only change which I think needs discussing as I'm not sure if this change is valid/wanted. (I think it makes sense, but I may not have all the relevant background information..)Following this renaming, I added "previous" and "new" view names to the
onViewChangedevent.Finally, the new
onViewChangedparameters are used in the "radar" module to improve the way the input frame is added and removed, as previously discussed here : #5958 (comment)