Focus on pause

[murezzda]

Is your Pull Request request related to another issue in this repository ?
Related to issue #97

Describe what the PR does
After pausing the player via the alt+k shortcut, the cursor is moved to the current word.

State whether the PR is ready for review or whether it needs extra work
PR still needs some testing, but functionality wise works.

[pietrop]

👋 @murezzda , thanks for this!
Tested the functionality, seems to be working fine, before I start reviewing the code, wanted to check if this PR is ready for review?

[murezzda]

Hi @pietrop, I will have a final look at the PR tomorrow and give you notice as soon as I've checked it again.

[pietrop]

I think you mentioned in TimedTextEditor getCurrentFocus() line 494 that

using convertToRaw here might be slowing down performance(?)

Which is something I might have noted elsewhere in the codebase, have you come across any alternatives? (otherwise we could roll with that for now)

[murezzda]

Hi @pietrop
I've checked the code again and it would be ready for review.

I have still the following points open, maybe you have an option/idea about it:

1. Where to put cursor / whether to select word or not.

• At the moment the cursor is placed at the end of the current word
• Alternatives could be to place it at the beginning of the current word or to pre-select the current word.
• Pre-selection would be a good option, but doesn't seem to be intuitive with the current layout, as the selected text has the same color as the text highlight from the player. Also it might lead to accidental deletion of words if people are not cautious.

2. Performance and word selection lag

• Regarding your question about the comment at line 494, I've based the methon on the already present getCurrentWord method which also has this comment in order to be able to find it if an alternative was found. At the moment I do not have an alternative.
• The performance of the word selection seems to be wanting at times, as the video highlight tends to move ahead of the selection if the spoken words are short. In my opinion this is does not break the feature, but could be annoying to the user.

[pietrop]

👋 @murezzda, thanks for the PR would you be able to add an option in settings to make this optional?
Let me know, I can help/show how that would work.
With @jamesdools , we were thinking that it could be on by default, but with an option to turn it of?

I also check with @Laurian re-using convertToRaw and he said it should be ok from a performance point of view.

[murezzda]

Hi @pietrop, making this feature optional makes also sense to me. I will check out the options code and will contact you if there are any questions.

If its not the problem of convertToRaw I am going to look over the selection loop again. I have a suspicion that there could be a better solution than the current one.

Thanks for the input!

[murezzda]

Hi @pietrop, I've added now the option of turning this feature off in the settings and I was also able to fix the issue with the lagging selection. The side-effect of fixing this is that the cursor will also be moved if the user pauses the video via the video-controls, but in my opinion this is anyway the sensible thing to do.

[murezzda]

Setting focus whenever a pause is triggered is not compatible with the pause-while-typing. If both are active, the cursor will move to the position of the video player whenever someone types during the video. This will prevent the user from correcting the word.

[pietrop]

ah, ok interesting, thanks for this, going to find some time have a look and running locally.

[pietrop]

Thanks @murezzda

1. Where to put cursor / whether to select word or not.

I think this seems fine

At the moment the cursor is placed at the end of the current word

We could also add a reminder in keyboard shortcuts that alt+shift+→ select the next word

And that alt+shift+← select previous word.

These are defaults behaviour, supported by draftJS.

2.Performance and word selection lag
I didn't notice any lags so far.

4. cursor in selection
Seems like the cursor gets "trapped" in the current word selection when one is present.

in the gif below you can see how if a word is not selected eg end of sentence, it works fine, but that if a current word is selected then it would add the text inside the selection. Then selection will disappear when playing again.

This is something that might look as simple technical glitch, but I can see how it might be confusing (and possibly mildly annoying?) for some users.

5. Settings option
Thanks for adding the optional toggle in settings.

6. not compatible with pause while typing

You mentioned

Setting focus whenever a pause is triggered is not compatible with the pause-while-typing. If both are active, the cursor will move to the position of the video player whenever someone types during the video. This will prevent the user from correcting the word.

And I can confirm the problem.

This makes me think, if we can't fix this, it should then not be on by default.
A workaround is that If it was off by default, then in the settingss, toggling this on, would turn pause while typing off to avoid conflict, and user can choose which one to use.

---

TL;DR

One of our assumption, that we are about to user test, is whether pause while typing, is a key feature while correcting automated transcript.

After having played around with it a bit more. I am inclined to say that we should probably close and park this for now. We can do further research and exploration and a later stage if it comes up as a user need. But wouldn't want to fall into the trap of premature optimisation of the tool and feature creep. As we are aiming to spend a bit of time to consolidate the editor and refactor the code, to make it more modular and more robust. Giving priority to #50 + #49 + #30

[murezzda]

Hi @pietrop

Thanks for the in-depth look at the PR!

I have a few additional comments, but generally I agree with your assessment.

Performance and word selection lag vs non-compatibility with pause-on-typing
The performance issues I've described previously where from the version before the last commit in this PR. In that version, the moving of the cursor was done inside the hotkey evaluation code. This ensured that focus was only set when a pause via hotkey happened. I changed this for two reasons: First, I thought it too confusing to restrict this functionality to hotkeys only. The second reason was that when this was done just after invoking self.togglePlayMedia();, the pausing of the media player and the moving of the cursor was not synchronous.

The current version moves the cursor in the onPause event in the Video Player. This ensures that the cursor is only moved after the player has stopped, but introduces the clash with the pause while typing feature as it is no longer possible to track the origin of the pause event.

I guess one could hack this to work by adding more state to track the origin of the event, but I think that would neither be a good nor a maintainable solution...

Cursor in Selection
Thanks for catching this. I will look at this -if- I will work on this feature in the future.

Verdict
I agree that we should park this feature for the moment, as there seem to be some issues which are not easily resolved. I will post an update if there are further developments which would solve these issues.

[pietrop]

Awesome, will close this for now, but we can revisit at later stage, thank you @murezzda