Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#3311 closed enhancement (fixed)

[PATCH] Show number of trade carts and looted resources in summary screen

Reported by: elexis Owned by: Itms
Priority: Nice to Have Milestone: Alpha 19
Component: UI & Simulation Keywords: simple patch
Cc: Patch:

Description

Currently the "Units" tab shows Infantry, Worker, Cavalry, Champion, Heroes and Navy. But having a good trade is essential for winning games with long duration, the number of trade carts should be added to this table. It would also show how many trade carts were killed.

Also the "Resources" tab should show the amount of loot that people got by killing trade carts and workers that carried resources.

Attachments (8)

ticket3311patch.doc (26.5 KB ) - added by bb 9 years ago.
the patch for the first part of this ticket
ticket3311patch.txt (6.6 KB ) - added by bb 9 years ago.
Here the patch as text-file (i don't know how to create a ".patch" file)
ticket3311.patch (26.2 KB ) - added by bb 9 years ago.
complete patch for this ticket
ticket3311.diff (11.0 KB ) - added by bb 9 years ago.
patch with using "-x --ignore-eol-style"
Screenshot from 2015-07-17 12:14:30.png (796.9 KB ) - added by bb 9 years ago.
Screenshot from 2015-07-17 12:24:41.png (804.3 KB ) - added by bb 9 years ago.
ticket3311_2.diff (11.8 KB ) - added by bb 9 years ago.
When looking at the other tabs, we see that the aligning is not consistant: at the market and score tab everything is on the upper row but at the misc tab "feminisation" is a row down. I think we should make this consistant. In my opinion we set "feminisation" a row higher and put "Loot" after the "Tributes". See the images in the attachments. I changed the other stuff and these in the new patch.
ticket3311_3.diff (12.8 KB ) - added by bb 9 years ago.
If I'am correct this patch changes the test correcly too. (The test worked also before this changes, so I thought it was alright)

Download all attachments as: .zip

Change History (20)

comment:1 by bb, 9 years ago

I have done the first part (trade carts), the second part (looting) will follow soon.

by bb, 9 years ago

Attachment: ticket3311patch.doc added

the patch for the first part of this ticket

comment:2 by bb, 9 years ago

Keywords: review patch added
Milestone: BacklogAlpha 19
Summary: Show number of trade carts and looted resources in summary screen[PATCH] Show number of trade carts and looted resources in summary screen

comment:3 by elexis, 9 years ago

Please upload the patches as a raw text file, they shouldn't be contained inside a ms word document. Thanks for working on this! :-)

by bb, 9 years ago

Attachment: ticket3311patch.txt added

Here the patch as text-file (i don't know how to create a ".patch" file)

comment:4 by Itms, 9 years ago

Hi :) You should use Tortoise SVN "Create patch" function (I assume you work on Windows). It automatically creates a .diff or a .patch file.

How did you generate the current patch?

comment:5 by elexis, 9 years ago

You can just rename the file to *.patch. If I'm not mistaken, svn diff > my.patch should work too.

In session.js and layout.js, LobbyRanking.py there are two lines which have too much whitespace, please align them. A newline in XpartaMuPP.py wouldn't hurt.

The loot you receive when killing enemy units (including traders) is available in binaries/data/mods/public/simulation/components/Looter.js.

comment:6 by bb, 9 years ago

I work on linux (fedora) and created the patch by "svn diff" in the command line, then this output occurs. Copy-pasting makes the file. There should be a command for making a patch, but can't find that on the internet.

I have finished the looting too, a complete patch will be uploaded next. The patch is made ith gedit, but still a copy from the command line output.

thx for helping itms and elexis!!

by bb, 9 years ago

Attachment: ticket3311.patch added

complete patch for this ticket

in reply to:  6 comment:7 by elexis, 9 years ago

You can simply use svn diff > filename to save the diff to a file.

The current patch contains many changes where the actual lines didn't change (presumably only the line ending has changed). Check svn diff --help to see some options. You could try -x -w, -x -w --ignore-eol-style or -x --ignore-eol-style to produce a patch where different line endings / whitespace are ignored.

by bb, 9 years ago

Attachment: ticket3311.diff added

patch with using "-x --ignore-eol-style"

comment:8 by elexis, 9 years ago

You can remove the braces in StatisticsTracker and use let instead of var whenever possible:

for (let type in amount) 
	this.lootCollected += amount[type]; 

Maybe the caption should be changed to "Loot" instead of "Looted" and lower the yStart of that element too, to align it with the other captions that have only one row (but I'm not sure about that).

Besides that, the patch looks good and ready to be committed in my opinion, well done!

https://i.imgur.com/d3jfonc.png https://i.imgur.com/d9HLSaK.png

Last edited 9 years ago by elexis (previous) (diff)

by bb, 9 years ago

Attachment: ticket3311_2.diff added

When looking at the other tabs, we see that the aligning is not consistant: at the market and score tab everything is on the upper row but at the misc tab "feminisation" is a row down. I think we should make this consistant. In my opinion we set "feminisation" a row higher and put "Loot" after the "Tributes". See the images in the attachments. I changed the other stuff and these in the new patch.

comment:9 by Itms, 9 years ago

Keywords: review removed

This patch looks really good. Before committing it it would be nice to fix some little things:

  • Line 49 of Looter.js, you have tabulations that you should remove
  • Add a new line after the new StatisticsTracker function

More importantly, I think you are breaking the GUIInterface tests by modifying the statistics tracking. You should run binaries/system/test and fix that. If you have questions don't hesitate to ask them :)

Thanks a lot for working on this and sorry for the long delay in reviewing.

by bb, 9 years ago

Attachment: ticket3311_3.diff added

If I'am correct this patch changes the test correcly too. (The test worked also before this changes, so I thought it was alright)

comment:10 by Itms, 9 years ago

Owner: set to Itms
Resolution: fixed
Status: newclosed

In 16932:

Show the number of trade carts and looted resources in the summary screen. Patch by bb, fixes #3311

comment:11 by Itms, 9 years ago

Yes indeed, that's why I wanted you to add. It wasn't a breaking oversight as I thought, but it's better to add it, to keep tests in sync with the code. :)

Thanks for this patch!

comment:12 by Itms, 9 years ago

In 16934:

Fixes the computation of team values for some of the summary counters, and reorder columns. Refs #3311

Note: See TracTickets for help on using tickets.