CCExtractor / ultimate_alarm_clock

GNU General Public License v2.0
68 stars 122 forks source link

App Theming Process Updated as per Issue #576 #577

Closed Gaurav-Kushwaha-1225 closed 5 days ago

Gaurav-Kushwaha-1225 commented 2 months ago

Description Issue No. - #576

This change addresses Issue #576 by adopting a more robust theming mechanism for the application. Also, regarding the theme issue on Add Timer Dialog Box.

Proposed Changes

Previous Method:

New Method (Inspired by reference article as mentioned in the Issue):

Fixes #576

Screenshots

light

Checklist

AryanSarafDev commented 2 months ago

Hey! Great Job. The existing theming uses ternary everywhere for checking the current theme. Can you make common color variables which can just switch values inside ThemeController when the theme is changed for optimization? for example: a functions which change the color values for all common variables when the theme is switched. Feel free to ask if you have any doubts!

Gaurav-Kushwaha-1225 commented 2 months ago

Hey! Great Job. The existing theming uses ternary everywhere for checking the current theme. Can you make common color variables which can just switch values inside ThemeController when the theme is changed for optimization? for example: a functions which change the color values for all common variables when the theme is switched. Feel free to ask if you have any doubts!

So, you are saying. Instead of having different Color type variables all over your code for both themes like ksecondaryColor & kLightsecondaryColor, etc. We should imagine having a single variable for this purpose. When we switch themes, we simply change what's inside these variables, and all the places that use these variables automatically update with the new theme!

This way, we only change the variables in one place i.e. in theme_controller.dart, and everything UI that relies on these variables gets updated throughout our app.

Did I get your point?? @AryanSarafDev

AryanSarafDev commented 2 months ago

Hey! Great Job. The existing theming uses ternary everywhere for checking the current theme. Can you make common color variables which can just switch values inside ThemeController when the theme is changed for optimization? for example: a functions which change the color values for all common variables when the theme is switched. Feel free to ask if you have any doubts!

So, you are saying. Instead of having different Color type variables all over your code for both themes like ksecondaryColor & kLightsecondaryColor, etc. We should imagine having a single variable for this purpose. When we switch themes, we simply change what's inside these variables, and all the places that use these variables automatically update with the new theme!

This way, we only change the variables in one place i.e. in theme_controller.dart, and everything UI that relies on these variables gets updated throughout our app.

Did I get your point?? @AryanSarafDev

Exactly! That's what I meant. If you'll need any help with it let me know @Gaurav-Kushwaha-1225 👍.

Gaurav-Kushwaha-1225 commented 1 month ago

Issue Resolution and Implementation Changes

Introduction

The issue related to widget color adaptation based on the current theme has been resolved. The previous implementation used a ternary operator in multiple places to change colors according to the theme. The new implementation introduces a function getColor(colorName) in theme_controller.dart, simplifying the process and ensuring automatic updates of widget colors based on the app theme.

Previous Implementation

Previously, widgets adapted to different colors according to the current theme using a ternary operator. For example:

backgroundColor: themeController.isLightMode.value == ThemeMode.Light
            ? kLightSecondaryBackgroundColor
            : kSecondaryBackgroundColor

New Implementation

The new implementation uses a function getColor(colorName) in theme_controller.dart, which helps in automatically updating the color of the widget according to the app theme. For example:

backgroundColor: themeController.getColor('secondaryBackgroundColor')

Example Usage

  1. Before:

    backgroundColor: themeController.isLightMode.value == ThemeMode.Light
                ? kLightSecondaryBackgroundColor
                : kSecondaryBackgroundColor
  2. After:

    backgroundColor: themeController.getColor('secondaryBackgroundColor')

Limitations

There are a few limitations with the new implementation. In some cases, the solution cannot be applied due to the use of different types of colors in light and dark themes. For example, in home_controller.dart:

TextStyle(
    color: themeController.currentTheme.value == ThemeMode.Light
        ? kLightPrimaryTextColor
        : kSecondaryTextColor,
)

In the above example, the updated method getColor cannot be used because it involves different types of colors (primary in light theme and secondary in dark theme).

Feedback

Let me know your review on the new implementation and any suggestions you might have. @AryanSarafDev

Gaurav-Kushwaha-1225 commented 1 month ago

Hey! Thank you for your kind feedback.

I understand the concerns about using a map for color names. To address these, I have implemented a new solution using a function in theme_controller.dart. Here's how it looks:

theme_controller.dart

Color secondaryColor() {
    return currentTheme.value == ThemeMode.light
        ? kLightSecondaryColor
        : kSecondaryColor;
}

Usage

backgroundColor: themeController.secondaryColor()

This way, we avoid the issues with string keys and the extra fetch function call, making it easier for new contributors and maintaining efficiency.

Regarding your query,

Let me know your feedback and any suggestions you might have. Thank You. @AryanSarafDev

AryanSarafDev commented 1 month ago

Hey! Thank you for your kind feedback.

I understand the concerns about using a map for color names. To address these, I have implemented a new solution using a function in theme_controller.dart. Here's how it looks:

theme_controller.dart

Color secondaryColor() {
    return currentTheme.value == ThemeMode.light
        ? kLightSecondaryColor
        : kSecondaryColor;
}

Usage

backgroundColor: themeController.secondaryColor()

This way, we avoid the issues with string keys and the extra fetch function call, making it easier for new contributors and maintaining efficiency.

Regarding your query,

  • Using a variable that updates automatically according to the current theme without calling a function directly in the backgroundColor property is challenging.
  • The primary issue is that Flutter's reactive framework relies on rebuilds to update the UI based on state changes. Functions like secondaryColor() provide a way to determine the correct color during these rebuilds.
  • Direct variable assignment without a function would require a more complex state management solution, potentially involving extensive use of state management libraries like Provider or Riverpod to listen for theme changes and update variables accordingly.
  • However, these approaches can introduce their own complexities and might not be as straightforward as using functions for theme-based color determination.

Let me know your feedback and any suggestions you might have. Thank You. @AryanSarafDev

@Gaurav-Kushwaha-1225 Hey, You can just use the Rx variables in view because they are wrapped in Obx() so they can listen to value changes. And you can update the variables in the Theme controller using two functions, One for dark and one for light which will be called during theme switching and update the values. The variables in the view will listen to the changes and update accordingly!.

ex-

in ThemeController class,

Rx\ kprimaryColor = Colors.white.obs;

toDark(){

kprimaryColor.value = Colors.balck;

}

and same for light.

Call the toDark and toLight functions on Theme switching and in controller's init().

And use the variables normally in view

Obx( () => Controller(color: themeController.kprimaryColor);

Gaurav-Kushwaha-1225 commented 3 weeks ago

Resuming Work on PR

Thank you for your understanding. @AryanSarafDev

Gaurav-Kushwaha-1225 commented 2 weeks ago

Implementation

Limitations

TextStyle(
    color: themeController.currentTheme.value == ThemeMode.Light
        ? kLightPrimaryTextColor
        : kSecondaryTextColor,
)

In the above example, I can't proceed with color: themeController.primaryTextColor.value or color: themeController.secondaryTextColor.value because widget has different color types in different themes i.e. primary in light theme and secondary in dark theme.

Let me know your feedback and any suggestions you might have regarding this limitation. Thank You. @AryanSarafDev

AryanSarafDev commented 2 weeks ago

Implementation

  • As discussed, I've replaced the map-based color system with a function in theme_controller.dart.
  • For example, the implementation now uses themeController.secondaryColor.value to give widgets secondaryColor property wrapped in Obx.

Limitations

  • As mentioned earlier, there is a limitation.
  • In some cases, the solution cannot be applied due to the use of different types of colors in light and dark themes. For example, in home_controller.dart:
TextStyle(
    color: themeController.currentTheme.value == ThemeMode.Light
        ? kLightPrimaryTextColor
        : kSecondaryTextColor,
)

In the above example, I can't proceed with color: themeController.primaryTextColor.value or color: themeController.secondaryTextColor.value because widget has different color types in different themes i.e. primary in light theme and secondary in dark theme.

Let me know your feedback and any suggestions you might have regarding this limitation. Thank You. @AryanSarafDev

Hey @Gaurav-Kushwaha-1225 , great job! Now that the theme controller is done, the light mode colours are not up to the mark and need some rework. As you mentioned some variables are different from dark mode too. If you want you can rework the light mode colours or I will take care of it. Some major UI changes are going to be merged tomorrow, you can have a look at the "features" branch. So I think you should plan according to it and add the new variables to it too.

Gaurav-Kushwaha-1225 commented 6 days ago

Implementation Changes

The issues with color clashes, caused by different types of colors used, varying opacities, etc. have now been resolved, ensuring a consistent appearance after the changes were implemented.

Changes made in the code

Feedback

Let me know your review and if it's correct I think it's ready to merge. @AryanSarafDev