#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)
Change History (20)
comment:1 by , 9 years ago
comment:2 by , 9 years ago
Keywords: | review patch added |
---|---|
Milestone: | Backlog → Alpha 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 , 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 , 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 , 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 , 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
.
follow-up: 7 comment:6 by , 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!!
comment:7 by , 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.
comment:8 by , 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
by , 9 years ago
Attachment: | Screenshot from 2015-07-17 12:14:30.png added |
---|
by , 9 years ago
Attachment: | Screenshot from 2015-07-17 12:24:41.png added |
---|
by , 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 , 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 , 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:11 by , 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!
I have done the first part (trade carts), the second part (looting) will follow soon.