6
\$\begingroup\$

This was my first foray into a Tkinter app and so I chose a simple and certainly overkill task to make into a gui.

Aside from one small aspect, this code functions as I was intending and seems to have adequate performance. I'm looking for help to make it more pythonic, not by simply pep8-ing it but by helping me better design the structure of functions and the code within them. For instance, do I really need self. for just about everything in my class mainframe?

The one thing that isn't performing as I want is the writing to CSV. It keeps the CSV file open in python until the tkinter window is closed and it also creates a CSV file when the dry run checkbox is checked.

One more note, this app was designed only to function on Windows. The full code can be viewed here. Thanks!

def path_shorten(path):
 path = path.split('/')
 if len(path) <= 4:
 return os.sep.join(path)
 else:
 return os.sep.join(path[:2]+['...']+path[-2:])
def open_file(selection,file_path_list):
 os.startfile(file_path_list[selection-1])
def msgbox(msg, dryrun_list=0, file_name_len=0, file_path_len = 0):
 mb = Tk()
 if isinstance(dryrun_list, list):
 mb.wm_title("Notice")
 label = ttk.Label(mb, text=msg, font=NORM_FONT)
 label.pack(side=TOP, fill=BOTH)
 scrollbar = Scrollbar(mb)
 scrollbar.pack(side=RIGHT, fill=Y)
 listbox = Listbox(mb, yscrollcommand=scrollbar.set, height=30, width=120, font='TkFixedFont')
 listbox.insert(END, 'File name'.ljust(file_path_len + 2) + 'Date Created') # plus 2 for column spacing
 listbox.itemconfig(0, {'bg': 'lightblue'})
 file_path_list = []
 for i in dryrun_list:
 manip_path = path_shorten(str(i[2]))
 justify = file_path_len
 listbox.insert(END, manip_path.ljust(justify+2) + str(i[1]) )
 file_path_list.append(i[2])
 listbox.pack(side=TOP, fill=BOTH)
 scrollbar.config(command=listbox.yview)
 listbox.bind('<Double-1>', lambda x: open_file(listbox.curselection()[0],file_path_list))
 else:
 mb.wm_title("Warning!")
 label = ttk.Label(mb, text=msg, font=NORM_FONT)
 label.pack(side=TOP, fill=BOTH)
 button = ttk.Button(mb, text="Okay", command=mb.destroy)
 button.pack(side=BOTTOM, fill=BOTH)
 mb.mainloop()
def walk_dir(main_obj, path, daydif, filetype):
 current_time = datetime.datetime.now().strftime("%m_%d_%y")
 dryrun_list = []
 file_name_len = 0
 file_path_len = 0
 if os.path.isdir(path) and os.path.exists(path):
 file_cnt = 0
 # this is for counting for prog bar increment
 for dirpath, dirnames, filenames in os.walk(path):
 for file in filenames:
 full_file_path = os.path.join(dirpath, file).replace("\\","/")
 seconds_dif = calendar.timegm(time.gmtime()) - os.path.getmtime(full_file_path)
 days, seconds = divmod(seconds_dif, 24 * 60 * 60)
 if int(days) >= daydif:
 if file.endswith("." + filetype):
 file_cnt += 1
 if file_cnt == 0:
 msgbox('No files found meeting the selected conditions. There may be empty directories but they cannot be deleted indepedent of deleting files')
 return False
 with open(path + '/Files removed ' + current_time + '.csv', 'w', newline='') as output:
 csv_output = csv.writer(output, delimiter=',')
 csv_output.writerows([['File Name', 'Removed From']])
 # this is for deleting
 for dirpath, dirnames, filenames in os.walk(path):
 for file in filenames:
 full_file_path = os.path.join(dirpath, file).replace("\\","/")
 time_created = time.ctime(os.path.getmtime(full_file_path))
 seconds_dif = calendar.timegm(time.gmtime()) - os.path.getmtime(full_file_path)
 days, seconds = divmod(seconds_dif, 24 * 60 * 60)
 if int(days) >= daydif:
 if file.endswith("." + filetype):
 main_obj.progbar.step((1 / file_cnt) * 100)
 main_obj.progbar.update()
 if main_obj.checkvar.get() == '0':
 #dry run not selected
 os.remove(os.path.join(dirpath, file))
 csv_output.writerows([[file,dirpath]])
 else:
 #dry run selected')
 if len(file) > file_name_len:
 file_name_len = len(file)
 if len(path_shorten(full_file_path)) > file_path_len:
 file_path_len = len(path_shorten(full_file_path))
 dryrun_list.append([file, time_created, full_file_path])
 if dryrun_list != []:
 msgbox('If you run with the current settings these are the files you will delete.', dryrun_list,
 file_name_len,file_path_len)
 dryrun_list = []
 return
 if main_obj.empdir_var.get() == '0': # rmdir checkbox
 print('remove dir not selected')
 msgbox('Processing complete!')
 return
 else:
 for dirpath, n, f in os.walk(path, topdown=False):
 try:
 os.rmdir(dirpath)
 except OSError as ex:
 print("Directory not empty.")
 msgbox('Processing complete!')
 return
 return # close csv????
 elif path == 'Click Browse to add folder':
 msgbox('Select a directory to run the script on.')
 else:
 msgbox("'" + path + "'" + ' does not exist or is not a valid path')
 file_cnt = 0
class MainFrame:
 def __init__(self, master):
 master.title('Dated File Removal Tool')
 # TOP FRAME
 self.topframe = ttk.Frame()
 self.topframe.grid(row=0, column=0, padx=10, pady=2, sticky='W')
 # browse button
 self.browseButton = Button(self.topframe, text="Browse", command=self.load_dir, width=15).grid(row=0, column=0)
 # okbutton test
 button = Button(self.topframe, text="Execute", command=self.ok_click, width=15)
 button.grid(row=0, column=1, sticky='E')
 # MIDDLE FRAME
 self.middleFrame = ttk.Frame()
 self.middleFrame.grid(row=1, column=0, sticky='W', padx=10, pady=5)
 # path display box
 self.dir_path = StringVar()
 browse_entry = Entry(self.middleFrame, width=65, textvariable=self.dir_path, state=DISABLED)
 browse_entry.grid(row=0, column=0)
 self.dir_path.set("Click Browse to add folder")
 # label frame
 self.options_lf = ttk.Labelframe(self.middleFrame, text='Options')
 # days combobox
 days = ('00 days', '30 Days', '60 Days', '90 Days')
 self.days_cmb = ttk.Combobox(self.options_lf, values=days, state='readonly')
 self.days_cmb.current(0) # set selection
 self.days_cmb.grid(row=0, column=0, padx=25)
 # file types combobox
 filetypes = ('jpg', 'mp3', 'avi')
 self.file_cmb = ttk.Combobox(self.options_lf, values=filetypes, state='readonly')
 self.file_cmb.current(0) # set selection
 self.file_cmb.grid(row=0, column=1, padx=25, pady=2)
 # test run checkbox
 self.checkvar = StringVar()
 dryrun_cb = Checkbutton(self.options_lf, text="Test Run", variable=self.checkvar)
 dryrun_cb.grid(row=1, column=0, sticky='W', padx=25)
 self.checkvar.set(1)
 # Remove dirs checkbox
 self.empdir_var = StringVar()
 empdir_cb = Checkbutton(self.options_lf, text="Remove Empty Dirs", variable=self.empdir_var)
 empdir_cb.grid(row=1, column=1, sticky='W', padx=25)
 self.empdir_var.set(0)
 # place in frame
 self.options_lf.grid(row=1, column=0, sticky='W')
 # BOTTOM FRAME
 self.bottomframe = ttk.Frame()
 self.bottomframe.grid(row=3, column=0)
 # prog bar
 self.progbar = ttk.Progressbar(self.bottomframe, orient='horizontal', mode='determinate', length=420)
 self.progbar.grid(row=1, column=0)
 def load_dir(self):
 dirname = askdirectory()
 if not dirname:
 pass # file upload cancelled
 self.dir_path.set(dirname)
 def ok_click(self):
 daydif = int(self.days_cmb.get()[:2])
 walk_dir(self, self.dir_path.get(), daydif, self.file_cmb.get())
asked Jun 3, 2016 at 12:23
\$\endgroup\$
3
  • 1
    \$\begingroup\$ Links to code hosted on third-party sites are permissible, but the most relevant excerpts must be embedded in the question itself. \$\endgroup\$ Commented Jun 3, 2016 at 12:31
  • \$\begingroup\$ Well, I'm not sure I'm the right person to judge what are or are not relevant excerpts. Do you think it would be appropriate to post 95% of the code in my post? \$\endgroup\$ Commented Jun 3, 2016 at 12:34
  • 1
    \$\begingroup\$ Perhaps. By the way, I copied my previous comment from the possible close reasons. \$\endgroup\$ Commented Jun 3, 2016 at 12:36

1 Answer 1

3
\$\begingroup\$

Avoid wildcard imports

PEP8 recommends avoiding wildcard imports, which is good advice. You should import tkinter with something like import tkinter as tk, and then prefix all of the widgets with tk. (eg: tk.Tk()).

Don't create more than one instance of Tk

You are creating two instance of Tk. Don't do that. This will not work quite the way you expect. It works in some simple scenarios, but there are limitations. Tkinter was designed to have exactly one instance of Tk, and to call mainloop exactly once. If you need more windows, create instances of Toplevel.

Group your layout statements together

I recommend grouping layout statements together in a logical fashion, rather than sprinkling them throughout your code. In my experience this makes the code much easier to read and maintain, and makes it much easier to visualize the layout.

For example:

self.topFrame = tk.Frame(...)
self.middleFrame = tk.Frame(...)
self.bottomFrame = tk.Frame(...)
self.topframe.grid(row=0, column=0, padx=10, pady=2, sticky='W')
self.middleFrame.grid(row=1, column=0, sticky='W', padx=10, pady=5)
self.bottomframe.grid(row=3, column=0)

As a slight alternative, you might want to consider moving each frame into either a class or a function, so that your main function is shorter.

For example:

def make_topFrame(master):
 self.topFrame = tk.Frame(master, ...)
 ...
def make_middleFrame(master):
 self.middleFrame = tk.Frame(master, ...)
 ...
def make_bottomFrame(master):
 self.bottomFrame = tk.Frame(master, ...)
 ...

With that, your __init__ function becomes much simpler:

class MainFrame:
 def __init__(self, master):
 master.title('Dated File Removal Tool')
 make_topFrame(master)
 make_middleFrame(master)
 make_bottomFrame(master)
 self.topframe.grid(row=0, column=0, padx=10, pady=2, sticky='W')
 self.middleFrame.grid(row=1, column=0, sticky='W', padx=10, pady=5)
 self.bottomframe.grid(row=3, column=0)

Don't prevent the user from resizing the window

In the full version of the code you have this:

root.resizable(width=FALSE, height=FALSE)

There's almost never a good reason to prevent the user from resizing the GUI. You should remove this line of code. Users may have different resolutions, or different default fonts, or larger or smaller physical displays. Let them make the window be whatever size works for them.

Always give at least one row and one column a positive weight

As a rule of thumb, whenever you use grid you need to call grid_rowconfigure and grid_columnconfigure at least once to give a positive weight to one row and one column. This is how tkinter knows to allocate extra space. Typically you'll do this for the row and column of the main widget -- typically a canvas or a text widget, though it could be a form as a whole, or sometimes an extra empty row and/or column. Without this, your resize behavior will not be very good.

301_Moved_Permanently
29.4k3 gold badges49 silver badges98 bronze badges
answered Jun 3, 2016 at 16:39
\$\endgroup\$
1
  • \$\begingroup\$ Thanks for your detailed response. I've grown tired of this program but recently started on my next tkinter project and will be putting all of your advice to work! \$\endgroup\$ Commented Jun 9, 2016 at 23:37

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.