Adding hotkey to go to notification event

[sjoblomj]

Pressing g will go to the position of the latest notification. If the notification window is open, pressing g will go to the position of the currently open notification.

Closes #1539.

[Flamefire]

Is the possible underflow for numMessages == 0 intended?

Maybe be explicit and do:

if(curMsgIdx < 0 || numMessages == 0)
    return nullptr;

Afterwards you can use static_cast<unsigned>(currMsgIdx) safely instead of the (C-style) cast to int.

[sjoblomj]

It is intended, since GetMsg checks that the index is within bounds and returns nullptr if not. But you're right that it's probably best to be explicit about it. 👍

[sjoblomj]

I added a check in a new commit to make sure the index is within bounds. Checking that currMsgIdx < 0 - as you suggest - does not make sense though, since currMsgIdx == -1 whenever the notification window is closed. In that case, we get the the most recent notification (numMessages - 1).

[Flamefire]

Although the change looks good to me I'm wondering why the existing code doesn't work:

s25client/libs/s25main/ingameWindows/iwPostWindow.cpp

Lines 251 to 253 in 35983cd

	case 'g': // Go to site of event
	Msg_ButtonClick(ID_GOTO);
	return true;

That should suffice. If it doesn't then I guess the window isn't active anymore so it doesn't get the events. In that case I'm wondering if we should even react on the key event.

From the issue:

In the old settlers [...] would click "n" to open the notification window, "enter" to go to the notification site, then "del" to remove the notification.

If 'g' doesn't work then 'del' wouldn't either and we'd need to fix that too. To handle those (kind of) window-specific events in the desktop is not really "clean" and might lead to issues when another window uses the same key for something. So we should really check if and why it currently does not work and rather fix that. If it is only that the window lost focus due to clicking somewhere else the workaround would simply be to press 'n' again (or click inside the window), but IIRC the window is supposed the get the event first before the desktop, but I'd need to check too.

[sjoblomj]

Although the change looks good to me I'm wondering why the existing code doesn't work:

s25client/libs/s25main/ingameWindows/iwPostWindow.cpp

Lines 251 to 253 in 35983cd

	case 'g': // Go to site of event
	Msg_ButtonClick(ID_GOTO);
	return true;

That should suffice. If it doesn't then I guess the window isn't active anymore so it doesn't get the events. In that case I'm wondering if we should even react on the key event.

From the issue:

In the old settlers [...] would click "n" to open the notification window, "enter" to go to the notification site, then "del" to remove the notification.

If 'g' doesn't work then 'del' wouldn't either and we'd need to fix that too. To handle those (kind of) window-specific events in the desktop is not really "clean" and might lead to issues when another window uses the same key for something. So we should really check if and why it currently does not work and rather fix that. If it is only that the window lost focus due to clicking somewhere else the workaround would simply be to press 'n' again (or click inside the window), but IIRC the window is supposed the get the event first before the desktop, but I'd need to check too.

Like you say, the notification window also has a hotkey for g which goes to the position of the currently open message. And like you say, the notification window must be focused for this to work; so if the player has clicked on the game interface or another window, this no longer works.

So, the bigger question would be if the game interface should also have this hotkey or not, regardless of whether the notification window is open, which is what this PR implements. I'm not too familiar with the code, but I suppose it could be hairy to 1) let unfocused windows receive key strokes first, and 2) make sure different unfocused windows don't have the same key binding for different things.

Regardless, the current behavior is a bit of an annoyance, where you'd think pressing g would take you to the position of the event, but it turns out the notification window has lost focus and nothing happens.

As for the question of the other keys: If the del key should also have a similar functionality as g in this PR, I'd argue that there should be checks to make sure the notification window is open (but possibly unfocused) - otherwise it seems like the player could delete messages by mistake, possibly without even realizing it.

[Flamefire]

So, the bigger question would be if the game interface should also have this hotkey or not, regardless of whether the notification window is open

I don't think it is a good idea to have that jump-to when the window is not open. It might seem quite random, especially as a newer message might have arrived so the jump position is not fully predictable. Of course that might be a feature: Jump to position of latest notification.

the notification window must be focused for this to work; so if the player has clicked on the game interface or another window, this no longer works.

In combination with the above I lean towards that this is supposed to work that way. Are people really having the post window open all the time while doing something else? Pressing "n" and then "g" should always work, so that would only be one additional key press.

Do we have any other windows with hotkeys that exist in the original S2? Then we could test how the behavior with respect to focused windows worked there. Maybe @Spikeone knows more or has suggestions.