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())
-
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\$Perry– Perry2016年06月03日 12:31:02 +00:00Commented 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\$click here– click here2016年06月03日 12:34:58 +00:00Commented Jun 3, 2016 at 12:34
-
1\$\begingroup\$ Perhaps. By the way, I copied my previous comment from the possible close reasons. \$\endgroup\$Perry– Perry2016年06月03日 12:36:09 +00:00Commented Jun 3, 2016 at 12:36
1 Answer 1
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.
-
\$\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\$click here– click here2016年06月09日 23:37:10 +00:00Commented Jun 9, 2016 at 23:37