#3354 closed enhancement (fixed)
[PATCH] GUI code cleanup
Reported by: | leper | Owned by: | leper |
---|---|---|---|
Priority: | Should Have | Milestone: | Alpha 19 |
Component: | Core engine | Keywords: | patch |
Cc: | Yves | Patch: |
Description
GUI code cleanup
- Style fixes (most of the patch)
type *name
totype* 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.
Attachments (3)
Change History (13)
by , 9 years ago
Attachment: | gui_cleanup.patch added |
---|
comment:1 by , 9 years ago
follow-up: 3 comment:2 by , 9 years ago
Cc: | added |
---|
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
tofoo = bar
andtype *name
->type* name
changes (should be all of them in the cpp files now)
follow-up: 5 comment:3 by , 9 years ago
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.
comment:5 by , 9 years ago
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.
by , 9 years ago
Attachment: | gui_cleanup_inc_on_2.patch added |
---|
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.
follow-up: 7 comment:6 by , 9 years ago
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
comment:7 by , 9 years ago
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.
comment:8 by , 9 years ago
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.
comment:10 by , 9 years ago
Keywords: | review removed |
---|
It was a bit of a pain to review, but also really satisfying to see this code cleaned :p
CGUIList*
m_Wanted?=0.f
, better batch fix those