Theme Toggle switch refactoring

Question

I built a theme toggle switch function, and refactored it following an awesome suggestion in a previous answer.

Before the refactoring the function looked like this:

def toggle_theme():
    if state["theme"]["light_mode"] is False:
        # If it is dark mode at the moment, make light
        state["theme"]["light_mode"] = True
        state["theme"]["theme_bg"] = config["theme"]["light_bg"]
        screen.bgcolor(config["theme"]["light_bg"])
        theme_button.config(text=config["buttons"]["theme"]["dark_text"])
        # If it is light mode, make dark
    else:
        state["theme"]["light_mode"] = False
        state["theme"]["theme_bg"] = config["theme"]["dark_bg"]
        screen.bgcolor(config["theme"]["dark_bg"])
        theme_button.config(text=config["buttons"]["theme"]["light_text"])

After the refactoring it looks like this:

def toggle_theme() -> None:
    ''' If light mode is on, switch to dark and vice versa. '''
    state["theme"]["theme_bg"] = state["theme"]["theme_bg"][::-1]

This is the state that is being changed:

state = "theme": { "theme_bg": ['dark_bg', 'light_bg'] }

Is toggle_theme() an appropriately simple and readable solution?

The function has the side effect of changing a variable in state in a non-local scope. Should this side-effect be indicated?

After fiddling for a while I settled with theme_bg as an argument of def main() and switching it directly with command:

theme_bg: List=["black", "white"]

theme_button = Button(
    button_frame, text=buttons_theme_light_text, command=screen.bgcolor(not theme_bg)
)

Answer 1

I am not sure your code is ready for review. I have some comments that you might wish to consider if you will be further updating your code. But these comments are too long and too many to be posted as actual comments. So think of this post as a pre-review for whatever it is worth. I will confine my remarks to the second code version you have:

A Minimal, Complete Example

It's okay to post minimal code that demonstrates what you are doing, but that code should be complete, i.e. it compiles and runs. You have many names in the code that are not defined, e.g. screen, List, Button, etc. How much bigger would the code be if you included some missing import statements and the statements required to create a minimal GUI? Not much - and we would be able to execute the code and see how it works.

A More Complete Docstring

It appears that you have two themes:

1. The background color is black and the button text is "☀️",
2. The background color is white and the button text is ""🌘".

Now I may have gotten that wrong and that is all the more reason why your theme_toggle docstring should give a more complete description of what it is you are trying to accomplish.

Why is theme_toggle Encapsulated Within Function main?

Some of the statements that implement your theme toggle are within function theme_toggle while others are within the enclosing main function. It would be preferable to have the definitions that are currently within main placed at global scope and function theme_toggle should also be at global scope. This would certainly make the function more widely accessible to other functions. If this code were implementing what I consider to be a theme, the implementation would be using classes (I will describe why below).

What is a Theme?

TL;DR

Feel free to skip this wordy exposition and jump to the last paragraph in bold.

The themes I have worked with apply specific styling to the various widgets in your GUI. You can generally switch from one theme to another and all you have changed is the "look and feel." So changing a button's font, for example, would be an example. But when you toggle your theme, you are actually changing the caption being displayed in a very specific button. Clearly, if you had multiple buttons on your window, you would only be changing one specific button and changing it in a way that to me seems non-standard for a theme. I would not call what you are doing "toggling a theme." Let me elaborate:

You are changing the presentation's state in a way that goes beyond styling: I consider the button's caption to be part of it's state. Whether the button is up or down would also be part of its state. Whether the button is enabled or disabled would be part of its state. But a button's color, unless it had some semantic value, would not be. A situation where the button color does represent state is if we display two different colors depending on whether the button is enabled or not. So whereas a change in theme might change the colors used to denote enabled and disabled, for any given theme currently in use a change in color represents a change in state. But changing the button's color just because you changed the theme, would not be a change in state (if it was enabled before it is still enabled). Anyway, that is the distinction I am making between state and styling that has nothing to do with state.

If you had multiple buttons and changing themes meant changing the styling of the buttons, then you might implement code that can walk through all the widgets and update the buttons it finds. Alternatively, on button creation you can add that button to a the set of all buttons in the presentation. Then it becomes a simple matter to iterate the set.

So to my way of thinking (others may disagree), you are not toggling a theme as I generally understand it. Or if you really mean to be toggling a theme, the code is not very versatile.

Update

I am sorry I gave such a verbose post, which may have been confusing. My main objection was that I normally would consider the text of a button to represent part of that button's state independent of how it is styled (font choice, color choices, etc.). So I think of a theme as changing the styling of widgets without changing its state.

But I clearly misunderstood your post and it is now clear to me that this button you are referring to is what is being used to toggle the theme. Somehow I missed that and for that I apologize. So when you toggle the theme, then it is appropriate that the button that is being used to do that and also to show what the current theme is should change its state as long as all the other widgets should only have their styling change.

So it now appears to me that the only "widget" in the presentation whose style is changing due to the toggling is just the screen itself. But consider a potentially future situation where you have lots of other widgets whose styling should be changed. You either have to explicitly add code to toggle_theme for each widget in your presentation. Every time you change the presentation adding or removing a widget, you have to update toggle_theme. That could be a maintenance nightmare. That's why I would suggest somehow tracking all widgets that are being added to a presentation by widget type in some appropriate class instances. Then toggle theme can iterate through the list of widgets and according to type change its style appropriately. I hope that makes things clearer.

But what you are still missing is a minimal, complete example that I could in principle copy and paste with no additions and run it. But given that limitation, the following is how I would encapsulate the theme toggler in a class ThemeToggler. This code is necessarily missing the needed import statements and definitions.

# import class defintions for Screen and Button (not included)

class ThemeToggler():
    """This class toggles the theme in a presentation
    simply by changing the screens color and the "text" of the
    button that when clicked triggers a call to the toggle_theme method below."""
    # What is missing is a defintion of config
    _themes = (
        {"theme_bg": config["theme"]["dark_bg"], "theme_button": "🌘"}, # dark_mode
        {"theme_bg": config["theme"]["light_bg"], "theme_button": "☀️"} # light_mode
    )
    
def __init__(self, screen: Screen, theme_button: Button):
    self._light_mode = True
    self._screen = screen
    self._theme_button = theme_button
    self._set_theme()

def _set_theme(self):
    """Update presentation according to the current mode."""
    theme = self._themes[self._light_mode]
    self._screen.bgcolor(theme['theme_bg'])
    self._theme_button.bgcolor.config(text=theme["theme_button")
    
def toggle_theme(self):
    """We must arrange to have this method called as part of event
    processing when the theme button has been clicked."""
    self._light_mode = not self._light_mode
    self._set_theme()
    
def main():
    # Build presentation defining screen and theme_button:
    ... # Most of the code is omitted
    # Create theme toggle
    theme_button = Button(
        button_frame, text=buttons_theme_text[not light_mode_on], command=theme_toggle
    )
    theme_button.pack()
    ...
    
    theme_toggler = ThemeToggler(screen, theme_button)

    # Now arrange for theme_toggler.togle_theme to be invoked when
    # theme_button is clicked:
    ...

if __name__ == '__main__':
    main()