Skip to content

Improve focus handling in Connect dialog #3859

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.

Sign up for GitHub

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

Open
softins wants to merge 2 commits into
base: main
Choose a base branch
from
+16 −10
Open

Improve focus handling in Connect dialog #3859

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5624cf2
Deselect server in list when clicking on address box
softins Aug 3, 2026
fc0cf14
No longer need handler for server addr edit text changed
softins Aug 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 14 additions & 9 deletions src/connectdlg.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,9 @@ CConnectDlg::CConnectDlg ( CClient* pNCliP, CClientSettings* pNSetP, const bool
cbxServerAddr->setMaxCount ( MAX_NUM_SERVER_ADDR_ITEMS );
cbxServerAddr->setInsertPolicy ( QComboBox::NoInsert );

// install event filter to catch FocusIn
cbxServerAddr->installEventFilter ( this );

// set up list view for connected clients (note that the last column size
// must not be specified since this column takes all the remaining space)
#ifdef ANDROID
Expand Down Expand Up @@ -276,8 +279,6 @@ CConnectDlg::CConnectDlg ( CClient* pNCliP, CClientSettings* pNSetP, const bool
QObject::connect ( edtFilter, &QLineEdit::textEdited, this, &CConnectDlg::OnFilterTextEdited );

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

QObject::connect ( cbxDirectory, static_cast<void ( QComboBox::* ) ( int )> ( &QComboBox::activated ), this, &CConnectDlg::OnDirectoryChanged );

// check boxes
Expand Down Expand Up @@ -661,13 +662,6 @@ void CConnectDlg::OnServerListItemDoubleClicked ( QTreeWidgetItem* Item, int )
}
}

void CConnectDlg::OnServerAddrEditTextChanged ( const QString& )
{
// in the server address combo box, a text was changed, remove selection
// in the server list (if any)
lvwServers->clearSelection();
}

void CConnectDlg::OnCustomDirectoriesChanged()
{

Expand Down Expand Up @@ -1195,3 +1189,14 @@ void CConnectDlg::OnCurrentServerItemChanged ( QTreeWidgetItem* current, QTreeWi
QAccessible::updateAccessibility ( new QAccessibleAnnouncementEvent ( lvwServers, announcement ) );
#endif
}

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

@pljones pljones Aug 3, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't actually like this.

The problem is that hitting connect when focus was in the Server text box should use that value.

Moving focus from the server list to any other field should have no effect on the selected server -- at that point, you have no idea of the user intent.

Why should moving to one field (Server) behave differently from another (Filter)?

@softins softins Aug 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem is that hitting connect when focus was in the Server text box should use that value.

Exactly. That's what @dingodoppelt was saying in #3857, and what does not always happen currently.

Moving focus from the server list to any other field should have no effect on the selected server -- at that point, you have no idea of the user intent.

Except that if the field they have clicked on is the Server Address combo box, it's reasonable to assume that it contains the server they want to connect to.

Why should moving to one field (Server) behave differently from another (Filter)?

Because by moving to the Server Address field, the user is desiring to use its value. The connect button won't use that value if there is still a server selected in the list box.

Currently, the only ways to deselect the selected server in the server list are either to go to a different directory, or to edit or change the value in the Server Address field. Just selecting the Server Address field when it already contains the desired value from before does not currently allow its value to be used, if there is still a server selected in the list. This PR fixes that by interpreting putting focus on the Server Address field as intent that its value should be used when connecting, and therefore deselecting any server selected in the list.

If you need convincing, try this version out and see!

@pljones pljones Aug 3, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be far better just to check focus correctly, rather than changing the natural behaviour of a widget (OK - whichever, it's changing Qt's implementation).

Being able to tab from the server list into the Server field would make more sense than this -- and that action shouldn't change the item selected. If you carry on tabbing back to the server list, the same server should be selected.

The change proposed here would break that. It's prioritising mouse navigation over keyboard navigation.

@softins softins Aug 3, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be far better just to check focus correctly, rather than changing the natural behaviour of a widget (OK - whichever, it's changing Qt's implementation).

The problem is, by the time the Connect button click is being processed, I believe that button has focus, and you have lost the information about which widget had focus previously.

It's not "changing QT's implementation", it's making the perfectly reasonable UX design decision that the user, by choosing to select the Server Address box, is implying that that is now what they are interested in, and are no longer interested in the server they had previously clicked on in the list, so it can be deselected. That is already the behaviour if the user changes what is in the Server Address box. But they might not want to, if it already contains the desired server.

Being able to tab from the server list into the Server field would make more sense than this -- and that action shouldn't change the item selected. If you carry on tabbing back to the server list, the same server should be selected.

The Tab behaviour is a separate issue that warrants investigation and its own PR. Currently Tab moves between servers in the list instead of moving from the server list to the combo box.

The change proposed here would break that. It's prioritising mouse navigation over keyboard navigation.

No, the keyboard navigation is already broken. This PR doesn't change that, it just addresses and improves a specific shortcoming of the current UX design.

}

return QDialog::eventFilter ( obj, event );
}
3 changes: 2 additions & 1 deletion src/connectdlg.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,8 @@ class CConnectDlg : public CBaseDlg, private Ui_CConnectDlgBase
void EmitCLServerListPingMes ( const CHostAddress& haServerAddress, const bool bNeedVersion );
void UpdateDirectoryComboBox();

bool eventFilter ( QObject* obj, QEvent* event );

CClient* pClient;
CClientSettings* pSettings;

Expand All @@ -145,7 +147,6 @@ class CConnectDlg : public CBaseDlg, private Ui_CConnectDlgBase

public slots:
void OnServerListItemDoubleClicked ( QTreeWidgetItem* Item, int );
void OnServerAddrEditTextChanged ( const QString& );
void OnDirectoryChanged ( int iTypeIdx );
void OnFilterTextEdited ( const QString& ) { UpdateListFilter(); }
void OnExpandAllStateChanged ( int value ) { ShowAllMusicians ( value == Qt::Checked ); }
Expand Down
Loading