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 #903720

Qt: added trade information columns to goto dialog

Added by John Robertson over 1 year ago. Updated 4 months ago.

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

0%

Estimated time:

Description

Please review and advise.


Related issues

Related to Freeciv - Feature #905523: Document that enum route_direction is part of network protocolClosed

<a title="Actions" class="icon-only icon-actions js-contextmenu" href="#">Actions</a>

History

#1 Updated by Marko Lindqvist over 1 year ago

  • Category set to gui-qt

CodingStyle:
"- When declaring a pointer, there should be a space before '*' and no space
after, except if it is a second '*'."

"- Braces shall always be used after conditionals, loops, etc.:"
while (foo) bar;
->
while (foo) {
bar;
}

In gotodlg.h includes are not in alphabetical order within the // common block

Within some 'case's indentation is off

#2 Updated by John Robertson over 1 year ago

If beneficial, here is a VisualStudio configuration file that matches the freeciv coding style guides. Place in the root source folder then open the folder in MSVC.

#3 Updated by John Robertson over 1 year ago

This one follows the coding standards and enhances the new Trade column. The patch is a little bigger because I needed a 'RDIR_NONE', or an empty or no direction for the trade enumerations.

(I'll start working on the S3_0 patch, ...)

#4 Updated by Marko Lindqvist over 1 year ago

John Robertson wrote:

I needed a 'RDIR_NONE'

That's a network protocol change (especially as the values of the existing enum entries change) ...

(I'll start working on the S3_0 patch, ...)

...so it might not be worth all the trouble of porting to S3_0 (where protocol has been frozen), with the need of network capabilities code and stuff.

#5 Updated by Marko Lindqvist over 1 year ago

  • Related to Feature #905523: Document that enum route_direction is part of network protocol added

#6 Updated by John Robertson over 1 year ago

I have moved RDIR_NONE to the end with a value of 3 instead of at the beginning of the enumeration with a value of zero. This should preserve the network protocol.

But my testing is getting a SEGSEGV calling the c++ delete for the QPixmap of the sprite. The pointer is valid, the destructor completes and spills over into the library free() call correctly. I probably need to recompile Qt5 for Msys2, assuming there is a problem in the default Msys package.

#7 Updated by John Robertson over 1 year ago

@Marko, I have a small related change that updates the trade calculations as the selected unit changes. Do you want me to roll it into an inclusive patch for master and S3_0 or an independent patch?

#8 Updated by Marko Lindqvist over 1 year ago

John Robertson wrote:

@Marko, I have a small related change that updates the trade calculations as the selected unit changes. Do you want me to roll it into an inclusive patch for master and S3_0 or an independent patch?

Independent patch

#9 Updated by Marko Lindqvist over 1 year ago

  • Sprint/Milestone set to 3.1.0

- There's still while() loops in one line instead of having proper code block
- Handling of that new RDIR_NONE in various switch:es need to be more than "break;" - e.g. sanity check module should fail if some traderoute actually uses it, many places should fc_assert() against it

#10 Updated by Marko Lindqvist 4 months ago

John Robertson wrote:

But my testing is getting a SEGSEGV calling the c++ delete for the QPixmap of the sprite. The pointer is valid, the destructor completes and spills over into the library free() call correctly. I probably need to recompile Qt5 for Msys2, assuming there is a problem in the default Msys package.

Ping. I'm going to test this myself anyway, but was this ever resolved?

#11 Updated by Marko Lindqvist 4 months ago

Made an updated patch myself.

Marko Lindqvist wrote:

- There's still while() loops in one line instead of having proper code block

These, and some other minor style issues I noticed, fixed

- Handling of that new RDIR_NONE in various switch:es need to be more than "break;" - e.g. sanity check module should fail if some traderoute actually uses it, many places should fc_assert() against it

Sanity check made to fail, fc_assert() added to one place.

I have not yet tested on msys2 to see if this has issue there.

#12 Updated by Marko Lindqvist 4 months ago

  • Status changed from Resolved to Closed

Also available in: Atom PDF