[PATCH] GUI code cleanup

[leper]

GUI code cleanup

• Style fixes (most of the patch)
  • type *name to type* name (same for references and slightly different source strings
  • Removal of some commented out code (though not all, since checking if all of it is obsolete or a possible TODO is a bit exhausting)
  • Sorted/reorganized includes
  • Added comment for the closing #endif of include guards
• Commented some classes
• Removed some non-doxygen and redundant (the classes have all the content in a comment already) comments
• Use of early returns.
• Range-based for loops.
• Some type fixes (COList now uses size_t instead of having quite some casts).
• erase-unique in GUItext.cpp
• const some things

Note: Reviewing the patch while ignoring whitespace changes might make it a bit easier.

[Itms]

It was a bit of a pain to review, but also really satisfying to see this code cleaned :p

• CDropDown.cpp
  • 101 CGUIList*
  • 104 space
  • 110 space
  • 110 spaces
• CGUI.cpp
  • 796 space
  • 925 move comment to same line now that braces are removed
• CInput.cpp
  • 215 space
  • 224 remove braces
  • 245 space
  • 248 remove braces
  • 254 remove braces
  • 283 space
  • 301 remove space
  • 302 space
  • 316 "exact"
  • actually a lot of m_Wanted?=0.f, better batch fix those
  • Maybe the documentation blocks could get * all the way down, like we do in other places
  • 408, 412, 453, 457 braces
  • 885, this has a strange indentation now
  • 1164, remove, replaced by pCaption
  • 1194 space
  • 1269 space
  • 1306, 1423: why not range-based for, with update of the other varaibles inside the loop?
  • 1407 braces
  • 1497 braces, maybe merge
  • 1510 braces
• CList.cpp
  • 108, 363 space
• COList.cpp
  • 86,102,105 space
  • 308,309 space
  • 320 space
  • 359 space
  • 374 space
  • 395, 418 space
• GUIutil.h
  • 122,156 space
• IGUIScrollBar.cpp
  • 99,158 Indentation
  • 167 remove braces
• MiniMap.cpp
  • 152,157 space

[leper]

CCing Yves to verify if this piece of code should be JS::RootedObject obj(cx, &vp.toObject()); instead. (Included this in the revised patch.)

1306, 1423: why not range-based for, with update of the other varaibles inside the loop?

Could be done, but at least one of them uses continue, which makes the whole thing a bit harder to rewrite. I also think that this is a proper (ignoring if the piece of code couldn't be written in some nicer way) use of a for loop.

All other comments are addressed in the revised patch. Also includes a few more fixes:

• Switch/case style in JSInterface_IGUIObject.cpp
• No indentation level for some defines in that file.
• A few more foo=bar to foo = bar and type *name -> type* name changes (should be all of them in the cpp files now)

[Yves]

Replying to leper:

CCing Yves to verify if this piece of code should be JS::RootedObject obj(cx, &vp.toObject()); instead. (Included this in the revised patch.)

Indeed, that's an error. I'm surprised the compiler didn't at least print a warning there. You might want to use vpObj which is already set ​here. Actually vpObj could be set a bit earlier in this function and then used everywhere (​here, ​here and ​here) instead of doing the conversion again.

[leper]

In 16909:

Actually use vpObj instead of obj, and use it in all cases. Refs #3354.

[leper]

Replying to Yves:

Indeed, that's an error. I'm surprised the compiler didn't at least print a warning there.

Possibly because of a template-d overload or something. Fixed in the above commit, to make the cleanup commit just that.

[leper]

Updated for r16909.

[leper]

incremental patch on top of gui_cleanup_2.patch. Changes: Trailing spaces, one if-else style fix, one new const function to make one other const, one new const func.

[Itms]

Here for the complete patch (found almost nothing :p )

• CDropDown.cpp, line 125 127 any clue on what this is?
• CInput.cpp, line 493 missing spaces
• GUIutil.cpp, line 280, 288 what does that do?
• IGUIScrollBar.cpp, line 146 having the comment between else and { is weird
• JSInterface_IGUIObject.cpp maybe add new lines around lines 170-174, same for line 530

[leper]

Replying to Itms:

Here for the complete patch (found almost nothing :p )

• CDropDown.cpp, line 125 127 any clue on what this is?

Setting the selected item and scrolling to put that into view. Not really sure if that is needed there though. We can remove it if you want.

• CInput.cpp, line 493 missing spaces

Done.

• GUIutil.cpp, line 280, 288 what does that do?

Nice find! Replaced both function bodies with return GUIinstance.FindObjectByName(Object);.

• IGUIScrollBar.cpp, line 146 having the comment between else and { is weird

Agreed.

• JSInterface_IGUIObject.cpp maybe add new lines around lines 170-174, same for line 530

Done.

I'll commit this after getting some feedback on the first bullet point. Thanks for the reviews.

[Itms]

I suggest removing the SetSetting line as it is obviously not used for anything, and it must come from an old change, but leaving the UpdateAutoScroll(), because I suspect we would need it if we had dropdowns in scrollable elements, which is just not the case currently.

[leper]

In 16931:

GUI code cleanup. Fixes #3354.

Lots of code style fixes:

• type [*&]varname -> type[*&] varname
• else if (...) -> else if (...)
• Spaces around some ops.
• i++ -> ++i.
• switch-case style fixes.
• Indentation fixes.
• Removing some commented out code.
• include header sorting
• Changed multiple pointer var declarations to be one per line.
• Removed strange spaces in some places.
• Changed some include header guards to be consistent with the rest of the codebase.
• Use UNUSED() instead of UNUSED2().

Some small code fixes:

• Using .find() instead of .count() == 0.
• !.empty() instead of .size() == 0.
• Range-based for loops.
• Making some member functions const by small changes.
  • Adds GetScrollBarPos(idx) const for this.
• Using early returns/continues in some places.
• Uses size_t for some loops in CList and COList.
• Removes unused heading element (not attribute) from COList.
• Use ENSURE in one case where some custom code did something similar.
• Made some parameters const ptrs/refs.
• Change removal loop in GUItext.cpp to erase-unique.
• Made some static things const.
• Allow iterating over children of IGUIObject with range-based for loops by exposing begin() and end() (rename from ChildrenIt{Begin,End}()) and use it.

Comments:

• Comment COList.
• Update a few comments.
• Remove useless or duplicated comments.