added subtitle font settings in subtitle option dialog and added a to… #279

SmartDevStar · 2023-07-18T12:37:49Z

added subtitle font settings in subtitle option dialog and added a toggle button to show media player bottom bar permanently.

…ggle button to show media player bottom bar permanently

lrorpilla · 2023-07-18T12:58:32Z

~~Hi @SmartDevStar, thanks for your pull request. This project uses slang to generate and manage localisations, and you can access localised strings by using the t global variable.~~

~~Could I ask you to change your hardcoded strings to make use of new localised strings you can find in the localisation JSON file?~~

~~You can add values by editing that file and running flutter pub run slang.~~

Looks like I overlooked the edits to the localisation file. Thanks again and I'll test your branch sometime!

lrorpilla · 2023-07-18T13:34:28Z

Hey @SmartDevStar. I've requested some changes on this pull request. Thanks for making it again -- I haven't tested and run it myself yet but I think I get the general idea of what you want to do. I think the new button you've made for pinning the menu needs to go elsewhere, however...

I'll test these changes myself. I know that this PR was made in relation to #276, so I don't mind if you made multiple PRs in progression for the following tasks, so you're not blocked in case you are ready to work on another PR while I am still approving things -- I'm actually quite busy to approve and unable to merge or make a new release for this feature ASAP but I am happy for you to lodge multiple PRs in the meantime and just compile the app for now with your own forked branch in the meantime while I haven't gotten to it yet.

Thanks again and really appreciate it.

lrorpilla · 2023-07-18T13:09:56Z

yuuna/lib/src/pages/implementations/subtitle_options_dialog_page.dart

+    return ByteData.view(bytes.buffer);
+  }
+
+  void showColorPicker(String target) {


Is it possible you could change this to use an enum or a separate function instead of taking a String parameter?

lrorpilla · 2023-07-18T13:10:42Z

yuuna/lib/src/pages/implementations/subtitle_options_dialog_page.dart

+        context: context,
+        builder: (context) {
+          return AlertDialog(
+            title: const Text('Pick color'),


Hardcoded string, I would remove the title here altogether, since it's pretty obvious from the content that the activity is color picking. I also noticed that this doesn't have buttons to cancel or confirm the color selection before changing the color.

Does this also change the hex value in text on the TextEditingController you've made for fontColor and outlineColor, so the user can confirm that they are changing the setting before saving all the settings?

lrorpilla · 2023-07-18T13:12:10Z

yuuna/lib/src/pages/implementations/player_source_page.dart

@@ -1207,6 +1225,7 @@ class _PlayerSourcePageState extends BaseSourcePageState<PlayerSourcePage>
            child: Row(
              children: [
                const Space.small(),
+                buildPinButton(),


I don't think this is an intuitive position to place this menu item. This needs to go elsewhere.

lrorpilla · 2023-07-18T13:14:10Z

yuuna/lib/src/pages/implementations/subtitle_options_dialog_page.dart

@@ -29,25 +33,36 @@ class _SubtitleOptionsDialogPage
  late final TextEditingController _delayController;
  late final TextEditingController _fontSizeController;
  late final TextEditingController _fontNameController;
+  late final TextEditingController _fontColorController;
+  late final TextEditingController _outLineColorController;


It's a nitpick but I would prefer outline instead of outLine

lrorpilla · 2023-07-18T13:21:54Z

yuuna/lib/src/pages/implementations/subtitle_options_dialog_page.dart

+                      children: [
+                        JidoujishoIconButton(
+                          size: 18,
+                          tooltip: t.google_fonts,


When you long-press on an icon button, this should show an appropriate tooltip. Since you've changed the behavior of this button, the tooltip should now be different.

lrorpilla · 2023-07-18T13:23:04Z

yuuna/lib/src/pages/implementations/subtitle_options_dialog_page.dart

    _allowanceController =
        TextEditingController(text: _options.audioAllowance.toString());
    _delayController =
        TextEditingController(text: _options.subtitleDelay.toString());
    _fontSizeController =
        TextEditingController(text: _options.fontSize.toString());
    _fontNameController = TextEditingController(text: _options.fontName.trim());
+    _fontColorController = TextEditingController(text: 'Font Color');


Am I correct in understanding that this will be the initial text that appears when the settings dialog appears? If so, this won't do. I would want to see the current hex value of the color here instead.

lrorpilla · 2023-07-18T13:24:19Z

yuuna/lib/src/pages/implementations/subtitle_options_dialog_page.dart

@@ -410,8 +515,11 @@ class _SubtitleOptionsDialogPage
      subtitleOptions.regexFilter = newRegexFilter;
      subtitleOptions.fontName = newFontName;
      subtitleOptions.fontSize = newFontSize;
+      subtitleOptions.fontColor = fontColor.value;


Does the font and outline color change if I manually edit the text, and not use the picker? Can you verify if hex values entered are valid if I manually edit the text?

lrorpilla · 2023-07-18T13:26:29Z

yuuna/lib/src/pages/implementations/subtitle_options_dialog_page.dart

  late final TextEditingController _regexFilterController;
  late final TextEditingController _opacityController;
  late final TextEditingController _widthController;
  late final TextEditingController _blurController;

  late ValueNotifier<bool> _aboveBottomBarNotifier;
+  Color fontColor = Colors.white;
+  Color outLineColor = Colors.white;
+  List<String> fontWeights = ['Thin', 'Normal', 'Bold'];


I would prefer using an enum here if possible, or making use of FontWeight itself.

SmartDevStar · 2023-07-18T14:10:02Z

Hey, Irorpilla. I don't also have much time. If you are not satisfied for UI, provide me exact UI so we don't waste time.

…

On Tue, Jul 18, 2023 at 7:34 AM lrorpilla ***@***.***> wrote: Hey @SmartDevStar <https://github.com/SmartDevStar>. I've requested some changes on this pull request. Thanks for making it again -- I haven't tested and run it myself yet but I think I get the general idea of what you want to do. I think the new button you've made for pinning the menu needs to go elsewhere, however... I'll test these changes myself. I know that this PR was made in relation to #276 <#276>, so I don't mind if you made multiple PRs in progression -- I'm actually quite busy to approve and make a new release for this feature ASAP but I am happy for you to lodge multiple PRs in the meantime and just compile the app for now with your own forked branch in the meantime while I haven't gotten to it yet. Thanks again and really appreciate it. — Reply to this email directly, view it on GitHub <#279 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/A6XQEN44ADMSLFMDDYDRXT3XQ2GG7ANCNFSM6AAAAAA2OKWVEI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

lrorpilla · 2023-07-18T14:21:31Z

Since we are both busy, you are welcome to leave your PR as is in the meantime. I will need to ensure this pull request meets certain standards and is in line with the rest of the project before merging, and part of that process is meeting me halfway with these requested changes.

SmartDevStar · 2023-07-18T19:11:40Z

Updated the request changes.

…

On Tue, Jul 18, 2023 at 8:21 AM lrorpilla ***@***.***> wrote: Since we are both busy, you are welcome to leave your PR as is in the meantime. I will need to ensure this pull request meets certain standards and part of that process is meeting me halfway with these requested changes. — Reply to this email directly, view it on GitHub <#279 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/A6XQEN4HVLBUZRSW7KOGQO3XQ2LXLANCNFSM6AAAAAA2OKWVEI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

…pShown option

added subtitle font settings in subtitle option dialog and added a to…

3e4c200

…ggle button to show media player bottom bar permanently

lrorpilla self-requested a review July 18, 2023 12:55

lrorpilla requested changes Jul 18, 2023

View reviewed changes

updated for first PR

e793696

SmartDevStar added 5 commits July 19, 2023 16:03

fixed issues for subtitle font option issues and store bottom bar kee…

cf657ee

…pShown option

fixed issues for first milestone

f72cfa8

completed subtitle font settings for video player

a76e669

added subtitle seek and fullscreen mode functionality

2822424

completed subtitle settings of video player

6ff8296

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added subtitle font settings in subtitle option dialog and added a to… #279

added subtitle font settings in subtitle option dialog and added a to… #279

SmartDevStar commented Jul 18, 2023

lrorpilla commented Jul 18, 2023 •

edited

lrorpilla commented Jul 18, 2023 •

edited

lrorpilla Jul 18, 2023

lrorpilla Jul 18, 2023

lrorpilla Jul 18, 2023

lrorpilla Jul 18, 2023

lrorpilla Jul 18, 2023

lrorpilla Jul 18, 2023

lrorpilla Jul 18, 2023

lrorpilla Jul 18, 2023 •

edited

SmartDevStar commented Jul 18, 2023 via email

lrorpilla commented Jul 18, 2023 •

edited

SmartDevStar commented Jul 18, 2023 via email

added subtitle font settings in subtitle option dialog and added a to… #279

Are you sure you want to change the base?

added subtitle font settings in subtitle option dialog and added a to… #279

Conversation

SmartDevStar commented Jul 18, 2023

lrorpilla commented Jul 18, 2023 • edited

lrorpilla commented Jul 18, 2023 • edited

lrorpilla Jul 18, 2023

Choose a reason for hiding this comment

lrorpilla Jul 18, 2023

Choose a reason for hiding this comment

lrorpilla Jul 18, 2023

Choose a reason for hiding this comment

lrorpilla Jul 18, 2023

Choose a reason for hiding this comment

lrorpilla Jul 18, 2023

Choose a reason for hiding this comment

lrorpilla Jul 18, 2023

Choose a reason for hiding this comment

lrorpilla Jul 18, 2023

Choose a reason for hiding this comment

lrorpilla Jul 18, 2023 • edited

Choose a reason for hiding this comment

SmartDevStar commented Jul 18, 2023 via email

lrorpilla commented Jul 18, 2023 • edited

SmartDevStar commented Jul 18, 2023 via email

lrorpilla commented Jul 18, 2023 •

edited

lrorpilla commented Jul 18, 2023 •

edited

lrorpilla Jul 18, 2023 •

edited

lrorpilla commented Jul 18, 2023 •

edited