5
\$\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())
\$\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\$
    – pppery
    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\$
    – click here
    Commented Jun 3, 2016 at 12:34
  • 1
    \$\begingroup\$ Perhaps. By the way, I copied my previous comment from the possible close reasons. \$\endgroup\$
    – pppery
    Commented Jun 3, 2016 at 12:36

1 Answer 1

1
\$\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.

\$\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\$
    – click here
    Commented Jun 9, 2016 at 23:37

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.