Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enhanced financed window #104

Closed
DorpsGek opened this issue Apr 7, 2006 · 29 comments
Closed

Enhanced financed window #104

DorpsGek opened this issue Apr 7, 2006 · 29 comments
Labels
component: interface This is an interface issue flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) patch from FlySpray This issue is in fact a Patch, but imported from FlySrpay

Comments

@DorpsGek
Copy link
Member

DorpsGek commented Apr 7, 2006

Zr40 opened the ticket and wrote:

I've made a patch which reorders the finances window. It groups together recurring expenses, income and others, and shows a total for each group.
Comments are welcome!

Attachments

Reported version: trunk
Operating system: All


This issue was imported from FlySpray: https://bugs.openttd.org/task/104
@DorpsGek
Copy link
Member Author

DorpsGek commented Apr 7, 2006

Zr40 wrote:

New patch. Changes:
- Patch made with patch, not with tortoisesvn.
- Made finance window wider.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/104#comment175

@DorpsGek
Copy link
Member Author

DorpsGek commented Apr 7, 2006

Zr40 wrote:

New patch. Changes:
- Moved income group to the top, switching places with the expenses group.


This comment was imported from FlySpray: https://bugs.openttd.org/task/104#comment176

@DorpsGek
Copy link
Member Author

DorpsGek commented Apr 7, 2006

Zr40 wrote:

Seems the patch wasn't attached.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/104#comment177

@DorpsGek
Copy link
Member Author

DorpsGek commented Apr 7, 2006

Zr40 wrote:

New patch. Changes:
- Moved 'other' to third group, as it includes local authority costs.
- Added comments describing which group is being drawn.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/104#comment178

@DorpsGek
Copy link
Member Author

Zr40 wrote:

Attached a screenshot of the enhanced finances window.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/104#comment237

@DorpsGek
Copy link
Member Author

DorpsGek commented May 4, 2006

precision wrote:

# There is something wrong with the running cost "Total" (and the final "Total") on the screenshot.

# Adding a 4th column which contains a sum for each line would be a good extention.


This comment was imported from FlySpray: https://bugs.openttd.org/task/104#comment253

@DorpsGek
Copy link
Member Author

Meush wrote:

I second precision's comment, and I'd like to see some colour difference between adjactent rows/groups to distinguish the values easily. On the other hand, it would be much easier to commit a smaller patch, and add eye-candies later.
Good job on this patch. Small and useful :)


This comment was imported from FlySpray: https://bugs.openttd.org/task/104#comment424

@DorpsGek
Copy link
Member Author

DorpsGek commented May 6, 2007

Zr40 wrote:

I've updated the patch to apply again. It applies against the current trunk: r9793. Changes:
- Fixed the problem precision mentioned by precision.
- Added comments explaining the various indexes.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/104#comment1200

@DorpsGek
Copy link
Member Author

DorpsGek commented May 6, 2007

Zr40 wrote:

...that should be 'Fixed the problem mentioned by precision.', of course.


This comment was imported from FlySpray: https://bugs.openttd.org/task/104#comment1201

@DorpsGek
Copy link
Member Author

DorpsGek commented May 6, 2007

TrueBrain wrote:

I did a quick code-wise check, and some comments:

- if (tally < 0) { tally = -tally; str++; } <- wrong, put it on 2 lines
- 'tally', wtf is that? Use a better name?
- DrawString(2, 27 + i10 + 140, STR_7011_CONSTRUCTION + i, 0); <- please use spacebar... 'i10' -> 'i * 10'
- don't use tabs in GUI defines; use spaces, like everywhere else

That were the 4 things I notice on the first glance. Sorry, but code-style is important (and a bitch :))


This comment was imported from FlySpray: https://bugs.openttd.org/task/104#comment1202

@DorpsGek
Copy link
Member Author

DorpsGek commented May 6, 2007

Zr40 wrote:

- player_gui.c currently contains those one-liners, which I've copied and pasted. I've changed them to two lines, and altered the already present lines as well.
- A tally is a counter. According to my dictionary: a current score or amount. I've renamed it to 'total'.
- Those missing spaces originate from the original patch, where back then, player_gui.c contained the same code style. I've noticed that has changed, but apparently I missed some instances.
- As for the tabs, I don't know how they ended up in there. They've been removed.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/104#comment1203

@DorpsGek
Copy link
Member Author

DorpsGek commented May 6, 2007

Zr40 wrote:

New patch. Changes:
- Fixed wrong displayed total.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/104#comment1204

@DorpsGek
Copy link
Member Author

Zr40 wrote:

New patch. Changes:
- Removed trailing tabs.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/104#comment1227

@DorpsGek
Copy link
Member Author

Zr40 wrote:

I was told on # openttd the patch needs a rewrite. However, it wasn't stated what exactly needs rewriting.

So... What needs to be done? :)


This comment was imported from FlySpray: https://bugs.openttd.org/task/104#comment1277

@DorpsGek
Copy link
Member Author

Zr40 wrote:

I've updated the patch to apply against r10230.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/104#comment1404

@DorpsGek
Copy link
Member Author

Zr40 wrote:

It seems patch (the program, not my patch) ate part of r6924 when I was working on -v5. Fixed.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/104#comment1405

@DorpsGek
Copy link
Member Author

Zr40 wrote:

I've updated the patch to incorporate changes in r10208-r10209. It now applies against r10380.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/104#comment1489

@DorpsGek
Copy link
Member Author

Zr40 wrote:

I've updated the patch with the following changes:
- More comments. Everybody wants comments :)
- Reorder header name drawing calls to follow screen order instead of index order.
- Reduce coordinate math where possible.

Patch applies against r10581.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/104#comment1608

@DorpsGek
Copy link
Member Author

Zr40 wrote:

Patch updated for changes in r10587.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/104#comment1631

@DorpsGek
Copy link
Member Author

Zr40 wrote:

Patch updated for changes in r10704.

Can this be committed, please?

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/104#comment1732

@DorpsGek
Copy link
Member Author

TrueBrain wrote:

Some questions:

  1. maybe you can make the numbers enums, so you don't have to add in comments what each number represent? Makes modifying things much easier
  2. the result I personally do not like too much, as it becomes rather hard to read... I dunno what exactly makes it so, as in fact it is useful to see it per category. Maybe make tabs for each category where you can review things in more detail (like: expected income at end of year)?

This comment was imported from FlySpray: https://bugs.openttd.org/task/104#comment1895

@DorpsGek
Copy link
Member Author

DorpsGek commented Sep 4, 2007

Zr40 wrote:

1: Agreed, but that doesn't completely solve the problem. You still have the loops, which are dependent on the order (which is probably not documented anywhere). Maybe unrolling them is the solution?
2a: Perhaps it's not really clear which section is what? If so, all that's needed is section names.
2b: I don't like the tab idea, and it's also not trivial to implement - all I did was some simple reshuffling :)
2c: Trains show profit for this year and last year. Do they show expected profit?

I've updated the patch to follow changes in recent commits. It applies against r11043.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/104#comment2058

@DorpsGek
Copy link
Member Author

DorpsGek commented Feb 7, 2008

Zr40 wrote:

I've updated the patch for changes since r11043. It applies against r12073.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/104#comment3463

@DorpsGek
Copy link
Member Author

DorpsGek commented Feb 7, 2008

Zr40 wrote:

I've removed most loops; loop bodies have been extracted into functions. I've also replaced magic numbers with enums where possible.

The patch applies against r12083.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/104#comment3469

@DorpsGek
Copy link
Member Author

DorpsGek commented Feb 7, 2008

Zr40 wrote:

Removed sums from draw functions; placed in main DrawPlayerEconomyStats function. As a result, the DrawPlayerEconomy{Sum,Total} functions are redundant and have been removed.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/104#comment3471

@DorpsGek
Copy link
Member Author

DorpsGek commented Feb 7, 2008

Zr40 wrote:

New patch. Changes:
- Widgetified labels.
- Total is now shown when zero.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/104#comment3472

@DorpsGek
Copy link
Member Author

Zr40 wrote:

Updated patch to apply against r15297.

It's been almost three years. What does it take to get this committed?

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/104#comment5466

@DorpsGek
Copy link
Member Author

Zr40 wrote:

Changes:
- Fixed copy/paste error due to applying the 2008 patch manually. (It didn't apply because of a year worth of changes in svn.)
- There's some magic somewhere. Moved the Borrow/Repay buttons to the correct position to accommodate the magic.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/104#comment5467

@DorpsGek
Copy link
Member Author

peter1138 closed the ticket.

Reason for closing: Implemented

In r15301


This comment was imported from FlySpray: https://bugs.openttd.org/task/104

@DorpsGek DorpsGek added component: interface This is an interface issue flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) patch from FlySpray This issue is in fact a Patch, but imported from FlySrpay labels Apr 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: interface This is an interface issue flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) patch from FlySpray This issue is in fact a Patch, but imported from FlySrpay
Projects
None yet
Development

No branches or pull requests

1 participant