Page MenuHomeWildfire Games

Rename CProgressBar caption to progress
ClosedPublic

Authored by elexis on Nov 12 2019, 5:21 PM.

Details

Summary

It is irritating to the reader that "caption" is a keyword used for strings, but in CProgressBar "caption" refers to the numeric progress value, i.e. throws an error if receiving a string.

This also stands in contrast to the IGUIObject inheritance pattern where the same GUI setting type is present in GUI objects of different types yet having the same logic (enabled, hidden, size, ...).

We can find evidence even in this file that I'm not the only one:

// TODO Gee: (2004-09-01) Is this really updated each time it should?

The comment was copied from another place where "caption" was GUIM_SETTINGS_UPDATED in rP1232 / rP1237 / rP2116. Despite the later commit, it seems like a copy since it should be quite obvious from the code that it's updated correctly, but not so obvious for the CText caption (also the TODO dates back to a month prior to the commit date).

So the answer to the question is "yes", it is and it always has been (since it only depends on the cached size and the progress value and those have never been copies of states or dependent states that could have become out of sync unlike CInput).

Test Plan

Experience the irritation and notice that only these two GUI objects have the progressbar type.
Notice that the CRect could be cached, but then it has to be updated whenever m_CachedActualSize updates, which would be/will be messy.

Notice that one might argue that the entire class is duplicate with two instances of the CImage class, on the other hand it doesn't seem sufficient to delete this specialization, since the implementation isn't wrong nor that much duplication that it would be much effort to maintain in comparison to that being a JS implementation.

Diff Detail

Repository
rP 0 A.D. Public Repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

elexis created this revision.Nov 12 2019, 5:21 PM
elexis edited the test plan for this revision. (Show Details)Nov 12 2019, 5:23 PM

Successful build - Chance fights ever on the side of the prudent.

Link to build: https://jenkins.wildfiregames.com/job/vs2015-differential/566/display/redirect

Successful build - Chance fights ever on the side of the prudent.

Linter detected issues:
Executing section Source...

source/gui/ObjectTypes/CProgressBar.h
|  27| class·CProgressBar·:·public·IGUIObject
|    | [MAJOR] CPPCheckBear (syntaxError):
|    | Code 'classCProgressBar:' is invalid C code. Use --std or --language to configure the language.
Executing section JS...
Executing section cli...

Link to build: https://jenkins.wildfiregames.com/job/docker-differential/1080/display/redirect

elexis retitled this revision from CProgressBar irritation removal to Rename CProgressBar caption to progress.Nov 12 2019, 5:31 PM
elexis edited the summary of this revision. (Show Details)Nov 12 2019, 5:45 PM
This revision was not accepted when it landed; it landed in state Needs Review.Nov 12 2019, 5:53 PM
This revision was automatically updated to reflect the committed changes.
Owners added a subscriber: Restricted Owners Package.Nov 12 2019, 5:53 PM