2
\$\begingroup\$

I'm currently halfway through a Python project, but would like a review before I spend too much more time on this to avoid going too far down a rabbit hole.

I had previously wanted to use HTML, CSS, and JavaScript, but I swapped to Python so I can get the current Window's user while opening my app.

At my work, we have labels that look like this:

demo paper label

We print out this paper and put a label on the left-hand side, and then get written sign-off on it from various stakeholders. The labels are for products such as seeds, nuts, beans, etc. The specific design and layout of the label will vary based on our customer.

Current, paper workflow:

  1. The originator creates the form and hands it to the managers who need to sign off
  2. The managers initial (the two rightmost columns) to show that they have signed off. Sometimes only person needs to sign off, sometimes two.
  3. Once everything is done, someone will sign and date the label to verify that the label is good to go.

With the new Python application, I would like to be able to accomplish the same workflow without ever needing the physical piece of paper being passed around.

Here is my current (but incomplete) code:

from tkinter import *
import glob
import os
from PIL import Image, ImageTk, ImageGrab
import tkinter as tk
import datetime

#date & time
now = datetime.datetime.now()

root = tk.Tk()
root.title("SIGN OFF")
root.minsize(840, 800)

# Add a grid
mainframe = tk.Frame(root)
mainframe.columnconfigure(0, weight=1)
mainframe.rowconfigure(0, weight=1)
mainframe.pack(pady=100, padx=100)

# Create a Tkinter variable
tkvar = tk.StringVar(root)


# Directory
directory = "C:/Users/eduards/Desktop/work/data/to-do"
choices = glob.glob(os.path.join(directory, "*.jpg"))
tkvar.set('...To Sign Off...') # set the default option

# Dropdown menu
popupMenu = tk.OptionMenu(mainframe, tkvar, *choices)
tk.Label(mainframe, text="Choose your sign off here:").grid(row=1, column=1)
popupMenu.grid(row=2, column=1)

label2 = tk.Label(mainframe, image=None)
label2.grid(row = 4, column = 1, rowspan = 10)

# On change dropdown callback.
def change_dropdown(*args):
    """ Updates label2 image. """
    imgpath = tkvar.get()
    img = Image.open(imgpath)
    img = img.resize((240,250))
    photo = ImageTk.PhotoImage(img)
    label2.image = photo
    label2.configure(image=photo)


tk.Button(mainframe, text="Open", command=change_dropdown).grid(row=3, column=1)


def var_states():
    text_file = open("logfile.txt", "a")
    text_file.write("TIME: %s, USER: %s, One %d, Two %d\n" % (now,os.getlogin(), var1.get(), var2.get()))
    text_file.close()
    print("One %d, Two %d" % (var1.get(), var2.get()))

var1 = IntVar()
Checkbutton(mainframe, text="Ingredients present in full (any allergens in bold with allergen warning if necessary)", variable=var1).grid(column = 2, row=1, sticky=W)
var2 = IntVar()
Checkbutton(mainframe, text="May Contain Statement.", variable=var2).grid(column = 2, row=2, sticky=W)
var3 = IntVar()
Checkbutton(mainframe, text="Cocoa Content (%).", variable=var3).grid(column = 2, row=3, sticky=W)
var4 = IntVar()
Checkbutton(mainframe, text="Vegetable fat in addition to Cocoa butter", variable=var4).grid(column = 2, row=4, sticky=W)
var5 = IntVar()
Checkbutton(mainframe, text="Instructions for Use.", variable=var5).grid(column = 2, row=5, sticky=W)
var6 = IntVar()
Checkbutton(mainframe, text="Additional warning statements (pitt/stone, hyperactivity etc)", variable=var6).grid(column = 2, row=6, sticky=W)
var7 = IntVar()
Checkbutton(mainframe, text="Nutritional Information Visible", variable=var7).grid(column = 2, row=7, sticky=W)
var8 = IntVar()
Checkbutton(mainframe, text="Storage Conditions", variable=var8).grid(column = 2, row=8, sticky=W)
var9 = IntVar()
Checkbutton(mainframe, text="Best Before & Batch Information", variable=var9).grid(column = 2, row=9, sticky=W)
var10 = IntVar()
Checkbutton(mainframe, text="Net Weight & Correct Font Size.", variable=var10).grid(column = 2, row=10, sticky=W)
var11 = IntVar()
Checkbutton(mainframe, text="Barcode - Inner", variable=var11).grid(column = 2, row=11, sticky=W)
var12 = IntVar()
Checkbutton(mainframe, text="Address & contact details correct", variable=var12).grid(column = 2, row=12, sticky=W)

def user():
    user_input = os.getlogin()
    tk.Label(mainframe, text = user_input, font='Helvetica 18 bold').grid(row = 0, column = 1)


user()


def save():
    # pyautogui.press('alt')
    # pyautogui.press('printscreen')
    # img = ImageGrab.grabclipboard()
    # img.save('paste.jpg', 'JPEG')

    var_states()


tk.Button(mainframe, text = "Save", command = save).grid(row = 20, column = 1)


root.mainloop()

Here is the output:

digitized label

The top part in bold (Label in the sample image) is the current Windows user's username. The dropdown menu below that is the list of JPG files that need signing (TODO: structure the files to only show specific file names that need signing).

When you click "Save", a log file will be generated that shows your timestamped choices (TODO: add code to return which image was touched).

I've also realized that this currently only supports a single person signing, but I will need to restructure to support allowing multiple (currently up to 2) people to sign it.

Below is the file system for labels in JPG. Customer account number which is unique, as an example SFDG001 is one customer and ALPI001 is another. 2nd is the product code, each customer takes different product codes and never will have the same product code. (I know the images look the same, but its for test purposes)

sample JPGs that would need signing

UPDATE 26/09/2019 - NEW CODE

here is my updated code:

from tkinter import *
from tkinter import DISABLED
import tkinter.ttk as ttk
import os
import glob
from PIL import Image, ImageTk, ImageGrab
from pathlib import Path

# from openpyxl import load_workbook
class App():
    def __init__(self,master):



        notebook = ttk.Notebook(master)
        notebook.pack(expand = 1, fill = "both")

        #Frames
        main = ttk.Frame(notebook)
        manual = ttk.Frame(notebook)

        notebook.add(main, text='Main-Screen')
        notebook.add(manual, text='Manual')



        #Check boxes
        #Assigning Integers to variables
        var1 = IntVar()
        var1a = IntVar()
        var2 = IntVar()
        var2a = IntVar()
        var3 = IntVar()
        var3a = IntVar()
        var4 = IntVar()
        var4a = IntVar()
        var5 = IntVar()
        var5a = IntVar()
        var6 = IntVar()
        var6a = IntVar()
        var7 = IntVar()
        var7a = IntVar()
        var8 = IntVar()
        var8a = IntVar()
        var9 = IntVar()
        var9a = IntVar()
        var10 = IntVar()
        var10a = IntVar()
        var11 = IntVar()
        var11a = IntVar()
        var12 = IntVar()
        var12a = IntVar()

        #Text boxes for initials

        #Displaying checkboxes and assigning to variables
        self.Checkbox1 = Checkbutton(main, text="Ingredients present in full (any allergens in bold with allergen warning if necessary)", variable=var1)
        self.Checkbox1.grid(column = 2, row = 1, sticky = W)
        self.Checkbox2 = Checkbutton(main, variable = var1a)
        self.Checkbox2.grid(column = 1, row = 1, sticky = W)

        self.Checkbox3 = Checkbutton(main, text="May Contain Statement.", variable=var2)
        self.Checkbox3.grid(column = 2, row = 2, sticky = W)
        self.Checkbox4 = Checkbutton(main, variable = var2a)
        self.Checkbox4.grid(column = 1, row = 2, sticky = W)

        self.Checkbox5 = Checkbutton(main, text="Cocoa Content (%).", variable=var3)
        self.Checkbox5.grid(column = 2, row = 3, sticky = W)
        self.Checkbox6 = Checkbutton(main, variable = var3a)
        self.Checkbox6.grid(column = 1, row = 3, sticky = W)

        self.Checkbox7 = Checkbutton(main, text="Vegetable fat in addition to Cocoa butter", variable=var4)
        self.Checkbox7.grid(column = 2, row = 4, sticky = W)
        self.Checkbox8 = Checkbutton(main, variable = var4a)
        self.Checkbox8.grid(column = 1, row = 4, sticky = W)

        self.Checkbox9 = Checkbutton(main, text="Instructions for Use.", variable=var5)
        self.Checkbox9.grid(column = 2, row = 5, sticky = W)
        self.Checkbox10 = Checkbutton(main, variable = var5a)
        self.Checkbox10.grid(column = 1, row = 5, sticky = W)

        self.Checkbox11 = Checkbutton(main, text="Additional warning statements (pitt/stone, hyperactivity etc)", variable=var6)
        self.Checkbox11.grid(column = 2, row = 6, sticky = W)
        self.Checkbox12 = Checkbutton(main, variable = var6a)
        self.Checkbox12.grid(column = 1, row = 6, sticky = W)

        self.Checkbox13 = Checkbutton(main, text="Nutritional Information Visible", variable=var7)
        self.Checkbox13.grid(column = 2, row = 7, sticky = W)
        self.Checkbox14 = Checkbutton(main, variable = var7a)
        self.Checkbox14.grid(column = 1, row = 7, sticky = W)

        self.Checkbox15 = Checkbutton(main, text="Storage Conditions", variable=var8)
        self.Checkbox15.grid(column = 2, row = 8, sticky = W)
        self.Checkbox16 = Checkbutton(main, variable = var8a)
        self.Checkbox16.grid(column = 1, row = 8, sticky = W)

        self.Checkbox17 = Checkbutton(main, text="Best Before & Batch Information", variable=var9)
        self.Checkbox17.grid(column = 2, row = 9, sticky = W)
        self.Checkbox18 = Checkbutton(main, variable = var9a)
        self.Checkbox18.grid(column = 1, row = 9, sticky = W)

        self.Checkbox19 = Checkbutton(main, text="Net Weight & Correct Font Size.", variable=var10)
        self.Checkbox19.grid(column = 2, row = 10, sticky = W)
        self.Checkbox20 = Checkbutton(main, variable = var10a)
        self.Checkbox20.grid(column = 1, row = 10, sticky = W)

        self.Checkbox21 = Checkbutton(main, text="Barcode - Inner", variable=var11)
        self.Checkbox21.grid(column = 2, row = 11, sticky = W)
        self.Checkbox22 = Checkbutton(main, variable = var11a)
        self.Checkbox22.grid(column = 1, row = 11, sticky = W)

        self.Checkbox23 = Checkbutton(main, text="Address & contact details correct", variable=var12)
        self.Checkbox23.grid(column = 2, row = 12, sticky = W)
        self.Checkbox24 = Checkbutton(main, variable = var12a)
        self.Checkbox24.grid(column = 1, row = 12, sticky = W)


        ##DISABLE ON CLICK##
        def showstate(*args):
            if var1.get() or var2.get() or var3.get() or var4.get() or var5.get() or var6.get() or var7.get() or var8.get() or var9.get() or var10.get() or var11.get() or var12.get():
                self.Checkbox2.config(state = DISABLED)
                self.Checkbox4.config(state = DISABLED)
                self.Checkbox6.config(state = DISABLED)
                self.Checkbox8.config(state = DISABLED)
                self.Checkbox10.config(state = DISABLED)
                self.Checkbox12.config(state = DISABLED)
                self.Checkbox14.config(state = DISABLED)
                self.Checkbox16.config(state = DISABLED)
                self.Checkbox18.config(state = DISABLED)
                self.Checkbox20.config(state = DISABLED)
                self.Checkbox22.config(state = DISABLED)
                self.Checkbox24.config(state = DISABLED)
            if var1a.get() or var2a.get() or var3a.get() or var4a.get() or var5a.get() or var6a.get() or var7a.get() or var8a.get() or var9a.get() or var10a.get() or var11a.get() or var12a.get():
                self.Checkbox1.config(state = DISABLED)
                self.Checkbox3.config(state = DISABLED)
                self.Checkbox5.config(state = DISABLED)
                self.Checkbox7.config(state = DISABLED)
                self.Checkbox9.config(state = DISABLED)
                self.Checkbox11.config(state = DISABLED)
                self.Checkbox13.config(state = DISABLED)
                self.Checkbox15.config(state = DISABLED)
                self.Checkbox17.config(state = DISABLED)
                self.Checkbox19.config(state = DISABLED)
                self.Checkbox21.config(state = DISABLED)
                self.Checkbox23.config(state = DISABLED)

        var1.trace_variable("w", showstate)
        var1a.trace_variable("w", showstate)
        var2.trace_variable("w", showstate)
        var2a.trace_variable("w", showstate)
        var3.trace_variable("w", showstate)
        var3a.trace_variable("w", showstate)
        var4.trace_variable("w", showstate)
        var4a.trace_variable("w", showstate)
        var5.trace_variable("w", showstate)
        var5a.trace_variable("w", showstate)
        var6.trace_variable("w", showstate)
        var6a.trace_variable("w", showstate)
        var7.trace_variable("w", showstate)
        var7a.trace_variable("w", showstate)
        var8.trace_variable("w", showstate)
        var8a.trace_variable("w", showstate)
        var9.trace_variable("w", showstate)
        var9a.trace_variable("w", showstate)
        var10.trace_variable("w", showstate)
        var10a.trace_variable("w", showstate)
        var11.trace_variable("w", showstate)
        var11a.trace_variable("w", showstate)
        var12.trace_variable("w", showstate)
        var12a.trace_variable("w", showstate)

        ##DISABLE ON CLICK##


        #Send data
        def var_states():
            text_file = open("logfile.txt", "a")
            text_file.write("USER: %s, One %d\n" % (os.getlogin(), var1.get()))
            text_file.close()

        self.dataSend = Button(main, text = "Send", command = var_states).grid(column = 1, row = 13, sticky = W)

###################################################################################################################################
                                                        ##Load Image##
###################################################################################################################################

        # Create a Tkinter variable
        tkvar = StringVar(root)


        # Directory
        directory = "//SERVER/shared_data/Technical/Label Sign Off Sheets/sign off project"
        choices = glob.glob(os.path.join(directory, "*- to sign.jpg"))
        tkvar.set('...To Sign Off...') # set the default option

        # Images
        def change_dropdown():
            imgpath = tkvar.get()
            img = Image.open(imgpath)
            img = img.resize((529,361))
            photo = ImageTk.PhotoImage(img)
            label2.image = photo
            label2.configure(image=photo)

        #return path value
        p = None

        def func(value):
            global p
            p = Path(value)
            print('req:', p)

        #widgets
        self.msg1 = Label(main, text = "Choose here")
        self.msg1.grid(column = 0, row = 0)

        self.popupMenu = OptionMenu(main, tkvar, *choices)
        self.popupMenu.grid(row=1, column=0)

        self.display_label = label2 = Label(main, image=None)
        self.display_label.grid(row=2, column=0, rowspan = 500)

        self.open_button = Button(main, text="Open", command=change_dropdown)
        self.open_button.grid(row=502, column=0)



###################################################################################################################################
                                                        ##Tab 2 - Manual##
###################################################################################################################################

        def open_doc():
            os.system("start C:/Users/Eduards/Desktop/Portfolio")

        self.Manual_Button = Button(manual, text = "Open Manual", command = open_doc)
        self.Manual_Button.pack()





root = Tk()
root.minsize(950, 450)
root.title("SIGN OFF LABELS")
app = App(root)
root.mainloop()

Have added another checkboxes, if one side is clicked, the other side is disabled.

Added tabs, need to work on manual, to open a docx file.

Next, I will concentrate on adding a log in window for users to log in.

\$\endgroup\$
  • 3
    \$\begingroup\$ Welcome to Code Review! I've edited your question to more accurately describe your topic, and clarify some of your points. That being said, as currently written your question is off-topic. We require code to be completely implemented before being submitted for review. Are you able to restructure your code/question to be complete as-is, instead of including the elements that aren't doing what you want? \$\endgroup\$ – Dannnno Sep 18 at 20:08
  • \$\begingroup\$ Hi Dannnno, I give you final code by the end of the week hopefully, or start of next week (Monday Uk time) \$\endgroup\$ – 98Ed Sep 18 at 20:26
  • \$\begingroup\$ Also, thanks for the edit! I will keep that structure in mind and use it in later cases! \$\endgroup\$ – 98Ed Sep 18 at 20:27
5
\$\begingroup\$

Fix your imports of tkinter

You are importing tkinter twice, once with a wildcard and once "as tk". You should not be using the global import at all (see PEP9). Stick with a single import:

import tkinter as tk

There will be places in your code that need to be modified to account for this. For example, change all instance of Checkbutton to tk.Checkbutton.

Separate widget creation from widget layout

Don't write code like tk.Label(...).grid(...). Separate your widget creation from widget layout. It makes your code easier to read, and makes the layout easier to visualize. Plus, when you need to keep a reference to a widget, you won't trip over the common problem of having variables set to None because .grid(...) and .pack(...) return None.

For example:

choose_label = tk.Label(mainframe, text="Choose your sign off here:")
popupMenu = tk.OptionMenu(mainframe, tkvar, *choices)
other_label = tk.Label(mainframe, image=None)
open_button = tk.Button(mainframe, text="Open", command=change_dropdown)

choose_label.grid(row=1, column=1)
popupMenu.grid(row=2, column=1)
open_button.grid(row=3, column=1)
other_label.grid(row=4, column=1)

This makes it much easier to see which widgets are grouped together and how they are arranged on screen.

As a rule of thumb, I always create all widgets that share the same parent as a group, and then lay them out as a group. That way I don't have to hunt through all of the code trying to find widgets that are arranged together.

Organize your widgets

You've put everything in mainframe. However, looking at your UI design you clearly have different sections to the UI. Have your code reflect those different sections.

For example, you seem to have a left half and a right half to the GUI, and their layout needs are somewhat different. On the right is just a list of checkbuttons that are all aligned to the left. On the left is a more complex layout with different widgets where everything is centered. Also, the items on the left take up less space than the items on the right.

I recommend that you start the GUI by creating two frames, one for the left and one for the right.

left_frame = tk.Frame(...)
right_frame = tk.Frame(...)

You can then use pack to lay them out side-by-side, or use a paned window, or use grid. In this specific case I would choose pack simply because you don't have to worry about row and column weights.

For example, this causes each to be given half of the available free space in the window:

left_frame.pack(side="left", fill="both", expand=True)
right_frame.pac(side="right", fill="both", expand=True)

Next, focus on just one side of the UI. For example, all of the widgets on the left would be a child of left_frame:

choose_label = tk.Label(left_frame, text="Choose your sign off here:")
popupMenu = tk.OptionMenu(left_frame, tkvar, *choices)
other_label = tk.Label(left_frame, image=None)
open_button = tk.Button(left_frame, text="Open", command=change_dropdown)
save_button = tk.Button(left_frame, text = "Save", command = save)

Because these are all in a common frame, and separate from the widgets in the other frame, you are free to use pack, grid, or place. If you use grid, you don't have to worry about how the size of rows on the left affect the appearance of objects on the right.

Next, focus on the widgets on the right, following the same pattern: create the widgets as children of the right frame, and then lay them out using whatever layout manager works best.

Organize your code

You have code that looks like this, which is very hard to read:

var1 = IntVar()
Checkbutton(mainframe, text="Ingredients present in full (any allergens in bold with allergen warning if necessary)", variable=var1).grid(column = 2, row=1, sticky=W)
var2 = IntVar()
Checkbutton(mainframe, text="May Contain Statement.", variable=var2).grid(column = 2, row=2, sticky=W)
...

Instead, do one of two things. First, you can separate your data definitions (var1 = IntVar()) from your widget definition. For example:

var1 = IntVar()
var2 = IntVar()

Checkbutton(mainframe, text="Ingredients present in full (any allergens in bold with allergen warning if necessary)", variable=var1).grid(column = 2, row=1, sticky=W)
Checkbutton(mainframe, text="May Contain Statement.", variable=var2).grid(column = 2, row=2, sticky=W)

A better solution would be to use a data structure that lets you create these widgets and variables in a loop. By doing that, if you decide at a future date to change the look of a widget, you only have to change one or two lines of code rather than dozens.

For example, assuming you've created a separate frame just for the checkbuttons (eg: right_frame), it might look like this:

required_info = [
    "Ingredients present in full ...",
    "May Contain Statement",
    "Cocoa Content (%)",
    "...",
    ]


vars = []
for info in required_info:
    var = IntVar(value=0)
    vars.append(var)
    cb = tk.Checkbutton(right_frame, text=info, variable=var, onvalue=1, offvalue=0, justify="left")
    cb.pack(side="top", fill="x")

With that, to add another required piece of info you only have to add a single line to the required_info array, rather than two or three lines of code. Plus, it makes it trivial to rearrange the order of the items since you only have to reorder the list rather than the code

To get the values, you can then just iterate over the list of vars:

for var in vars:
    print(var.get())

You can even use the required information as the name of the widget:

for info in required_info:
    var = IntVar(value=0, name=info)
    ...

...
for var in vars:
    print("{} = {}".format(str(var), var.get()))

Use classes

In my experience, tkinter is much easier to maintain if you use classes. At the very least, I recommend using a single class for the whole application, if for no other reason than it lets you specify widgets that use callbacks before having to define the callback, leaving your main logic near the top of the file.

For example, instead of this:

<define some widgets>
def change_dropdown(*args): ...
<define more widgets>
def var_states(): ...
<define more widgets>
def user(): ...
user()
def save(): ...
<define more widgets>
root.mainloop()

... you could have this, which is considerably easier to read:

class App():
    def __init__(self):
        <define all widgets>
    def change_dropdown(self, *args): ...
    def var_states(self): ...
    def user(self): ...
    def save(self): ...

app = App()
app.root.mainloop()
\$\endgroup\$
  • \$\begingroup\$ Awesome! I will update my code using this as my guide and learn from it! Will put my new code as answer and mention you in it. Thanks! \$\endgroup\$ – 98Ed Sep 19 at 17:40
  • \$\begingroup\$ Love that loop idea for the checkboxes \$\endgroup\$ – 98Ed Sep 19 at 19:28
  • \$\begingroup\$ for this for var in vars: print("{} = {}".format(str(var), var.get())) How can i get the required info text to appear instead of PY_VAR1? \$\endgroup\$ – 98Ed Sep 19 at 19:41
  • \$\begingroup\$ @98Ed: var = IntVar(value=0, name=info). Notice the name= parameter. \$\endgroup\$ – Bryan Oakley Sep 19 at 19:42
  • \$\begingroup\$ kinda confused, I am trying to re-code the var_states function, should I be using this at all?.. \$\endgroup\$ – 98Ed Sep 19 at 19:47
2
\$\begingroup\$
  • The label images should be stored as .png files, not .jpeg. Use .png when you have non-photographic graphics, such as line drawings and text. Use .jpeg for photographic images.

  • Taking the snapshot of the application can be done without using a screenshot. See this question for details.

  • You may wish to revisit the web-application concept: it is simple to forward a Windows Kerberos authentication to a website; all major browsers support it. If your organization has computers joined to Active Domain, the authentication is pretty much already done for you. See for example this page for details of browser configuration. The web server can use e.g. mod_auth_gssapi module to accept Kerberos authentication forwarded by the user's browser. It can also fall back to manual user + password authentication. The web server can leverage an Active Domain for authentication.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.