4
\$\begingroup\$

Please explain what I can improve, and why. I'm new to Python, Tkinter & Code Review, so I have a lot to learn. :)

I created a Tkinter app for user-input, and the same types of widgets are repeated on each row. A single row is similar to a slide in PowerPoint because it has an image, numbers, and input text. I will later add the "export to separate file" functionality, but for now I'm focused on the "skeleton" of the code, which is the creation & display of the widgets.

Since each row is a repeat of the one above it, I know I could somehow create a "row-object" using a class, and/or use a for-loop to create each row. Since I'm not sure how to do either, my code is verbose and repetitive. I manually create each widget. I used Excel to spit out this part of the code. This is what my GUI looks like: notice the repetition in each row Rendered image of the GUI

This is my code:

import tkinter as tk
from tkinter import simpledialog,filedialog,colorchooser,messagebox,Frame,Button
from PIL import ImageTk, Image

root = tk.Tk()

load1 = Image.open("example.jpg")
root.render1 = ImageTk.PhotoImage(load1)

canv_1 = tk.Canvas(root, bg="gray")
canv_1.grid_rowconfigure(0, weight=1)
#canv_1.grid_columnconfigure((0, 1, 2, 3, 4, 5, 7), weight=1)
canv_1.grid_columnconfigure(6, weight=2)

canv_1.grid(row=0, column=0, sticky="news")
canv_1.grid(row=1, column=0, sticky="news")

labels = ["Image", "Chapter #", "Chapter Title", "Step", "Slide", "Sequence Step", "Instructions"]
root.label_wid = []
font1 = ("arial", 15, "bold")
for i in range(len(labels)):
    root.label_wid.append(tk.Label(canv_1, font=font1, relief="raised", text=labels[i]).grid(row=0, column=i,
                                                                                                        sticky="we"))
def exportCode():
    print("code exported")
    

def change_img(row):
    if row == 1:
        root.img1 = filedialog.askopenfilename()
        root.load_img1 = Image.open(root.img1)
        root.render_img1 = ImageTk.PhotoImage(root.load_img1)
        R1C0['image']=root.render_img1
    if row == 2:
        root.img2 = filedialog.askopenfilename()
        root.load_img2 = Image.open(root.img2)
        root.render_img2 = ImageTk.PhotoImage(root.load_img2)
        R2C0['image']=root.render_img2
    if row == 3:
        root.img3 = filedialog.askopenfilename()
        root.load_img3 = Image.open(root.img3)
        root.render_img3 = ImageTk.PhotoImage(root.load_img3)
        R3C0['image']=root.render_img3
    if row == 4:
        root.img4 = filedialog.askopenfilename()
        root.load_img4 = Image.open(root.img4)
        root.render_img4 = ImageTk.PhotoImage(root.load_img4)
        R3C0['image']=root.render_img4
    if row == 5:
        root.img5 = filedialog.askopenfilename()
        root.load_img5 = Image.open(root.img5)
        root.render_img5 = ImageTk.PhotoImage(root.load_img5)
        R3C0['image']=root.render_img5

c1 = "#a9d08e"
c2 = "#8dd1bf"

# Below is the really repetitive part that I would love to simplify:
###########################################################################################
# ===============================================================================================
R1C0= tk.Button(canv_1, image=root.render1, relief="raised", bg="light gray", command =  lambda: change_img(1))
R1C0.grid(row=1, column= 0, sticky="news")
R1C1= tk.Entry(canv_1, bg = c1, font=("arial",15)).grid(row= 1, column= 1, sticky= "news")
R1C2= tk.Entry(canv_1, bg = c1, font=("arial",15)).grid(row= 1, column= 2, sticky= "news")
R1C3= tk.Entry(canv_1, bg = c1, font=("arial",15)).grid(row= 1, column= 3, sticky= "news")
R1C4= tk.Entry(canv_1, bg = c2, font=("arial",15)).grid(row= 1, column= 4, sticky= "news")
R1C5= tk.Entry(canv_1, bg = c2, font=("arial",15)).grid(row= 1, column= 5, sticky= "news")
R1C6= tk.Text(canv_1, bg="white", wrap="word", font=("arial",15), width=20, height=10)
R1C6.grid(row=1, column= 6, sticky= "news")
R2C0= tk.Button(canv_1, image=root.render1, relief="raised", bg="light gray", command =  lambda: change_img(2))
R2C0.grid(row=2, column= 0, sticky="news")
R2C1= tk.Entry(canv_1, bg = c1, font=("arial",15)).grid(row= 2, column= 1, sticky= "news")
R2C2= tk.Entry(canv_1, bg = c1, font=("arial",15)).grid(row= 2, column= 2, sticky= "news")
R2C3= tk.Entry(canv_1, bg = c1, font=("arial",15)).grid(row= 2, column= 3, sticky= "news")
R2C4= tk.Entry(canv_1, bg = c2, font=("arial",15)).grid(row= 2, column= 4, sticky= "news")
R2C5= tk.Entry(canv_1, bg = c2, font=("arial",15)).grid(row= 2, column= 5, sticky= "news")
R2C6= tk.Text(canv_1, bg="white", wrap="word", font=("arial",15), width=20, height=10)
R2C6.grid(row=2, column= 6, sticky= "news")
R3C0= tk.Button(canv_1, image=root.render1, relief="raised", bg="light gray", command =  lambda: change_img(3))
R3C0.grid(row=3, column= 0, sticky="news")
R3C1= tk.Entry(canv_1, bg = c1, font=("arial",15)).grid(row= 3, column= 1, sticky= "news")
R3C2= tk.Entry(canv_1, bg = c1, font=("arial",15)).grid(row= 3, column= 2, sticky= "news")
R3C3= tk.Entry(canv_1, bg = c1, font=("arial",15)).grid(row= 3, column= 3, sticky= "news")
R3C4= tk.Entry(canv_1, bg = c2, font=("arial",15)).grid(row= 3, column= 4, sticky= "news")
R3C5= tk.Entry(canv_1, bg = c2, font=("arial",15)).grid(row= 3, column= 5, sticky= "news")
R3C6= tk.Text(canv_1, bg="white", wrap="word", font=("arial",15), width=20, height=10)
R3C6.grid(row=3, column= 6, sticky= "news")
R4C0= tk.Button(canv_1, image=root.render1, relief="raised", bg="light gray", command =  lambda: change_img(4))
R4C0.grid(row=4, column= 0, sticky="news")
R4C1= tk.Entry(canv_1, bg = c1, font=("arial",15)).grid(row= 4, column= 1, sticky= "news")
R4C2= tk.Entry(canv_1, bg = c1, font=("arial",15)).grid(row= 4, column= 2, sticky= "news")
R4C3= tk.Entry(canv_1, bg = c1, font=("arial",15)).grid(row= 4, column= 3, sticky= "news")
R4C4= tk.Entry(canv_1, bg = c2, font=("arial",15)).grid(row= 4, column= 4, sticky= "news")
R4C5= tk.Entry(canv_1, bg = c2, font=("arial",15)).grid(row= 4, column= 5, sticky= "news")
R4C6= tk.Text(canv_1, bg="white", wrap="word", font=("arial",15), width=20, height=10)
R4C6.grid(row=4, column= 6, sticky= "news")
R5C0= tk.Button(canv_1, image=root.render1, relief="raised", bg="light gray", command =  lambda: change_img(5))
R5C0.grid(row=5, column= 0, sticky="news")
R5C1= tk.Entry(canv_1, bg = c1, font=("arial",15)).grid(row= 5, column= 1, sticky= "news")
R5C2= tk.Entry(canv_1, bg = c1, font=("arial",15)).grid(row= 5, column= 2, sticky= "news")
R5C3= tk.Entry(canv_1, bg = c1, font=("arial",15)).grid(row= 5, column= 3, sticky= "news")
R5C4= tk.Entry(canv_1, bg = c2, font=("arial",15)).grid(row= 5, column= 4, sticky= "news")
R5C5= tk.Entry(canv_1, bg = c2, font=("arial",15)).grid(row= 5, column= 5, sticky= "news")
R5C6= tk.Text(canv_1, bg="white", wrap="word", font=("arial",15), width=20, height=10)
R5C6.grid(row=5, column= 6, sticky= "news")

# ================================================================================================

bt1 = tk.Button(canv_1, text="Export", font=font1, command=exportCode).grid(row=0, column=7)
load2 = Image.open("scroll-up.png")
root.render2 = ImageTk.PhotoImage(load2)
load3 = Image.open("scroll-down.png")
root.render3 = ImageTk.PhotoImage(load3)
scroll_up = tk.Button(canv_1, image=root.render2).grid(row=1, column=7, rowspan=2)  # , sticky="n")
scroll_up = tk.Button(canv_1, image=root.render3).grid(row=3, column=7, rowspan=2)  # , sticky="s")

root.mainloop()

Any tips on simplifying and refactoring this would be awesome.

\$\endgroup\$
5
  • 2
    \$\begingroup\$ Welcome to Code Review! I've edited your question to make it on topic for the site, but it would be nice if you could edit the title to make it more clear what your application is for (user input is very generic). \$\endgroup\$
    – Dan Oberlam
    Commented Jul 16, 2020 at 15:46
  • 2
    \$\begingroup\$ I also removed the section related to "what you tried and didn't work", as well as "what you want to be able to do eventually" - here on Code Review, we strictly review complete, working code. If you have questions on how to do something, consider posting on Stack Overflow, but please remember to review their rules for posting and make sure you've done your research before asking over there. \$\endgroup\$
    – Dan Oberlam
    Commented Jul 16, 2020 at 15:46
  • 1
    \$\begingroup\$ Hi @Dannnno thanks for the welcome and edits! I like to make the skeleton before adding the "guts" and functionality. Adding all the functionality here would add clutter. So for now my goal is to produce the image shown, without precluding the later option of extracting values from each entry widget. One way to do this is manually (as I did) but I have a feeling that I could have used a class or a for-loop instead. \$\endgroup\$
    – Wes Tomer
    Commented Jul 16, 2020 at 15:53
  • 1
    \$\begingroup\$ This question looks familiar. Did you try posting this code earlier? Did you fix the issues it had back then? \$\endgroup\$
    – Mast ♦
    Commented Jul 16, 2020 at 17:06
  • 1
    \$\begingroup\$ @Reinderien Yes, I abandoned the scroll bars from the previous code. Now I'm just trying to code incrementally -- the first working increment is what I have here. It works, as in it shows the widgets, and you can change the images and text. However I know there must be a less verbose way of showing all the widgets. Please let me know if I should wait to ask this question after I build in all the functionality I intend, rather than this working "skeleton" code. I can understand either way. \$\endgroup\$
    – Wes Tomer
    Commented Jul 16, 2020 at 17:34

1 Answer 1

Reset to default
5
\$\begingroup\$

If there's a list, make it a list

Do not represent RxCx variables as separate variables. Represent them as nested lists and it will greatly simplify your code. Consider grouping them by what makes them similar - column 0 (image_col), columns 1-5 (info_cols) and column 6 (instruction_col) as in the following example:

image_col = [
    tk.Button(
        canv_1, 
        image=root.render1, 
        relief="raised", 
        bg="light gray", 
        command = lambda: change_img(y),
    )
    for y in range(1, 6)
]

for y, image_cell in enumerate(image_col, 1):
    image_cell.grid(row=y, column=0, sticky='news')

instruction_col = [
    tk.Text(
        canv_1,
        bg="white",
        wrap="word",
        font=("arial",15), width=20, height=10)
]

for y, image_cell in enumerate(instruction_col, 1):
    image_cell.grid(row=y, column=6, sticky='news')

Replace your change_img so that there are no ifs, and everything is done via list lookup from the row index passed. Replace root.img*, root.load_img* and root.render_img* with lists.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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