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

Add auto text edit field focus to subtitlelistwidget and searchwidget #157

Closed Calvin-Xu closed 1 year ago

Calvin-Xu commented 1 year ago

Closes https://github.com/ripose-jp/Memento/issues/156

ripose-jp commented 1 year ago

Also amend your commit message to remove capitals.

Calvin-Xu commented 1 year ago

I'll do that; another thing I am thinking is that if the subtitle track changed, the findWidget should trigger SubtitleListWidget::findText. A common scenario where this happens is that I have words noted down while I was watching (on something that does not have Memento), then I go back to Memento and add the vocab with the help of search. If the vocab is not in the current episode, I load the next one & its subtitles and continue.

I tried something like this, but it does not seem work perhaps because of timing issues. I would like to hear your input here.

connect(
        mediator, &GlobalMediator::playerSubtitleTrackChanged,
        this, [this](const int64_t sid) {
                    handlePrimaryTrackChange(sid);
                    findText(m_ui->lineEditSearch->text());
                },
        Qt::QueuedConnection
    );
Calvin-Xu commented 1 year ago

additional: I am thinking that Ctrl-F in the main window should open the subtitlelist and the find widget together (equivalent to Ctrl-E Ctrl-F), but that seems like a larger change

ripose-jp commented 1 year ago

I'll do that; another thing I am thinking is that if the subtitle track changed, the findWidget should trigger SubtitleListWidget::findText

I don't disagree, but that's not necessarily expected behavior from search. For example, in Firefox searching for text on one page and then clicking to a new page will not update the search results automatically. I'm not against it, but it's more of a design decision. Supposing the code you posted did work, it would execute the search even if the user was not searching which is definitely not something that should happen.

additional: I am thinking that Ctrl-F in the main window should open the subtitlelist and the find widget together (equivalent to Ctrl-E Ctrl-F), but that seems like a larger change

I'll think on it. My only concern is that if Memento binds too many keys, they may start to collide with bindings used in popular mpv scripts. I've wanted to make Memento's binds rebindable like mpv's, but that currently gate kept behind a configuration file overhaul.

Calvin-Xu commented 1 year ago

For example, in Firefox searching for text on one page and then clicking to a new page will not update the search results automatically.

I agree this is the more desirable behavior. However Firefox would update the search results on enter key (text input focus having already moved to find on page), whereas Memento only updates on text changed and does not handle the enter key. Perhaps that is something that can be improved.

ripose-jp commented 1 year ago

However Firefox would update the search results on enter key

It does not. You have to do Ctrl+F and then press Enter, which make sense since maybe the webpage is using Enter for something else. I bind Enter to sub-seek 0 in my input.conf, so I'd like to be careful with how I handle it in Memento.

That said, I'm not opposed to making Ctrl+F, instead of closing the search, just give it focus again and potentially execute the search. I'll look into what behavior I find the most usable.

Calvin-Xu commented 1 year ago

You have to do Ctrl+F and then press Enter

Yeah I should clarify that is what I meant.

I bind Enter to sub-seek 0 in my input.conf, so I'd like to be careful with how I handle it in Memento.

I agree and I also do that.

That said, I'm not opposed to making Ctrl+F, instead of closing the search, just give it focus again and potentially execute the search. I'll look into what behavior I find the most usable.

Yes, this seems like the more standard behavior now that I pay attention to it in text editors, etc.

ripose-jp commented 1 year ago

Yes, this seems like the more standard behavior now that I pay attention to it in text editors, etc.

I've decided to go with this behavior.