FreeOrion

Hello FreeOrion's community!

I've been posting some patches on the tracker for the last couple of days -- so far only fixing some already reported and well described small UI bugs. As I've just begun to delve into the sources any testing and constructive criticism is even more welcome than usually.

Find enjoying programming work on Programming work page.

i am sorry, but to test anything i need working win32 binary

Thanks for the contributions, but please don't post patches on sourcefoge. Post them on the forums, as otherwise they won't get noticed, and since we'll discuss them on the forums anyway.

@OndrejR: What I see is that it has fixing bugs on the first line ;-) But, yes, I've seen that and it remains to be seen if I'll be able to help with some other things.

@mZhura: I don't know about any cross-compiling scripts for FO and setting up a Windows environment is not an option at the moment (my Windows is hardly working itself, burdening it with a task as complex as compiling would surely make it reach some singularity), but thanks for your willingness! (I guess you can always test the current stable build, just check the tracker/forum to see if it's not already fixed before reporting.)

@Geoff: Hmmm, I usually find trackers and such rather useful for bug/patch management, but sure, from my side forum is just as good or even better.



OK, to the point. Here's another small patch, hopefully fixing this: https://sourceforge.net/tracker/?func=d ... tid=544942 .

There are two SidePanels: one for MapWnd and one for ProductionWnd. Both contain InfoPanels that keep track of their expanded/collapsed state using a static object->bool vector (so panels for a single population/resource centre or planet all should be either collapsed or expanded). Toggling production screen switches the side panels by simply showing one and hiding the other, info panels are not updated in any way, so their appearance becomes inconsistent with the statically stored state.

The patch adds a call to SidePanel->Update() when changing visibility of the production window (Update() is a static function that updates both side panels). That is the lightest update path available -- still it does a bit too much, however I'd rather not have anything like "UpdateInfoPanelsCollapsedExpandedState". This update is needed for showing and hiding production.

Some side notes: Why are 2 side panels needed? And, if info panels for an object are all collapsed or not maybe it should really be a single shared panel?


PS: I no longer like the forum that much, as it won't allow me to post neither .diff nor .patch -- insisting on compressing 5 lines of text.

I think I've enabled attaching .diff and .patch files... I'm an administrator though, so it might be letting me do things other can't, so could someone check?

If something's only 5 lines though, you could just post it in a forum code tag.

Some compilation warning fixes: gcc (4.3.3) has been complaining about a few unused and uninitialized variables each time, so I've made some corrections to be able to see the actual warnings. A couple can be probably commited, but some are really working around gcc not being able to infer that a variable is initialized -- may not be needed with a different compiler or even with a different version of gcc.

Empire, System, FleetWnd: Prevent some "unused variable" warnings, using some explicit style (like in other parts of the code) rather than removing (I understand this is meant to enhance readability).
Server: Don't use the same (iterator) variable name too many times (causing an ambiguity); should also be easier to read now.
CMakeLists: Add -Wno-deprecated flag only for C++.
CUIDraw, PagedGeometry/*: Be more explicit about initializing variables. [These are the "bad" ones.]

The only left, Ogre::AxisAlignedBox kind, should go away soon, due to these changes: http://ogre.svn.sourceforge.net/viewvc/ ... 28&r2=8787 .

@Geoff: Great, now it works, thank you.

You've used a bunch of tabs in your latest diff. These will need to be converted to spaces.

Edit: Also, for the unused variable warnings, you can just remove the variable declarations and avoid adding an extra line. So replace
with just

Should I replace all such variables? Personally I would as they don't add too much clarity, but there are quite a few. (Maybe they make the type more clear, but in most cases it's already rather obvious). Anyway lets decide on some style.

I believe the tabs are only in the PagedGeometry stuff that uses them everywhere, If you could, please convert it all yourself (unless you want a patch that changes each and every line that is :-)

For the stuff in PagedGeometry, I'd rather not change it at all, since it's not code written by us, and if it's modified, the notice at the top requires that we also modify it to make it clear that it has been modified. Also, the PagedGeometry stuff won't be changing very much, so presumably won't have to recompiled often, so won't be constantly generating the warnings...

I've made a bunch of other changes based on your patch. The changes to System.cpp at line 190 don't seem necessary, as opposed to the similar change at 176 which was an unused variable so could be shortened to not actually store the return value of the Object() call. If having the extra variable is causing warnings, I'll remove it in those cases, but in general it should be fine to declare and assign within an if () and this is done in many places... I think there's any special style change needed, except in cases where a warning is given about such a declared and assigned but then unused variable.

In CUIDrawUtil.cpp you've added a default case to a switch that asserts and then calls a function abort(). I modified that to add a default case that outputs some error text to the log file instead.


I discussed this with tzlaine a few years ago, and there was probably a reason at some point, but I don't remember what it is. It'd probably work find just to have a single SidePanel, now, but someone would have to go through and rewrite all the code to work that way, which I don't particularly want to do myself, and which isn't really necessary since things are working now, more or less.

PagedGeometry, sure, lets leave that (although I do revert/recompile often). SidePanel x2 -- I feel the same.

Unused variables: I don't mean assigning in an expression :-) But rather wonder should I do some similar code clean up along the way. Example attached (remove unneeded variables, positive conditions, don't add wrong values). Also was considering "const"-ing, correcting case etc., I guess I'll just correct whatever comes in the way -- let me know if I start to overdo that.

CUIDraw: Considering that (if it could at all happen ;-) there is an uninitialized access just after that default statement, are you absolutely sure that this message would be logged before the segmentation fault? Due to the possibility of compiler reordering that's not so obvious to me (although probably yes in this case, but with a bit of luck):





If you change cout to cerr or add an endl, you are going to see that message, but hopefully that shows the danger. You might want to consider throw-ing that error in such a low-level case (this is done in similar places already).

OK, that's purely academic, sorry for wasting your time, and of course do as you please :-)

I've noticed that expand buttons don't fit in InfoPanels if the font is made smaller, here's a fix (makes the population/resource/military panels at least as high as the button).

Unless you recompile everything, including unchanged files, that shouldn't matter, since those files aren't changing much...


The presence of the unneeded variables don't really hurt anything, and arguably makes the code easier to read since one doesn't need to look up functions like GetHullType to know what they're returning, but I don't really care either way if those are or aren't temporarily stored then discarded after the if checking the return value.

Positive vs. negative conditions doesn't really matter... I don't think it's worth going through just to change that just out of preference for positive. I usually use whichever of positive or negative works, and in these cases I was only outputting error text when the part or hull wasn't found, and was adding the passed-in name in either case, so the negative condition worked best.

Not adding the names of nonexisting parts or hulls to empires is changing how the code works, so isn't really cleanup anymore. When things are working, it shouldn't make a difference, but it might be helpful when debugging to see what part or hull name was actually added to an empire, without needing to have working log output, in case that name was corrupted or somesuch.


These changes are fine. I often get nitpicky when rewriting some code.


OK, I moved the default case above the SHAPE_RIGHT case, without a break between them, so the default is the same as SHAPE_RIGHT and things should get initialized fine.


Committed.

Good work. Slightly related to this, would it be possible to not decrease the meter icons below 16x16px(about the size they are when using a font size of 12)?

You mean they should scale up, but not down, like this:


?