5
\$\begingroup\$

Update from Digitizing paper label system with Python

I have implemented a username and password log in and main application to be used after successful log in.

I am ready for some more feedback.

My main troubles is error handling. For example log in screen, I'll need to implement statement to show if user does not exist etc.. Nothing too fancy. Also, maybe add a scroll bar and center the screen for both classes when opened up.

Also, if there is nothing inside a folder, the drop down menu dissapears and shows nothing, I'll need to work on that too. Maybe pop another screen saying 'No files in folder, come back later'.

Here is my full code:

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

def main():
    root = Tk()
    app = Window1(root)
    root.mainloop()

class Window1:
    def __init__(self,master):
        self.master = master
        self.master.title("User Log In")
        self.master.geometry('400x150')
        self.frame = Frame(self.master)
        self.frame.pack(fill="both", expand=True)


        self.label_username = Label(self.frame, text="Username: ",font=("bold",16))
        self.entry_username = Entry(self.frame, font = ("bold", 14))
        self.label_password = Label(self.frame, text="Password: ",font=("bold",16))
        self.entry_password = Entry(self.frame, show="*", font = ("bold", 14))

        self.label_username.pack()
        self.entry_username.pack()
        self.label_password.pack()
        self.entry_password.pack()

        self.logbtn = Button(self.frame, text="Login", font = ("bold", 10), command=self._login_btn_clicked)
        self.logbtn.pack()

    #close and stop tkinter running in backround, also see line #64
    def on_closing(self):
        self.master.destroy()


    def _login_btn_clicked(self):
        # print("Clicked")
        username = self.entry_username.get()
        password = self.entry_password.get()

        # print(username, password)
        account_list = [line.split(":", maxsplit=1) for line in open("passwords.txt")]
        # list of 2-tuples. Usersnames with colons inside not supported.
        accounts = {key: value.rstrip() for key, value in account_list}
        # Convert to dict[username] = password, and slices off the line ending.
        # Does not support passwords ending in whitespace.
        if accounts[username] == password:
            self.master.withdraw()
            self.newWindow = Toplevel(self.master)
            self.newWindow.protocol("WM_DELETE_WINDOW", self.on_closing)
            self.app = Window2(self.newWindow, window1 = self)
        else:
            messagebox.showinfo("User message", "Invalid username or password specified please try again")


class Window2:
    def __init__(self,master, window1):
        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')
        self.window1 = window1


        def clock():
            t=time.strftime('%d/%m/%Y, %H:%M:%S, ',time.localtime())
            if t!='':
                self.display_time.config(text=t,font='times 15')
            main.after(100,clock)
        self.display_time=Label(main)
        self.display_time.grid(column = 3, row = 0)
        clock()


        username = self.window1.entry_username.get()
        self.User = Label(main, text = 'User: '+ username, font = ('15'))
        self.User.grid(column = 4, row = 0)


        var1 = IntVar()
        var2 = IntVar()
        var3 = IntVar()
        var4 = IntVar()
        var5 = IntVar()
        var6 = IntVar()
        var7 = IntVar()
        var8 = IntVar()
        var9 = IntVar()
        var10 = IntVar()
        var11 = IntVar()
        var12 = IntVar()

        #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, text="May Contain Statement.", variable=var2)
        self.Checkbox2.grid(column = 2, row = 2, sticky = W)

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

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

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

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

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

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

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

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

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

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


        #PrintScreen
        def PrintScreen():
            pyautogui.keyDown('alt')
            pyautogui.keyDown('printscreen')
            pyautogui.keyUp('printscreen')
            pyautogui.keyUp('alt')
            self.dataSend['state'] = 'normal'

        def var_states():
            text_file = open("logfile.txt", "a")
            text_file.write("Username: [%s], option 1: [%d], option 2: [%d], option 3: [%d], option 4: [%d], option 5: [%d], option 6: [%d], option 7: [%d], option 8: [%d], option 9: [%d], option 10: [%d], option 11: [%d], option 12: [%d], Original Sign Off: [%s]\n" % (username,var1.get(),  var2.get(),  var3.get(),  var4.get(),  var5.get(),  var6.get(),  var7.get(),  var8.get(),  var9.get(),  var10.get(),  var11.get(),  var12.get(), self.p))
            text_file.close()
            self.img = ImageGrab.grabclipboard()
            self.img.save("%s" % ('//SERVER/shared_data/Technical/Label Sign Off Sheets/sign off project/Signed/'+ os.path.basename(self.p).strip('- to sign.jpg') + ' ' + username+ '.jpg'), 'JPEG')
            ed = ('//SERVER/shared_data/Technical/Label Sign Off Sheets/sign off project/Signed/'+ os.path.basename(self.p).strip('- to sign.jpg') +' ed'+ '.jpg')
            Nb = ('//SERVER/shared_data/Technical/Label Sign Off Sheets/sign off project/Signed/'+ os.path.basename(self.p).strip('- to sign.jpg') +' Nb'+ '.jpg')
            jj = ('//SERVER/shared_data/Technical/Label Sign Off Sheets/sign off project/Signed/'+ os.path.basename(self.p).strip('- to sign.jpg') +' jj'+ '.jpg')
            kl = ('//SERVER/shared_data/Technical/Label Sign Off Sheets/sign off project/Signed/'+ os.path.basename(self.p).strip('- to sign.jpg') +' kl'+ '.jpg')

            if os.path.exists(ed) and os.path.exists(Nb) or os.path.exists(jj) or os.path.exists(kl):
                os.remove('//SERVER/shared_data/Technical/Label Sign Off Sheets/sign off project/To sign/' + os.path.basename(self.p))
            else:
                print("False")





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

###################################################################################################################################
                                                        ##Load Image##
###################################################################################################################################
        # Create a Tkinter variable
        self.tkvar = StringVar()

        # Directory
        self.nonedisplayed = "N/A"
        self.directory = "//SERVER/shared_data/Technical/Label Sign Off Sheets/sign off project/To Sign"
        self.choices = glob.glob(os.path.join(self.directory, "*- to sign.jpg"))



        self.tkvar.set('...To Sign Off...') # set the default option

        # Images
        def change_dropdown():
            imgpath = self.tkvar.get()
            img = Image.open(imgpath)
            photo = ImageTk.PhotoImage(img)
            label2.image = photo
            label2.configure(image=photo)
            self.CaptureScreen['state'] = 'normal'


        #return path value
        self.p = None
        def func(value):
            global p
            self.p = Path(value)
            print(self.p)

        #widgets
        self.msg1 = Label(main, text = "Choose here")
        self.msg1.grid(column = 0, row = 0)
        self.popupMenu = OptionMenu(main, self.tkvar, *self.choices, command = func)
        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 manualopen():
            os.startfile('//SERVER/shared_data/Technical/Food Safety & Quality Manual/Section 21 - Process Control/21.LABL.02 - Labelling notes.docx')

        self.manualBtn = Button(manual, text= "open doc", command = manualopen)
        self.manualBtn.pack()




if __name__ == '__main__':
    main()

Also, my passwords.txt file looks like this:

ed:password1
Nb:rother
jj:4spaces
kl:timepass
\$\endgroup\$
5
\$\begingroup\$

Storing passwords in plaintext

I already mentioned this in a comment to your question: Don't store passwords in plaintext! It's usually not even a good idea for testing (when are you going to test the password storage then?). Plus, it's quite easy to do in Python e.g. using the passlib library. Inspired from their example section:

from passlib.hash import pbkdf2_sha256

hash_ = pbkdf2_sha256.hash("password")
print(hash_)
$pbkdf2-sha256$29000$2RtjbM0ZY8y5l/IeQyhFCA$wxMKXJPS6gtJRKHWki1.z0UQHQlR292ZvFvWEm0wSYc
# verify provides a constant time key comparison function in order to avoid timing attacks
assert pbkdf2_sha256.verify("password", hash_)
assert not pbkdf2_sha256.verify("password2", hash_)

The example above uses PBKDF2 algorithm to store the password, but passlib also supports bcrypt, scrypt, and the current "state of the art" argon2. All of them are hash functions specifically designed to be used for password storage.

If you don't want to work with an external library, the Python standard library module hashlib has hashlib.scrypt from Python 3.6 onwards (if your platform supports OpenSSL). The usage of this function might not be as smooth as with the library above, and will require a little bit more care from your side.

Hard-coded paths

Your application has a few hard-coded paths where your labels should be stored (probably?). You'd usually want to avoid that, so that your code does not have to be changed whenever the location of those files should change. Environment variables or a config file, maybe also written in JSON, are things to look at. If that's all to fancy for what you have in mind, at least store them into a MODULE_LEVEL_CONSTANT in order to avoid copy-and-pasting the same long string over and over again.

Reading and writing files

When using open(...) it's a common best practice to use it in conjunction with a context manager, aka with statement. Using with open(...) as file_: takes care to properly close the file no matter what, i.e. also in the event of an exception, which would usually require using something like try: ... finally: ....

String formatting

The current code uses the "old style" % string formatting, which was superseded by .format(...) in Python 2.7 and f-strings in Python 3.6.

foo = 1
# old style
"%d" % foo
# Python 2.7+
"{}".format(foo)
# Python 3.6+
f"{foo}"

Especially f-string are very powerful and convenient. Maybe have a look at this blog post if you are interested to learn more about each of the three variants and their features.

Style and formatting

The overall appearance of the code seems a bit rough. Especially blank lines are not used very to structure the code into logical blocks. Since your code has grown quite a bit, I'd highly recommend looking for an IDE with autoformatting capabilities or at least a style-checker integration (Flake8, Pylint, ...). This meta post here on Code Review lists a good first overview of tools and editors to choose from.

\$\endgroup\$
  • \$\begingroup\$ Hey @AlexV appreciate for your time in reviewing my Python script. I will take everything on account and improve my script and keep it updated on Code Review! \$\endgroup\$ – 98Ed Oct 2 at 20:26
  • 3
    \$\begingroup\$ I really appreciate that you like the answer enough to accept it right away. Here on Code Review it's usually a good idea to wait a bit (at least a few hours to half a day) before accepting one, since answers here can take quite some time and you never know what there is to come from other members of the community :-) \$\endgroup\$ – AlexV Oct 2 at 20:29
  • 2
    \$\begingroup\$ That is nothing to be sorry about. Just give the community some time to work with your code. \$\endgroup\$ – AlexV Oct 2 at 20:32
  • 2
    \$\begingroup\$ F"{foo}" method is really short, love it \$\endgroup\$ – 98Ed Oct 3 at 9:05
  • 1
    \$\begingroup\$ I don't really work with tkinter so I'm not in the position to give you suggestions on that (it's also something comments are not meant for). If you cannot find the answer using Google or the search at Stack Overflow, maybe consider asking a new question there on Stack Overflow (also check the How to Ask page there). \$\endgroup\$ – AlexV Oct 4 at 8:20
3
\$\begingroup\$

First lets fix your imports.

You are importing * and you should do import tkinter as tk so you don't overwrite anything. You are also importing DISABLED and that is already imported from * so on the very next line you are already overwriting imports.

Next did major PEP8 clean up. This includes proper spacing between qualifiers, classes, functions, comments and so on as well as working on the max line length for coding.

Next we got rid of a lot of unneeded self prefixes. You only need self when you have a class attribute or method you will need to use again in the code down the line.

Then we need to replace all you concatenation with format() as format is the current correct method of concatenation.

One big save on rows is to build your Check Buttons into a list and then reference that list when writing your data.

One last change was to convert your Window1 class to be the root window through inheritance as well as convert Window2 to a Toplevel class. This allows us to use self.master to work with the parent window.

I got your code down to 147 lines of code and I could probably get it down lower if I could do some testing. Take a look at the below and let me know if you have any questions.

I do have a concern on this line:

account_list = [line.split(":", maxsplit=1) for line in open("passwords.txt")]

I am a fan of one liners however I think (I may be wrong) this remains open as you never tell it to close. I would instead load the file using a with open() statement instead and then take the data and throw that into your one liner. The benefit to using with open() is that once the work is complete it will auto close the connection/file.

I will be adding more detail to this answer when I get home but for now this should be a good start.

Edit: Some more error fixing.

import tkinter as tk
import tkinter.ttk as ttk
from tkinter import messagebox
from PIL import Image, ImageTk, ImageGrab
from pathlib import Path
import pyautogui
import glob
import time
import os


def main():
    Window1().mainloop()


def manual_open():
    os.startfile('//SERVER/shared_data/Technical/Food Safety & Quality Manual/Section 21 -'
                 ' Process Control/21.LABL.02 - Labelling notes.docx')


class Window1(tk.Tk):
    def __init__(self):
        super().__init__()
        self.title("User Log In")
        self.geometry('400x150')
        frame = tk.Frame(self)
        frame.pack(fill="both", expand=True)

        tk.Label(frame, text="Username: ", font=("bold", 16)).pack()
        self.entry_username = tk.Entry(frame, font=("bold", 14))
        self.entry_username.pack()
        tk.Label(frame, text="Password: ", font=("bold", 16)).pack()
        self.entry_password = tk.Entry(frame, show="*", font=("bold", 14))
        self.entry_password.pack()
        tk.Button(frame, text="Login", font=("bold", 10), command=self._login_btn_clicked).pack()

    def on_closing(self):
        self.destroy()

    def _login_btn_clicked(self):
        username = self.entry_username.get()
        password = self.entry_password.get()
        account_list = [line.split(":", maxsplit=1) for line in open("passwords.txt")]
        accounts = {key: value.rstrip() for key, value in account_list}
        if accounts[username] == password:
            self.withdraw()
            self.app = Window2()
            self.app.protocol("WM_DELETE_WINDOW", self.on_closing)
        else:
            messagebox.showinfo("User message", "Invalid username or password specified please try again")


class Window2(tk.Toplevel):
    def __init__(self):
        super().__init__()
        notebook = ttk.Notebook(self)
        notebook.pack(expand=1, fill="both")
        main = ttk.Frame(notebook)
        manual = ttk.Frame(notebook)
        notebook.add(main, text='Main-Screen')
        notebook.add(manual, text='Manual')
        self.display_time = tk.Label(main)
        self.display_time.grid(column=3, row=0)
        self.clock()
        self.username = self.master.entry_username.get()
        tk.Label(main, text='User: {}'.format(self.username), font='15').grid(column=4, row=0)

        self.checkbutton_list = []
        checkbutton_verb = ["Ingredients present in full (any allergens in bold with allergen warning if necessary)",
                            "May Contain Statement.", "Cocoa Content (%).", "Vegetable fat in addition to Cocoa butter",
                            "Instructions for Use.", "Additional warning statements (pitt/stone, hyperactivity etc)",
                            "Nutritional Information Visible", "Storage Conditions", "Best Before & Batch Information",
                            "Net Weight & Correct Font Size.", "Barcode - Inner", "Address & contact details correct"]

        for ndex, i in enumerate(checkbutton_verb):
            x = tk.IntVar()
            self.checkbutton_list.append([tk.Checkbutton(main, text=i, variable=x), x])
            self.checkbutton_list[-1][0].grid(column=2, row=ndex+1, sticky='w')

        directory = "//SERVER/shared_data/Technical/Label Sign Off Sheets/sign off project/To Sign"
        choices = glob.glob(os.path.join(directory, "*- to sign.jpg"))
        tk.Label(main, text="Choose here").grid(column=0, row=0)
        self.tkvar = tk.StringVar()
        self.tkvar.set('...To Sign Off...')
        tk.OptionMenu(main, self.tkvar, *choices, command=self.func).grid(row=1, column=0)
        self.label2 = tk.Label(main, image=None)
        self.label2.grid(row=2, column=0, rowspan=500)
        tk.Button(main, text="Open", command=self.change_dropdown).grid(row=502, column=0)
        self.dataSend = tk.Button(main, text="Send", command=self.var_states, state='disabled')
        self.dataSend.grid(column=1, row=13, sticky='w')
        self.CaptureScreen = tk.Button(main, text="PrintScreen", command=self.print_screen, state='disabled')
        self.CaptureScreen.grid(column=1, row=14, sticky='w')
        self.manualBtn = tk.Button(manual, text="open doc", command=manual_open)
        self.manualBtn.pack()

    def clock(self):
        t = time.strftime('%d/%m/%Y, %H:%M:%S, ', time.localtime())
        if t != '':
            self.display_time.config(text=t, font='times 15')
        self.after(1000, self.clock)

    def print_screen(self):
        pyautogui.keyDown('alt')
        pyautogui.keyDown('printscreen')
        pyautogui.keyUp('printscreen')
        pyautogui.keyUp('alt')
        self.dataSend['state'] = 'normal'

    def var_states(self):
        text_file = open("logfile.txt", "a")
        formatted_string = 'Username: {}'.format(self.username)
        for ndex, sub_list in enumerate(self.checkbutton_list):
            formatted_string = '{}, Option {}: '.format(formatted_string, ndex+1, sub_list[1].get())
        text_file.write(formatted_string)
        text_file.close()
        self.img = ImageGrab.grabclipboard()
        self.img.save('//SERVER/shared_data/Technical/Label Sign Off Sheets/sign off project/Signed/{} {} .jpg'
                      .format(os.path.basename(self.p).strip('- to sign.jpg'), self.username), 'JPEG')
        ed = '//SERVER/shared_data/Technical/Label Sign Off Sheets/sign off project/Signed/{} ed.jpg'\
             .format(os.path.basename(self.p).strip('- to sign.jpg'))
        nb = '//SERVER/shared_data/Technical/Label Sign Off Sheets/sign off project/Signed/{} Nb.jpg'\
             .format(os.path.basename(self.p).strip('- to sign.jpg'))
        jj = '//SERVER/shared_data/Technical/Label Sign Off Sheets/sign off project/Signed/{} jj.jpg'\
             .format(os.path.basename(self.p).strip('- to sign.jpg'))
        kl = '//SERVER/shared_data/Technical/Label Sign Off Sheets/sign off project/Signed/{} kl.jpg'\
             .format(os.path.basename(self.p).strip('- to sign.jpg'))

        if os.path.exists(ed) and os.path.exists(nb) or os.path.exists(jj) or os.path.exists(kl):
            os.remove('//SERVER/shared_data/Technical/Label Sign Off Sheets/sign off project/To sign/{}'
                      .format(os.path.basename(self.p)))
        else:
            print("False")

    def change_dropdown(self):
        img = Image.open(self.tkvar.get())
        photo = ImageTk.PhotoImage(img)
        self.label2.image = photo
        self.label2.configure(image=photo)
        self.CaptureScreen['state'] = 'normal'
        self.p = None

    def func(self, value):
        self.p = Path(value)
        print(self.p)


if __name__ == '__main__':
    main()

For your error in the comments I am not getting this same issue. Please provide full trace back. As you can see below I can print the value of accounts resulting from this line accounts = {key: value.rstrip() for key, value in account_list} and that is the same line you get an error on.

enter image description here

\$\endgroup\$
  • \$\begingroup\$ Hi Mike, thanks for your feedback! For your code, this error shows up self.tk = master.tk AttributeError: 'function' object has no attribute 'tk' \$\endgroup\$ – 98Ed Oct 9 at 7:26
  • \$\begingroup\$ @98Ed Ah I think that is because I forgot super in your top level class. I added that in. IF you can give me a line number for the error that would help. \$\endgroup\$ – Mike - SMT Oct 9 at 11:54
  • \$\begingroup\$ @98Ed I found the error sorry. I had self.frame instead of frame in one of the buttons. It should run now. \$\endgroup\$ – Mike - SMT Oct 9 at 18:44
  • \$\begingroup\$ @98Ed Ok I did some testing by removing some code (related to your paths) and it should all work now. Let me know if you have any more problems. \$\endgroup\$ – Mike - SMT Oct 9 at 19:02
  • \$\begingroup\$ Hi Mike, sorry for late reply. Here is the ValueError for the script above: ``` accounts = {key: value.rstrip() for key, value in account_list} ValueError: not enough values to unpack (expected 2, got 1)``` \$\endgroup\$ – 98Ed Oct 10 at 5:54

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.