Blur shortcuts improvements #461
Conversation
There was a problem hiding this comment.
I think you are close, but I would like to suggest a different overall approach.
As mentioned in the comments, your current approach is:
- On key-press change internal state.
- Check state in loop and recalculate classes as necessary.
I would suggest this instead:
- On key-press call
classList.remove('asbplayer-subtitles-blurred')on the appropriate subtitle track. RecordunblurredSubtitleTracks[track] = trueonly if the class was actually removed. - In polling loop add back the class 'asbplayer-subtitles-blurred' for every marked track in
unblurredSubtitleTracksin theif (subtitlesAreNew)block. - Empty out
unblurredSubtitleTracksinsetSubtitleSettings.
This way you only modify the CSS classes at the moments when they need to be modified and avoid having to recalculate the classes over and over again in the polling loop.
In general, calculating the full state in a loop can be simpler and more bug free but in this case I think the performance implications merit the more event-based approach which isn't actually that much more complex.
| @@ -103,7 +103,7 @@ | |||
| "toggleAsbplayerSubtitleTrack2": "Untertitel 2 in asbplayer umschalten", | |||
| "toggleAsbplayerSubtitleTrack3": "Untertitel 3 in asbplayer umschalten", | |||
| "toggleAsbplayerSubtitleTracks": "Untertitel in absplayer umschalten", | |||
| "toggleAsbplayerBlurTrack": "Toggle subtitle track {{trackNumber}} blur in asbplayer", | |||
There was a problem hiding this comment.
You should keep the old key toggleAsbplayerBlurTrack as well since the same loc file is used for all versions of the extension.
There was a problem hiding this comment.
reverted the old key
| @@ -310,21 +312,25 @@ export default class SubtitleController { | |||
| this.onNextToShow?.(slice.nextToShow[0]); | |||
| } | |||
|
|
|||
| let shouldUnblur = false; | |||
| if (this.subtitleSettings) { | |||
There was a problem hiding this comment.
This is relatively expensive code to be running ~10 times a second. I think there is a more performant approach.
| src: context.video.src, | ||
| }; | ||
| chrome.runtime.sendMessage(command); | ||
| context.subtitleController.unblurredSubtitleTracks[track] = |
There was a problem hiding this comment.
Instead of modifying the state of subtitleController I suggest implementing a function to trigger the CSS class changes since a function would give you the opportunity to run some code (vs changing internal state and polling for it in a loop).
artjomsR
force-pushed
the
feat/blur_shortcuts_improvements
branch
from
62b00a7
to
f232dc8
Compare
|
I pushed what I described as the solution. I took it for granted that you could query the displaying subtitle elements and also know their track number. I think this is where the difficulty was. |
|
That's great, thank you so much for looking into it! From my point of view, it's functionally complete now, the only thing now that comes to my mind is the translation strings. I've tried to cherry-pick |
|
Thinking about it we should probably add completely new loc strings for this "unblur" behavior. Since it is significantly different from "toggle." But we will need to keep the old loc keys/values for older versions of the extension. |
|
Do you mean having shortcuts for both toggle and unblur actions? I think Toggle Unblur is too similar to Hide/Show Subtitle Track shortcut (in terms of workflow), but then again nothing's preventing us from having both |
Nope, not shortcuts, just new translation (localization) keys. So that we change the labels for the existing shortcuts, while keeping the existing labels for older versions of the extension that have the old toggle behavior. |
|
I've checked and blur shortcuts worked for me after the rename so should be all good now |
|
Thanks again! |
Implement #451
atm I've been only focusing on the extension. Current implementation works, but it's not immediate to unblur / reblur (on subtitle change). I suspect this is due to refreshing the caching, but I've not figured out a way to update it otherwise. So, not sure the current implementation can be improved
Happy if somebody else wants to update / pick this up, as I'm feeling a bit stuck with this as of now
(I've also realised that the renaming of the shortcut broke the untoggling, would need to investigate why. But the PR works as of the 1st commit
3df421e)