Zuu wrote:

Changed
QueryStringBaseWindow* qs = dynamic_cast<QueryStringBaseWindow*>(w);
to
QueryStringBaseWindow qs = dynamic_cast<QueryStringBaseWindow>(w);

Attachments

• unified_open_osk2.patch (9.01 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/2375#comment4931

Zuu wrote:

* Removed silly tab in a code line
* Changed (qs != 0) to (qs != NULL)
* Commented the osk_ok and osk_cancel parameters of the QueryStringBaseWindow constructor

To comment on the question how this conforms with a possible extension to having more edit boxes per window I'd like to say:
- When making support for multiple edit boxes per window I think we need to move storing of text and current carret position from QueryStringBaseWindow to a struct that somehow gets associated with the widget perhaps using some kind of pointer/index stored in the data member of widget. And when doing this the OSK ok/cancel parameters could be placed in this struct. But for now I don't want to fiddle with how to associate that struct with the widget.
- So while I think further on how things can fit with more edit boxes I also want to keep with the current aim to only expand to multiple edit box-windows open at a given time.

Also, as you might have seen, while this patch itself may not make a huge difference in Window & Widget Focus patch I make use of the changes here to control the opening of OSK window to only happen when you click on an already focused widget. The alternative that I could come up with would be to change the virtual OnClick function to also take a parameter that tells each window if widget focus was changed. That is a huge number of windows to change. On top instead of handling it transparent to window code, each window will need to take care to only show the OSK window when clicking on an already focused edit box. (could of course be done in the ShowOnScreenKeyboard function by passing the widget_focus_changed parameter, but still I did prefer the by the patch chosen solution more)

Attachments

• unified_open_osk3.patch (9.27 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/2375#comment4937

Zuu wrote:

Changed to by frosch123 suggested approach:

Instead of storing osk_ok/osk_cancel in QueryStringBaseWindow, the OpenOSKWindow member function of QueryStringBaseWindow pases zero as ok and cancel widget index and for windows that want to send non-zero they have to overload the OpenOSKWindow function. Also the OpenOSKWindow function has been renamed to OnOpenOSKWindow and of course been made virtual.

I like it this way better as the ok/cancel parameters are more clearly marked that they belong to the OSK window than before. This solution is also flexible enough so that with multiple edit boxes the window could add a switch case statement in OnOpenOSKWindow to pass different arguments to ShowOnScreenKeyboard function if needed.

Attachments

• unified_open_osk4.patch (6.53 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/2375#comment4938

Zuu wrote:

Updated to trunk. Still contains the questioned dynamic_cast.

Attachments

• unified_open_osk_6.patch (6.65 KiB)

This comment was imported from FlySpray: https://bugs.openttd.org/task/2375#comment4952

Rubidium wrote:

What is the use of osk_ok/osk_cancel in QueryStringBaseWindow? It doesn't seem to be used at all.

Furthermore using dynamic_cast is fine.

This comment was imported from FlySpray: https://bugs.openttd.org/task/2375#comment5182

Zuu wrote:

osk_ok/osk_cancel members of QueryStringBaseWindow holds the ok/cancel widget ids that are the third and fourth argument to ShowOnScreenKeyboard. Since windows such as QueryStringWindow don't call it themself anymore with this patch, these arguments need to be stored somewhere.

I should also say that I have received a suggestion to change the code so that the patch doesn't use the osk_ok/osk_cancel members. Instead the QueryStringBaseWindow would have a virtual function OnShowOSK or similar name that calls ShowOnScreenKeyboard with 0, 0 as third and fourth argument. Windows that want to use other argument than that will override the function with an own OnShowOSK function that calls ShowOnScreenKeyboard with the wanted arguments.

I like this suggestion, and after having not worked on this patch for a while I'm surprised I didn't implement that suggestion. Probably because I needed to spend all my time on my exams or so.

Nice to hear you think the dynamic_cast is fine, I got the response that it was unwelcome from some people, but couldn't find an easy way to remove it that kept the code clean.

This comment was imported from FlySpray: https://bugs.openttd.org/task/2375#comment5185

Rubidium wrote:

If it would be used, then yes... but osk_ok is used nowhere, just search the diff for osk_ok and you'll find it gets declared as instance variable and it does this->osk_ok = [this->]osk_ok (which is pointless). Same for osk_cancel.

This comment was imported from FlySpray: https://bugs.openttd.org/task/2375#comment5186

Zuu wrote:

Hmm, time to be ashamed. I've downloaded version 1 of my patch, not version 6. :-)

In version 6, the changes I was speaking about has been implemented. So osk_ok and osk_cancel is just a leftover from how it used to be. I will remove them.

This comment was imported from FlySpray: https://bugs.openttd.org/task/2375#comment5187

Rubidium closed the ticket.

Reason for closing: Implemented

In r14804, version 6 but without osk_ok and osk_cancel.

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