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

add class WindowPopup, enabling easy window opening around cursor's position #4665

Closed
DorpsGek opened this issue Jun 28, 2011 · 7 comments
Closed
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

blup opened the ticket and wrote:

Simply add the ability of windows to be opened under the cursor. It also modify the WindowDesc structure of the proximity station selection list to use it. Works with both trunk (r22610) and 1.1.1.

Attachments

Reported version: trunk
Operating system: All


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

DorpsGek commented Jul 2, 2011

Alberth wrote:

+ case WDP_CURSOR:
+ if ( _cursor.pos.x < default_width / 2 )
+ pt.x = 0;
+ else if (_cursor.pos.x + default_width / 2 > _screen.width )
+ pt.x = _screen.width - default_width;
+ else
+ pt.x = _cursor.pos.x - default_width / 2;

The code style forbids such multi-line statements.
See also http://wiki.openttd.org/Coding_style

An if is either a single line

if (...) .... ;

or multiple lines with curly braces, like for example

if (....) {
....
} else {
....
}

In this case, the if can be written as a Clamp function call (by Rubidium):

pt.x = Clamp(0, _screen.width - default_width,_cursor.pos.x - default_width / 2);


This comment was imported from FlySpray: https://bugs.openttd.org/task/4665#comment10039

@DorpsGek
Copy link
Member Author

DorpsGek commented Jul 3, 2011

blup wrote:

Done. I did my best to respect all your conventions. Also modified the bridge building window to use it. Patch works for both trunk and 1.1.1.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/4665#comment10045

@DorpsGek
Copy link
Member Author

DorpsGek commented Jul 3, 2011

blup wrote:

small modifications to previous patch (comments & coding style).

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/4665#comment10046

@DorpsGek
Copy link
Member Author

DorpsGek commented Jul 4, 2011

Alberth wrote:

Nice step. More code, thus lots of new things that can go wrong :p
I wrecked a copy of your patch by removing good code, and adding my comments.

The idea is good, the code and the style needs work.

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/4665#comment10048

@DorpsGek
Copy link
Member Author

DorpsGek commented Jul 6, 2011

blup wrote:

Interesting approach. I was surprised at first, but it seems to be a good one.
Extending Window into WindowPopup seemed to be a good way to do it since I did not wanted to hacking directly into Window and intercept the execution flow at some weird place. Since the only thing that was required to be done was to implement a generic OnInitialValue(), this approach was looking to be the most promising.

Any particular reason why you don't put these constants in the constructor? That saves 3 lines of code in both windows. The "-5" values seem standard, so you could use them as default values.
Simply because my task was not to decide what is default and what is not. Since you ask me to set these value as default, I modified the code to reflect your decision.

Did you try to compile without adding the '# include "window_gui.h"' line?
I declared my class and its enum for the types in window_gui.h. If you don't want to include "window_gui.h" into window.cpp, I could change WindowPopup::type and WindowPopup::WindowPopup(t) types to unsigned integer.

enums start at 0 (which you do not need to specify.
Personal coding habits. Normally my enumerated list types values are between 1 and ENUM_TYPE_END.

We don't use setters and getters, instead we make variables simply public.
Removed, changed modifier_{x|y} names to wpu_mod_{x|y} and made them protected.

Why don't you use a switch statement here? Seems easier.
You were right, changed. I'm just rarely using switch statements.

Fixed a few comments and space/tab stuff. Also fixed the algorithm so windows align perfectly with right screen edges (it was not the case with the right edge.)

Attachments


This comment was imported from FlySpray: https://bugs.openttd.org/task/4665#comment10050

@DorpsGek
Copy link
Member Author

DorpsGek commented Jul 7, 2011

Alberth wrote:

For the next time, please give patches a unique name, so it is easier to state
what patch is being discussed.

These comments is about the patch 'WindowPopup-trunk.diff, posted at Wednesday July 6 in fs# 4665.

Compared to my last reviwed version not many improvements :(
Stuff to do:

  1. Remove 'public', 'protected', and 'private' lines from the struct. They
    serve no purpose.

  2. Make the data settable from the constructor, using the same names as the
    instance variable names in the formal parameter list.

  3. enums start at 0 in OpenTTD.

  4. There is no need to include "window_gui.h" in window.cpp, it is already available.

  5. There is no need to include in window.cpp for two reasons:
    - Its purpose is to detect programming errors. Rather than hiding them, we
    prefer a crash in that case.
    - The constant is already available as UINT_MAX.

  6. If WPUT_WIDGET_RELATIVE mode is selected, simply assume you get sane values,
    don't test for sanity. If the programmer doesn't, it will crash on the first test.

  7. Somewhat in the same area, providing a default in a switch is done with
    "default: NOT_REACHED();" so it crashes.

  8. There should be an empty line between the alternatives in a switch.

  9. White space in front of the code in WindowPopup::OnInitialPosition() is a MESS.
    Fix it!
    (Download the patch, and do a global search/replace of space by some
    character to see it, as your editor seems to be too broken.)

  10. Using sm_width cannot be correct. It would only work for windows that are
    not horizontally enlarged by a bigger desc->default_width.
    Perhaps there is another bug lurking in there?

Other suggestions:

A. Instead of 'wpu' or 'wpu_mod', you could use 'popup' in the names of the variables.

B. Also use a prefix to 'type', this name is too generic.

C. -5 to define a position inside a widget is not obvious, reverse the sign.

D. The Doxygen comments for OnInitialPosition are not needed, as it is an
overridden method.


This comment was imported from FlySpray: https://bugs.openttd.org/task/4665#comment10054

@DorpsGek
Copy link
Member Author

andythenorth closed the ticket.

Reason for closing: Won't implement

Mass closure of patch tickets with no commentary for >5 years. Goal is to reduce patch queue as an experiment to see if it aids faster reviewing and rejection/acceptance (it may not). If this offends you and the patch is maintained and compiles with current trunk, discuss with andythenorth in irc. (andythenorth has no ability to review patches but can re-open tickets).


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

@DorpsGek DorpsGek added component: interface This is an interface issue flyspray This issue is imported from FlySpray (https://bugs.openttd.org/) wontfix patch from FlySpray This issue is in fact a Patch, but imported from FlySrpay labels Apr 7, 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