Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

SmartDevStar
Copy link

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
@arianneorpilla arianneorpilla self-requested a review July 18, 2023 12:55
@arianneorpilla
Copy link
Owner

arianneorpilla commented Jul 18, 2023

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!

@arianneorpilla
Copy link
Owner

arianneorpilla commented Jul 18, 2023

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.

return ByteData.view(bytes.buffer);
}

void showColorPicker(String target) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

context: context,
builder: (context) {
return AlertDialog(
title: const Text('Pick color'),
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

children: [
JidoujishoIconButton(
size: 18,
tooltip: t.google_fonts,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

_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');
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

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'];
Copy link
Owner

@arianneorpilla arianneorpilla Jul 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@SmartDevStar
Copy link
Author

SmartDevStar commented Jul 18, 2023 via email

@arianneorpilla
Copy link
Owner

arianneorpilla commented Jul 18, 2023

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
Copy link
Author

SmartDevStar commented Jul 18, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants