juanjo wrote:

...

Attachments

• company.png (136.75 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/5379#comment11724

juanjo wrote:

Just redrawing the screen will solve it.

Attachments

• SharesOption-RedrawScreen.diff (0.31 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/5379#comment11818

Alberth wrote:

I had a quick look at your patch, but it does not convince me as the right solution. I think there are two problems with it:

- RedrawScreen is very blunt. You redraw everything, even though only two(?) small buttons are invalid. You even redraw everything when those buttons are not even at the screen.

- In about one year or longer in the future, nobody will understand what that RedrawScreen call is doing there. There is no way to retrieve what that call aims to fix from the source code. Someone will see it, think "he, that's strange, it must be a left-over from some ancient time" and remove it. Since nothing obvious breaks, and nothing indicates what has changed after removal, the devs at that time will assume nothing needed that call, and the bug will thus get re-introduced.

This comment was imported from FlySpray: https://bugs.openttd.org/task/5379#comment11846

juanjo wrote:

1. "RedrawScreen is very blunt."
   Other settings like gui.expenses_layout do the same: They do not add any comment and redraw the whole screen even if no expenses window is open. There are some other settings that are written exactly the same way. Anyway, a single line comment may be added, but it will difficult readability of settings.ini.

And you are right that the problem is about the two buttons buy/sell and not just the "buy shares" I noticed.

2. The commit message can explain why this is introduced. So, if somebody in the future is worried about why this was introduced, he/she could look the message of the commit.

This comment was imported from FlySpray: https://bugs.openttd.org/task/5379#comment11847

frosch wrote:

I think the problem is more that enabling/disabling buttons is something that should be done in OnInvalidateData(), and not like the company GUI currently does inside OnPoint().
So, normally the "correct" call would be to InvalidateWindowData() instead of RedrawScreen(). The latter only works because the company GUI is coded sloppily.

This comment was imported from FlySpray: https://bugs.openttd.org/task/5379#comment11850

juanjo wrote:

I have looked some *_gui.cpp files, and there is no unified style of what goes inside OnInvalidateData() and what goes inside OnPaint().

I guess company GUI must be rewritten before closing this bug.

This comment was imported from FlySpray: https://bugs.openttd.org/task/5379#comment11851

Alberth wrote:

OnPaint() in all derived window classes should be empty. The idea is that you should change or update state when the condition changes, instead of doing it as a side-effect of painting the window. As you have noticed, we haven't quite yet reached that point.

OnInvalidateData() depends very much on what messages get sent to the window. I added some doxygen comments about the meaning of the parameter values here and there, but it is not complete.
In general, you are free to add a case in it; the contents is not expected to be similar between different windows afaik.

This comment was imported from FlySpray: https://bugs.openttd.org/task/5379#comment11852

Rubidium closed the ticket.

Reason for closing: Fixed

In r24968

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