Design for this automated menu-building method in GUI

Question

I have a github repository where I uploaded the code of a sample Python-tkinter application for illustrate how to do a GUI with i18n following the MVC pattern and good practices.

The thing is that I recently added an automated method for building a top menu in the application in a way that you just have to create a very dict with a very intuitive menu structure and put the callback handlers inside for having a menu.

The problem is that it is a very large class now and I think the design can be improved A LOT (see the file in the repo under the feature/menu branch).

I would abstract it in a couple of ways, but the handicap is that it should be fairly easy so that people could use this project to make their own tkinter-app-skeleton with their menu and having the callback handlers in the dict makes it difficult to separe the menu_struct variable from the GUIView class.

Also, the recursive function process_menu_unit() needs access to the tk.Tk() instance in GuiView.root (self.root inside the class). So we only need to re-structure self.build_menu() and the functions and variables used by self.build_menu(), which I already mentioned.

For the sake of completeness, I'm putting here the code for the main file with some comments, but I would recommend to visit the github page:

import os import sys from gettext import translation from pathlib import Path import tkinter as tk from tkinter import ttk, messagebox from common import deputils from common.constants import LOCALE_DIR from common.events.events import SetPathEvent from common.exceptions.exceptions import MvcError from common.observer import Observer from controller.controller import Controller from controller.testcontroller import MockController from view.dialogs import LayoutDialog, LanguageDialog from view.icons import IconProvider from view.widgets.tooltip import Tooltip # Simple handlers for illustrating def about_app(): messagebox.showinfo("App", "The application\nthat does nothing") def are_you_sure(event=None): if messagebox.askyesno("", "Are you sure you want to quit the App?"): messagebox.showinfo('Peche da ventá', 'Isto pecharía a ventá') # window.destroy() def open_file(): messagebox.showinfo("Open doc", "We'll open a file here...") # This dict represents the menu from the top level options to the more nested ones # I use names beginning with '__' for default tkinter properties/names that can # used with widgets to configure them # This dict comes with the callback handlers menu_struct = { 'File': { '__menu': True, 'Open': open_file, 'Open recent file...': { '__menu': True, 'file1': open_file, 'file2': open_file, }, 'Quit': { '__command': are_you_sure, '__accelerator': 'Ctrl-Q', '__underline': 1 }, }, 'Edit': { '__menu': True, 'Replace': lambda: messagebox.showinfo('App', 'Replaces everything'), 'Load file': lambda: messagebox.showinfo('App', 'Load a file...'), }, 'About': { '__command': about_app }, } def process_menu_unit(context_to_add, menu, **kwargs): """ Recursively builds the menu with the context dict received over a tkinter menu_component. With each recursive call: - the context_dict shrinks because some of its elements have been built on the menu_component - the menu_component grows and is bigger, because more of its elements have been built :param context_to_add: the context dict with the menu options with processing still pending :param menu_component: the visual tkinter menu component where more elements will be added """ if not isinstance(context_to_add, dict): return for key, value in context_to_add.items(): if not key.startswith('__'): if callable(value): menu.add_command(label=key, command=value) elif isinstance(value, dict): options = {'label': key} options_set = {opt_name[2:]: opt_value for opt_name, opt_value in value.items() if opt_name.startswith('__')} for optionkey, optionvalue in options_set.items(): if optionkey == 'menu': options['menu'] = tk.Menu(menu, tearoff=False) else: options[optionkey] = optionvalue if 'command' in options: menu.add_command(**options) elif isinstance(value, dict) and 'menu' in options: menu.add_cascade(**options) process_menu_unit(value, **options) # For brevity, I'm not putting here some code for handling dependencies class GuiView(ttk.Frame, Observer): ROW_MINSIZE = 5 COL_MINSIZE = 35 PADDING = 5 ICON_PLACE = 'right' def __init__(self, controller: Controller, *args, **kwargs): self.root = tk.Tk() # Sets the app icon self.root.tk.call('wm', 'iconphoto', self.root._w, IconProvider.get('save_disk')) #self.root.bind("<Button-1>", lambda e: self.root.destroy()) self.root.withdraw() d = LanguageDialog(self.root) self.selected_language = d.selected_language lang = translation('mvcPython', LOCALE_DIR, languages=[d.get_selected_locale()], fallback=True) lang.install() super().__init__(master=self.root, *args, **kwargs) self.controller = controller self.selected_profile = tk.StringVar() self.destiny_path = tk.StringVar() self.destiny_path.set(f'{Path.home()}') # This builds the menu! self.build_menu() # Dynamic resizing """ Col0 Col1 Col2 Col3 Col4 Row 0 ----------- Create ----------- Row 1 Profile Cmb Entry SaveTo Row 2 --- Add ---- -------------- Yscr Row 3 --- Rmv ---- ---- List ---- Yscr Row 4 -- Clear --- -------------- Yscr Row 5 ---- Xscr ---- Row 6 ---------- Collect ----------- Row 7 ----------- Text ------------- Yscr Row 8 ----------- Xscr ------------- Yscr """ rows = (0, 0, 0, 0, 0, 0, 0, 1, 0) columns = (0, 0, 1, 0, 0) for numrow, weight in enumerate(rows): self.rowconfigure(numrow, weight=weight, minsize=GuiView.ROW_MINSIZE) for numcol, weight in enumerate(columns): self.columnconfigure(numcol, weight=weight, minsize=GuiView.COL_MINSIZE) self.pack(fill=tk.BOTH, expand=tk.TRUE) # Widgets folder_img = IconProvider.get('open_folder') btn_dialog = ttk.Button(self, text=_('Open dialog'), image=folder_img, compound=GuiView.ICON_PLACE, command=self._on_btn_dialog) Tooltip(btn_dialog, text=_('this button opens a dialog')) btn_dialog.grid(row=0, column=0, columnspan=5, sticky='ew', padx=GuiView.PADDING, pady=GuiView.PADDING) ops = Controller.get_combo_options() ttk.Label(self, text=_('Simple label')).grid(row=1, column=0, padx=GuiView.PADDING, pady=GuiView.PADDING) cmb_profile = ttk.Combobox(self, values=ops, state='readonly', textvariable=self.selected_profile) Tooltip(cmb_profile, text=_('combobox for selecting values')) cmb_profile.grid(row=1, column=1, sticky='we', padx=GuiView.PADDING, pady=GuiView.PADDING) path_entry = ttk.Entry(self, textvariable=self.destiny_path, state='readonly') path_entry.grid(row=1, column=2, sticky='we', padx=GuiView.PADDING, pady=GuiView.PADDING) # We use an icon in this button save_disk_img = IconProvider.get('save_disk') btn_saveto = ttk.Button(self, image=save_disk_img, text=_('Save'), compound=GuiView.ICON_PLACE, command=self._on_open_save_dialog) Tooltip(btn_saveto, text=_('button for saving things')) btn_saveto.grid(row=1, column=3, columnspan=2, sticky='we', padx=GuiView.PADDING, pady=GuiView.PADDING) btn_add_values = ttk.Button(self, image=folder_img, compound=GuiView.ICON_PLACE, text=_('Add value'), command=self._on_open_collectd_dialog) Tooltip(btn_add_values, text=_('button for add values')) btn_add_values.grid(row=2, column=0, columnspan=2,sticky='nswe', padx=GuiView.PADDING, pady=GuiView.PADDING) garbage_bin_img = IconProvider.get('garbage_bin') btn_remove_metric_paths = ttk.Button(self, image=garbage_bin_img, compound=GuiView.ICON_PLACE, text=_('Remove values'), command=self._on_remove_collectd_selection) Tooltip(btn_remove_metric_paths, text=_('this button removes values')) btn_remove_metric_paths.grid(row=3, column=0, columnspan=2, sticky='nswe', padx=GuiView.PADDING, pady=GuiView.PADDING) btn_clear_metric_paths = ttk.Button(self, image=garbage_bin_img, compound=GuiView.ICON_PLACE, text=_('Clear'), command=self._on_clear_collectd_selection) Tooltip(btn_clear_metric_paths, text=_('this button clears all')) btn_clear_metric_paths.grid(row=4, column=0, columnspan=2, sticky='nswe', padx=GuiView.PADDING, pady=GuiView.PADDING) # Listbox with dynamic vertical scroll xscr = ttk.Scrollbar(self, orient=tk.HORIZONTAL) xscr.grid(row=5, column=2, rowspan=1, columnspan=2, sticky='WE') yscr = ttk.Scrollbar(self, orient=tk.VERTICAL) yscr.grid(row=2, rowspan=3, column=4, sticky='NS') self.lst_path = tk.Listbox(self, height=5, xscrollcommand=xscr.set, yscrollcommand=yscr.set) self.lst_path.grid(row=2, column=2, rowspan=3, columnspan=2, sticky='nswe', padx=GuiView.PADDING, pady=GuiView.PADDING) xscr['command'] = self.lst_path.xview yscr['command'] = self.lst_path.yview metrics_img = IconProvider.get('metrics') btn_collect = ttk.Button(self, text=_('Do things'), image=metrics_img, compound=GuiView.ICON_PLACE, command=self._on_collect_metrics) Tooltip(btn_collect, text=_('this button gets the stuff done')) btn_collect.grid(row=6, column=0, columnspan=5, sticky='we', padx=GuiView.PADDING, pady=GuiView.PADDING) # Text widget with dynamic scroll xscr = ttk.Scrollbar(self, orient=tk.HORIZONTAL) xscr.grid(row=8, column=0, columnspan=4, sticky='WE') yscr = ttk.Scrollbar(self, orient=tk.VERTICAL) yscr.grid(row=7, column=4, sticky='NS') self.output = tk.Text(self, height=8, yscrollcommand=yscr.set, xscrollcommand=xscr.set) self.output.configure(state=tk.DISABLED) self.output.grid(row=7, column=0, columnspan=4, sticky='nswe', padx=GuiView.PADDING, pady=GuiView.PADDING) yscr['command'] = self.output.yview xscr['command'] = self.output.xview def build_menu(self): main_menu = tk.Menu(self.root) self.root.config(menu=main_menu) process_menu_unit(menu_struct, **{'menu': main_menu}) def init_ui(self): """ Shows the UI """ self.root.title(_('MyProject')) self.root.iconify() self.root.update() self.root.deiconify() self.root.minsize(self.root.winfo_width(), self.root.winfo_height()) self.root.mainloop() def start(self): """ Shows the already initialized and built UI. __init__ should be called previously """ self.init_ui() def refresh(self, value): msg = '\n' if isinstance(value, str): msg += value elif isinstance(value, MvcError): msg += value.messages[0] messagebox.showerror('Error', msg) elif isinstance(value, SetPathEvent): self.destiny_path.set(value.info) if msg.strip(): self.output.config(state=tk.NORMAL) self.output.insert(tk.END, msg) self.output.config(state=tk.DISABLED) def _on_collect_metrics(self): """ Handles event when the collect metrics button is pressed :param __: the event """ self.controller.collect_metrics(self.destiny_path.get(), self.lst_path.get(0, tk.END), self.selected_profile.get()) def _on_open_save_dialog(self, __=None): """ Handles event when the save button is pressed :param __: the event """ self.destiny_path.set( tkfilebrowser.askopendirname(initialdir=self.destiny_path.get(), title=_('Please, save file on your desired path')) ) def _on_open_collectd_dialog(self, __=None): """ Handles event when the add metrics button is pressed :param __: the event """ initial_dir = '' dest_path = self.destiny_path.get() if dest_path and dest_path.strip(): initial_dir = os.path.dirname(dest_path) upper_selected_profile = self.selected_profile.get().upper() top_title_msg = _('Profile {}').format(upper_selected_profile) collectd_paths = tkfilebrowser.askopendirnames(initialdir=initial_dir, title=top_title_msg) for p in collectd_paths: self.lst_path.insert(tk.END, p) def _on_remove_collectd_selection(self, __=None): """ Handles event when the remove metrics path button is pressed :param __: the event """ for idx in self.lst_path.curselection(): self.lst_path.delete(idx) def _on_clear_collectd_selection(self, __=None): """ Handles event when the clear button is pressed :param __: the event """ self.lst_path.delete(0, tk.END) def _on_btn_dialog(self, __=None): """ Handles event when the clear button is pressed :param __: the event """ try: d = LayoutDialog(self) except MvcError as ge: self.refresh(ge) class TerminalView(Observer): def __init__(self, controller: Controller, *args, **kwargs): self.controller = controller lang = translation('mvcPython', LOCALE_DIR, languages=['en'], fallback=True) lang.install() def start(self): """ Shows the prompt. """ while True: cmd_in = input('> ') if cmd_in in self.controller.__class__.__dict__ and callable(self.controller.__class__.__dict__[cmd_in]): self.controller.__class__.__dict__[cmd_in](self.controller) def refresh(self, value): print(value) if __name__ == '__main__': v = GuiView(MockController()) v.init_ui() 

Answer 1

variants

With each recursive call:

• the context_dict shrinks because some of its elements have been built on the menu_component
• the menu_component grows and is bigger, because more of its elements have been built

Thank you for stating these explicitly; I appreciate it. It helps with understanding the function's responsibilities, and helps with any potential debugging session.

I wouldn't have minded seeing optional type annotation up in the signature, though.

wrong input

 if not isinstance(context_to_add, dict): return 

The docstring was pretty clear: this shall be a dict. If we return, it appears to me it would be due to caller error. Consider raising fatal exception here, so the buggy caller will get fixed.

With annotations and $ mypy --strict *.py, presumably the bad calling code would have been highlighted by the linter before we even get to the point of running the code.

extra __menu entry

 if callable(value): menu.add_command( ... ) 

I like the DSL defined by menu_struct, and I immediately found '__menu': True to be quite clear. However, since a menu must contain callable commands, this test suggests to me that perhaps we could dispense with '__menu' entries. It would be a matter of making this library slightly more complex, in order to simplify things for the various callers, removing some redundancy. Also, hopefully there's a whole class of potential calling mistakes which become impossible to mess up, after such simplification.

You have some very simple tests for 'command' in options and 'menu' in options, which I think could be replaced by a pair of helpers. The helper could perform those simple tests, but alternatively it could scan through the values to see if a menu or a command is intended.

 '__underline': 1 

I confess I found that a little surprising. I was expecting to maybe see a value of True there.

Oohhhh, it becomes optionvalue in this assignment: options[optionkey] = optionvalue. I guess using 1 is a win here, so you can just point folks at the tkinter documentation, ok. But if caller offers True, maybe run it through int() as a convenience? Or maybe tk accepts bool just fine already.

I encourage you to think about user mistakes. If caller's dict doesn't exactly match the intended format, you will want to report an error so we know we're losing. Better, offer a diagnostic error message, guiding the hapless user down a path that will let them win. Write automated tests that verify a couple of bad dicts produce the intended diagnostic messages.

leftover code

Those four manifest constants are very clear, thank you.

 #self.root.bind("<Button-1>", lambda e: self.root.destroy()) 

It's review time, so a phase of development is done. Time to delete commented code. You can always get it back with git if you need it later.

nit, typo: I think you wanted a "destination" path rather than self.destiny_path.

raw string

 error_msg += f"\nC:\> pip install pywin32" 

You meant to type fr"..." rather than f"..." there. A sequence like \t is valid and means TAB, but a sequence like \> is not valid, so use a raw string to properly express your intent.

Actually, there's no variable interpolation, so a simple r"..." would suffice.

extra method

 def start(self): """ Shows the already initialized and built UI. __init__ should be called previously """ self.init_ui() 

The "called previously" comment is redundant, since it's very difficult to accidentally skip the constructor.

And this whole method appears to be redundant with init_ui, and may be deleted. Consider renaming that other method to be start, if that's the public interface you want your API to offer callers.

Invoking root.mainloop() is perhaps a little adventurous, as it blocks. You might offer callers a little more flexibility by omitting it, and making it caller's responsibility to run the main loop when they're ready. For example, caller might have more than one window to worry about.

structure

very large class

Sure, it keeps growing. Break your window into three or four sections and create one new module per section, like this:

1. Open Dialog
2. row with Simple Label ... Save
3. Add, Remove, Clear, along with textbox
4. Do Things, along with textbox

Maybe lump 1 and 2 together, whatever makes sense to you.

This will force you to break out each init + callback code into new module. Your goal should be to have low coupling. That is, the __init__ constructor winds up with a ton of transient local variables like btn_add_values, and also with a bunch of permanent object attributes like self.output. To the extent feasible, try to bury such variables so code in a central position doesn't even have to know about them. Only the Open Dialog module should know about filespec details, that sort of thing.