Top / bottom setting for individual subtitles #508

artjomsR · 2024-09-02T08:52:27Z

Mostly working for the extension and still needs cleaning up, but I wanted to ask first: is there any penalty for always having 2 subtitle containers (one at top and one at bottom), even if one of them is not used? E.g., all subtitles are displayed at the bottom?

killergerbah · 2024-09-20T23:39:43Z

is there any penalty for always having 2 subtitle containers (one at top and one at bottom), even if one of them is not used?

Sorry for the really late response on this. I probably clicked through this PR and then forgot to reply.
I don't think there should be a huge penalty. My guess is that the DOM cache will be the biggest drain on resources, but also probably not a big deal. But it would be good to avoid rendering the subtitles unnecessarily. E.g. if only top or only bottom then only render top or only render bottom.

artjomsR · 2024-09-22T19:26:22Z

no problem, thanks for getting back to me! I've updated the PR now, let me know what you think :)

I'm also thinking if we should remove the if checks from shouldRenderBottomOverlay (and remove it altogether), leaving only shouldRenderTopOverlay: boolean;: I think users would use either only bottom or bottom + top subtitles. But I don't see why someone would use only top subtitles

killergerbah

As always thank you. It took me a while to get to this because of work and also trying to get 1.5.0 out first.

killergerbah · 2024-10-04T12:30:17Z

extension/src/controllers/subtitle-controller.ts

        this._preCacheDom = preCacheDom;
    }

    cacheHtml() {
-        if (!(this.subtitlesElementOverlay instanceof CachingElementOverlay)) {
+        if (!(this.bottomSubtitlesElementOverlay instanceof CachingElementOverlay) || !(this.topSubtitlesElementOverlay instanceof CachingElementOverlay)) {


Currently, the check assumes that both overlays are always the same type, which may be true and enforced internally by this class.

But since it does not hurt, I think to be safe the instanceof check should be done independently for bottomSubtitlesElementOverlay and topSubtitlesElementOverlay before calling uncacheHtml.

killergerbah · 2024-10-04T12:32:20Z

extension/src/controllers/subtitle-controller.ts

@@ -154,6 +169,22 @@ export default class SubtitleController {
            this.cacheHtml();
        }

+        const newAlignments = allTextSubtitleSettings(newSubtitleSettings).map((s) => s.subtitleAlignment);
+        if (newAlignments !== this.subtitleTrackAlignments) {


This check will always return true because JavaScript will use object identity for comparison. This class already implements _arrayEquals, you can use that instead.

killergerbah · 2024-10-04T12:39:06Z

extension/src/controllers/subtitle-controller.ts

-
-    get subtitleAlignment() {
-        return this._subtitleAlignment;
+    getSubtitleTrackAlignment(trackIndex: number) {


Can be private

killergerbah · 2024-10-04T12:40:56Z

extension/src/controllers/subtitle-controller.ts

@@ -451,7 +473,10 @@ export default class SubtitleController {
            this.notificationElementOverlayHideTimeout = undefined;
        }

-        this.subtitlesElementOverlay.dispose();
+        if (this.shouldRenderBottomOverlay)


Skip the should* checks - just dispose both overlays.

killergerbah · 2024-10-04T12:45:12Z

common/settings/settings.ts

@@ -149,7 +150,8 @@ const subtitleSettingsKeysObject: { [key in keyof SubtitleSettings]: boolean } =
    subtitleCustomStyles: true,
    subtitleBlur: true,
    imageBasedSubtitleScaleFactor: true,
-    subtitlePositionOffset: true,
+    bottomSubtitlePositionOffset: true,


Use subtitlePositionOffset instead of bottomSubtitlePositionOffset and add a comment saying it's the bottom offset. The naming is less clear but without a system to migrate the settings structure it's safer to preserve older names, since the extension and app share settings.

artjomsR · 2024-10-08T13:25:05Z

No problem for the delay, there's no rush :)

I've updated the PR, should've addressed all your comments. You didn't address my previous comment, not sure if you have an opinion about this?

I'm also thinking if we should remove the if checks from shouldRenderBottomOverlay (and remove it altogether), leaving only shouldRenderTopOverlay: boolean;: I think users would use either only bottom or bottom + top subtitles. But I don't see why someone would use only top subtitles

killergerbah · 2024-10-09T10:51:48Z

No problem for the delay, there's no rush :)

I've updated the PR, should've addressed all your comments. You didn't address my previous comment, not sure if you have an opinion about this?

I'm also thinking if we should remove the if checks from shouldRenderBottomOverlay (and remove it altogether), leaving only shouldRenderTopOverlay: boolean;: I think users would use either only bottom or bottom + top subtitles. But I don't see why someone would use only top subtitles

Sorry forgot to reply to that as well. There are two reasons why I think we should leave it as is:

It's not obvious to me how we would work that into the current settings UI. For example you would have to prevent users from switching all tracks to the top. It would seem then the track specific settings would have to be aware of each other which might introduce complexity.
There are users who use top only. For example to add second language subtitles to websites that already have their own subtitles.

artjomsR · 2024-10-09T11:04:25Z

I meant to keep the functionality as is, simply skip the if shouldRenderBottomOverlay check and always invoke all functionality related to the bottom overlay. Once again, based on the principle that most people would use the bottom container.

But as you say, if considerable number of users use top only, then we can leave everything as is :)

killergerbah

It looks like you removed the subtitleAlignment setting. If you're not using that setting, how are you intending for users to be able to pick the alignment for individual tracks?

killergerbah · 2024-10-12T03:50:33Z

scripts/anki-connect-proxy/anki-connect-proxy.exe

I think this file should be part of a different PR.

artjomsR · 2024-10-12T09:31:07Z

It looks like you removed the subtitleAlignment setting. If you're not using that setting, how are you intending for users to be able to pick the alignment for individual tracks?

in common/settings/settings.ts, I've moved it from SubtitleSettings (which I understood as "global" settings) to TextSubtitleSettings (which I understood as settings which apply to individual tracks), which is effectively all this PR is about

Replay_2024-10-12_10-26-57.mp4

killergerbah · 2024-10-12T22:47:02Z

Understood!

killergerbah · 2024-10-13T09:37:47Z

extension/src/controllers/subtitle-controller.ts

@@ -160,6 +172,27 @@ export default class SubtitleController {
            this.cacheHtml();
        }

+        const newAlignments = allTextSubtitleSettings(newSubtitleSettings).map((s) => s.subtitleAlignment);


I found a bug that I believe has to do with this line. See the video below. You'll notice that there are two tracks, but when switching all tracks, or track 0, to the 'top' alignment, one of the tracks disappears.

2024-10-13.6.05.27.mov

Despite the name,allTextSubtitleSettings doesn't return the TextSubtitleSettings for a specific number of tracks, it only returns settings for the ones that are actually configured.

This means that we need to account for TextSubtitleSettings missing for some tracks. You'll notice that SubtitleController._subtitleStyles does this by assuming any missing track-specific settings should just inherit the settings for track 0. Unfortunately this means that calculating shouldRenderBottomOverlay and shouldRenderTopOverlay will always depend on the _subtitles field and we would need to re-calculate those fields whenever _subtitles changes.

So for now I suggest the following:

Remove shouldRenderBottomOverlay and shouldRenderTopOverlay fields. Always cache HTML for both overlays.

(Optional) Refactor CachingElementOverlay so that topSubtitlesElementOverlay and bottomSubtitlesElementOverlay can share the same DOM cache. This will cancel out the overhead of always having two overlays.

killergerbah · 2024-10-13T10:10:40Z

extension/src/controllers/subtitle-controller.ts

-    get subtitleAlignment() {
-        return this._subtitleAlignment;
+    private _getSubtitleTrackAlignment(trackIndex: number) {
+        return this.subtitleTrackAlignments[trackIndex] || 'bottom';


As mentioned above the default value should really be the setting for the 0th track:

this.subtitleTrackAlignments[trackIndex] || this.subtitleTrackAlignments[0]

artjomsR · 2024-10-18T08:30:16Z

I've added your suggested change of || this.subtitleTrackAlignments[0] which seems to have fixed the bug for me, I've tested several times. So I'm not sure if the caching changes are still required now

killergerbah · 2024-10-18T09:09:13Z

Sorry, yeah you're right. Thanks again for your patience.

artjomsR marked this pull request as draft September 2, 2024 08:57

artjomsR force-pushed the feat/top_bottom_subtitles branch 7 times, most recently from 000e7b6 to c1635d8 Compare September 2, 2024 09:32

artjomsR marked this pull request as ready for review September 22, 2024 19:25

killergerbah requested changes Oct 4, 2024

View reviewed changes

artjomsR added 11 commits October 8, 2024 14:19

extension: Add topSubtitleParams as a separate container

a52d3ba

settings + extension: assign subtitle tracks to their position

cf67bf6

add topOffset setting

b86d02e

fix blur not working

cdeb114

localise

ea1267d

cleanup

85b2cba

test fixes

31d6acf

only render respective overlays if they're actually enabled

444dd42

subtitlesElementOverlay => bottomSubtitlesElementOverlay

7652f25

update cacheHtml()

cc02d4a

subtitlePositionOffset => bottomSubtitlePositionOffset

346e43f

artjomsR force-pushed the feat/top_bottom_subtitles branch from ec0df42 to eaef90b Compare October 8, 2024 13:23

artjomsR requested a review from killergerbah October 8, 2024 13:25

review comments

3b213c7

artjomsR force-pushed the feat/top_bottom_subtitles branch from eaef90b to 3b213c7 Compare October 8, 2024 14:59

artjomsR added 2 commits October 9, 2024 12:10

pretty

3af1032

test fixes

a00105b

killergerbah reviewed Oct 12, 2024

View reviewed changes

remove file comitted by mistake

63aba9d

killergerbah requested changes Oct 13, 2024

View reviewed changes

review comments: fix track disappearing

7b123fb

killergerbah merged commit b39cef5 into killergerbah:main Oct 18, 2024
1 check passed

killergerbah added this to the Extension v1.6.0 milestone Oct 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Top / bottom setting for individual subtitles #508

Top / bottom setting for individual subtitles #508

artjomsR commented Sep 2, 2024

killergerbah commented Sep 20, 2024

artjomsR commented Sep 22, 2024 •

edited

Loading

killergerbah left a comment

killergerbah Oct 4, 2024

killergerbah Oct 4, 2024

killergerbah Oct 4, 2024

killergerbah Oct 4, 2024

killergerbah Oct 4, 2024 •

edited

Loading

artjomsR commented Oct 8, 2024

killergerbah commented Oct 9, 2024

artjomsR commented Oct 9, 2024

killergerbah left a comment

killergerbah Oct 12, 2024 •

edited

Loading

artjomsR commented Oct 12, 2024 •

edited

Loading

killergerbah commented Oct 12, 2024

killergerbah Oct 13, 2024 •

edited

Loading

killergerbah Oct 13, 2024

artjomsR commented Oct 18, 2024

killergerbah commented Oct 18, 2024

Top / bottom setting for individual subtitles #508

Top / bottom setting for individual subtitles #508

Conversation

artjomsR commented Sep 2, 2024

killergerbah commented Sep 20, 2024

artjomsR commented Sep 22, 2024 • edited Loading

killergerbah left a comment

Choose a reason for hiding this comment

killergerbah Oct 4, 2024

Choose a reason for hiding this comment

killergerbah Oct 4, 2024

Choose a reason for hiding this comment

killergerbah Oct 4, 2024

Choose a reason for hiding this comment

killergerbah Oct 4, 2024

Choose a reason for hiding this comment

killergerbah Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

artjomsR commented Oct 8, 2024

killergerbah commented Oct 9, 2024

artjomsR commented Oct 9, 2024

killergerbah left a comment

Choose a reason for hiding this comment

killergerbah Oct 12, 2024 • edited Loading

Choose a reason for hiding this comment

artjomsR commented Oct 12, 2024 • edited Loading

killergerbah commented Oct 12, 2024

killergerbah Oct 13, 2024 • edited Loading

Choose a reason for hiding this comment

killergerbah Oct 13, 2024

Choose a reason for hiding this comment

artjomsR commented Oct 18, 2024

killergerbah commented Oct 18, 2024

artjomsR commented Sep 22, 2024 •

edited

Loading

killergerbah Oct 4, 2024 •

edited

Loading

killergerbah Oct 12, 2024 •

edited

Loading

artjomsR commented Oct 12, 2024 •

edited

Loading

killergerbah Oct 13, 2024 •

edited

Loading