Connect button in connect dialogue doesn't connect to the expected server

[dingodoppelt]

Describe the bug

In the connect dialogue we can connect to a server either by double clicking or single clicking and hitting the connect button.
When there is a custom server address in the combo box the connect button will only connect to that server, it if was either changed or the combo box hasn't lost focus (the user didn't click anywhere).

To Reproduce

1. Open connect dialogue
2. Have a server address in the combo box
3. Single-Click on any server in the serverlist
4. Single-Click on the combo box and notice that it has focus again
5. Hit the connect button
6. Find yourself on the server you clicked in the serverlist instead of the one from the combo box

Expected behavior

Giving focus to the combo box should make the connect button connect to that combo box item as it corresponds to the last selected item that the user probably set on purpose.

Operating system
All

Version of Jamulus
All

[dingodoppelt]

The issue here is that only in CConnectDlg::OnConnectClicked() we evaluate the current state.

// get the IP address to be used according to the following definitions:
// - if the list has focus and a line is selected, use this line
// - if the list has no focus, use the current combo box text

The above sounds sane but actually doesn't happen for we check only if ( CurSelListItemList.count() > 0 ) which is never 0 once we clicked on any server in the list.

[dingodoppelt]

Since b03c422 we have an eventFilter and I think this might solve our issues here, too. We need to capture the focus change and either set lvwServers->selectedItems() to 0 or introduce a variable with the server address to connect to so the existing comment actually becomes true.
I'd favour the latter solution because lvwServers->selectedItems() seems like the wrong choice. It is either 0 or 1 and doesn't reflect which field has focus because when the user clicks the button, the button has focus and it doesn't make sense to rely on the current state.

[softins]

I've just tried this in 3.12.3 on my PC, and can confirm the behaviour described.

Thinking about it as I did so, I think it would actually be logical to deselect any selected item in the list when giving focus to the combo box. I can't think of a reason to keep a server in the list selected, as the user would click on their desired server when giving focus back to the server list.

In fact, this already happens when the combo box contents are actually changed. This fires CConnectDlg::OnServerAddrEditTextChanged(), which in turn calls lvwServers->clearSelection();. We just need to do it when the combo box gains focus. There would then be no need to do it on a change, as it would already be in focus.

So, rename OnServerAddrEditTextChanged to OnServerAddrEditFocus, and change the connect:

QObject::connect ( cbxServerAddr, &QComboBox::editTextChanged, this, &CConnectDlg::OnServerAddrEditTextChanged );

to be:

QObject::connect ( cbxServerAddr, &QComboBox::focusInEvent, this, &CConnectDlg::OnServerAddrEditFocus );

The only thing I can think of that we would need to consider is accessibility. A user on the server list, who then tabs through all the fields and gets back to the list would no longer be on the server they might have previously selected, due to the focus having passed through the combo box. Currently, they would be.

I'm just about to make the above change locally and try it out.

[softins]

Ok, it doesn't work, because focusInEvent isn't actually a signal, and I didn't appreciate the difference.

We would need to install an event filter (currently, only the client settings dialog has one), and watch for the FocusIn event.

lvwServers->clearSelection(); could be called directly from that handler if we agree it's the behaviour we want.

[dingodoppelt]

Ok, it doesn't work, because focusInEvent isn't actually a signal, and I didn't appreciate the difference.

We would need to install an event filter (currently, only the client settings dialog has one), and watch for the FocusIn event.

lvwServers->clearSelection(); could be called directly from that handler if we agree it's the behaviour we want.

Yes, that's why I mentioned eventFilter in b03c422. We might take that as a template to handle stuff like this. We need this mechanism in multiple places where settings changed by for example JSON-RPC need to be reflected in the UI

[softins]

OK, I've tried it with an event filter:

bool CConnectDlg::eventFilter(QObject* obj, QEvent* event)
{
    if (obj == cbxServerAddr && event->type() == QEvent::FocusIn)
    {
        // remove selection in the server list (if any)
        lvwServers->clearSelection();
    }

    return QDialog::eventFilter(obj, event);
}

and it works as expected. Clicking on the server address combo box immediately clears the selection in the server list. And Connect therefore then connects to the expected server in the combo box.

The only thing I can think of that we would need to consider is accessibility. A user on the server list, who then tabs through all the fields and gets back to the list would no longer be on the server they might have previously selected, due to the focus having passed through the combo box. Currently, they would be.

I don't think this will be an issue. While in the server list, Tab moves through the servers one by one, not immediately to the next widget. So the user needs to tab through all the servers before they can tab to the server address combo box.

[dingodoppelt]

The only thing I can think of that we would need to consider is accessibility. A user on the server list, who then tabs through all the fields and gets back to the list would no longer be on the server they might have previously selected, due to the focus having passed through the combo box. Currently, they would be.

I don't think this will be an issue. While in the server list, Tab moves through the servers one by one, not immediately to the next widget. So the user needs to tab through all the servers before they can tab to the server address combo box.

Maybe @chigkim and @OnjLouis have something to add here in terms of accessibility.

[OnjLouis]

I don't think this will be an issue. While in the server list, Tab moves through the servers one by one, not immediately to the next widget. So the user needs to tab through all the servers before they can tab to the server address combo box.

As a screen-reader user myself I've always found that a bit unusual, speed-wise. Up/down arrow go through the server list, but then so does tab. This means it's hard to quickly jump to genre list without deploying screen-reader-specific keys. Ideally up/down stays as showing server list, enter default-connects to selected server and when tabbing to the server url specifically, enter would take that current focus and connect to the again, highlighted server.

Pressing Enter on the Genre field or any place that isn't strictly server-related should do nothing at all.
I must admit I do find it slightly difficult to get back to somewhere sane if I'm far down in the server list and want to perhaps change to a different category, or even connect to a server in my list.
It's certainly not a showstopper or anything of the sort, it's just one of those quirky things about Jamulus that makes it feel like it differs from most other apps.

[softins]

I don't think this will be an issue. While in the server list, Tab moves through the servers one by one, not immediately to the next widget. So the user needs to tab through all the servers before they can tab to the server address combo box.

As a screen-reader user myself I've always found that a bit unusual, speed-wise. Up/down arrow go through the server list, but then so does tab. This means it's hard to quickly jump to genre list without deploying screen-reader-specific keys. Ideally up/down stays as showing server list, enter default-connects to selected server and when tabbing to the server url specifically, enter would take that current focus and connect to the again, highlighted server.

Yes, that does seem unusual. As a separate issue/PR, it would be good to normalize that a bit, so that Tab would jump out of the server list to the next widget.

@dingodoppelt I have a branch that implements the event filter and so fixes the current issue. Shall I post it as a PR, or do you have other relevant work in progress?

[dingodoppelt]

@dingodoppelt I have a branch that implements the event filter and so fixes the current issue. Shall I post it as a PR, or do you have other relevant work in progress?

No, I just had a first look and have nothing written down but if I had, it'd be similar to your solution so please go ahead and open the PR :)