ripose-jp / Memento

An mpv-based video player for studying Japanese
https://ripose-jp.github.io/Memento/
GNU General Public License v2.0
472 stars 22 forks source link

Auto text edit field focus #156

Closed Calvin-Xu closed 1 year ago

Calvin-Xu commented 1 year ago

I have been using the Ctrl-F search functionality more lately and noticed that the text input focus does not automatically move to the text field. I've been using the following hack and it seems very ergonomic to me:

connect(
        m_findShortcut, &QShortcut::activated, this,
        [=] { 
          m_ui->widgetFind->setVisible(!m_ui->widgetFind->isVisible());
          if (m_ui->widgetFind->isVisible())
          {
              m_ui->widgetFind->findChild<QLineEdit*>("lineEditSearch")->setFocus();
          }
        },
        Qt::QueuedConnection
    );

I feel like there is probably a proper way to do it, and also wonder if this behavior can be adopted for all lookup shortcuts (i.e., Ctrl-R).

ripose-jp commented 1 year ago

I'm on board with the idea. My only criticism is you can do all of this

if (m_ui->widgetFind->isVisible())
{
    widgetFind->findChild<QLineEdit*>("lineEditSearch")->setFocus();
}

with this

m_ui->lineEditSearch->setFocus();

If you want to make a pull request, I'll merge it. If not I can do it myself.

Calvin-Xu commented 1 year ago

I'd certainly make a fixed PR. The only thing is whether you want the same thing to be done for SearchWidget, which I feel is also more complex after a bit of poking.

ripose-jp commented 1 year ago

Doing the same for SearchWidget is not any more complex than this one. I would say that if you did choose to include SearchWidget, make the change in a separate commit and move the implementation for showEvent into the cpp file.

ripose-jp commented 1 year ago

Better yet, you can just connect the widgetShown signal to setFocus and leave showEvent where it is.