Project

Profile

Help

HostedRedmine.com has moved to the Planio platform. All logins and passwords remained the same. All users will be able to login and use Redmine just as before. Read more...

Feature #901393

Qt: Add the city's size to the Info Panel of the City dialog.

Added by Marko Lindqvist about 2 months ago. Updated about 1 month ago.

Status:
Closed
Priority:
Normal
Category:
gui-qt
Sprint/Milestone:
Start date:
Due date:
% Done:

0%

Estimated time:

Description

Feature #872815 has a new patch from John Robertson to implement city's size display on city dialog of Qt-client.

History

#1 Updated by Marko Lindqvist about 2 months ago

- INFO_POPULATION is confusing. City population (at least 10000 people) as shown in the city dialog is different thing from the city size.
- Do not have 'default:' -case for the switch here. We don't want it to silence compiler warning about missing handling of an enum value here if someone adds a new one

#2 Updated by John Robertson about 2 months ago

· Got it, I will change it to INFO_CITY_SIZE or maybe INFO_CITIZENS, whichever you might prefer.
· On the second point, ... the `default:` case is intended to catch the missing label for an added INFO_xxx enum. Is a soft warning or a hard exit preferred?

#3 Updated by Marko Lindqvist about 2 months ago

John Robertson wrote:

· On the second point, ... the `default:` case is intended to catch the missing label for an added INFO_xxx enum. Is a soft warning or a hard exit preferred?

Catching it already at compile time (as an hard error with --enable-debug as is normal in development time) is preferred over maybe running on it on run time.

#4 Updated by John Robertson about 2 months ago

The problem is that the list of labels is built at run-time, not compile time; both in the unchanged current code and the proposed patch. Thus I don't think that there can be a compile time check to assert that the number of elements match between the enum and the label list.

Additionally, the runtime building of the label list in the proposed patch is to ensure that the labels are in the same order as the enums for subsequent algorithmic convenience. The current code does not have an order guarantee.

#5 Updated by Marko Lindqvist about 2 months ago

In addition to removing the 'default' case, you need to have 'i' to have correct enum type instead of being an int. Then the compiler will see if there's no 'case' for some value of 'i'. That's how all the other enum 'switch' cases in freeciv code work, and what CodingStyle document tells you to do.

#6 Updated by John Robertson about 2 months ago

I like this version much better, .... thanks for the guidance.

#7 Updated by Marko Lindqvist about 2 months ago

A bit modified version:
- Fixed trailing whitespaces git was warning about (they probably were not caused by this patch, but just were on the lines touched)
- Keep original field names instead of translated ones in specenum, translate only when calling city_info_name()
- Dropped '+' sign from the number of specialists. It's not an addition to city size, but part of it (at first I thought that the number would indicate city growth)

Can you provide versions for the stable branches (this does not apply)?

#9 Updated by Marko Lindqvist about 2 months ago

  • Status changed from In Progress to Resolved
  • Assignee set to Marko Lindqvist

#10 Updated by John Robertson about 2 months ago

I get various toolset deprecation errors in the S2_6_2 build. E.g.:
· `QT_DEPRECATED_X("Use default constructor instead") Q_DECL_CONSTEXPR inline QFlags(Zero) noexcept : i(0) {}`
· ` error: 'int QFontMetrics::width(const QString&, int) const' is deprecated: Use QFontMetrics::horizontalAdvance [-Werror=deprecated-declarations]`

I can work through an independent patch of these toolset updates, then produce an unencumbered city_panel patch, ...

or just abandon the city_panel patch for S2_6_2.

What would you recommend?

#11 Updated by Marko Lindqvist about 2 months ago

Your S3_0 patch applied to S2_6 as is.

Qt deprecation warnings are Bug #896335.

#12 Updated by John Robertson about 2 months ago

Great, I won't worry about it then.

#13 Updated by Marko Lindqvist about 1 month ago

John Robertson wrote:

or just abandon the city_panel patch for S2_6_2.

For the record: S2_6_2 was a branch for emergency 2.6.2.1 release. It won't continue from there, unless we have to do another emergency release before 2.6.3 is ready, and even then only those critical fixes would be cherry-picked to it.
S2_6 is the freeciv-2.6 branch.

#14 Updated by Marko Lindqvist about 1 month ago

  • Status changed from Resolved to Closed

Also available in: Atom PDF