5
\$\begingroup\$

Below code produces a basic text editor that I have written using tkinter with python. I tried to be as well structured as I can but in some parts I have been lazy about it.

Classes that are used:

  • MainWindow: I tried to manage anything topmost level using this class but I was unable to tell if something was 'topmost level' or not at some parts such as changing the title using a children's method like new_file.
  • MainFrame: This class was really the non-configuration parts of the 'topmost level' stuff, and was created with concerns of putting it in another Toplevel widget. It holds the information on widget's configurations and their methods. But again, at the end I don't really see a strict line between this and MainWindow.
  • MainMenu: This class is the structure class to hold sub-menus.
  • FileMenu: This is a sub-menu class to be assigned as file related parts of the MainMenu.
  • EditMenu: This is a sub-menu class to be assigned as edit related parts of the MainMenu.
  • AutoScrollbar: This exists for smart scrollbar visibility.

Here's the code:

#required for anything GUI related
import tkinter as tk
#required for fileopen and save options
from tkinter import filedialog
#required for file's basename
import os
#required for save_before_leave basically
from tkinter import messagebox

#Nae Unicode text entering is missing

class MainWindow(tk.Tk):
    def __init__(self):
        super().__init__()
        self.title("Untitled - NaePad")

        self.mainFrame = MainFrame(self)
        self.mainFrame.pack(fill="both", expand=True)

        self.mainFrame.text.focus_set()
        self.protocol("WM_DELETE_WINDOW", lambda : self.save_before_leave(self.destroy))

    def save_before_leave(self, callback, *args):

        textBuffer = self.mainFrame.text.get('1.0', 'end-1c')

        #if there's a current file
        if self.mainFrame.curFilePath:
            #if it content doesn't match the current buffer
            if self.mainFrame.curFileCont != textBuffer:
                response = messagebox.askyesnocancel("NaePad - Unsaved File", "Do you want to save before leaving?")
                #if response is yes
                if response:
                    self.mainFrame.save_file()
                    callback()
                #if response is no
                elif response is False:
                    callback()

            else:
                callback()

        else:
            #if there's any text in the buffer
            if textBuffer:
                response = messagebox.askyesnocancel("NaePad - Unsaved File", "Do you want to save before leaving?")
                #If it's a yes
                if response:
                    self.mainFrame.save_as_file()
                    callback()
                #if it's a no
                elif response is False:
                    callback()
            else:
                callback()
        #required in order to prevent "tagbinds" from happening
        return "break"


class MainFrame(tk.Frame):
    def __init__(self, master):
        super().__init__(master)
        self.master = master

        #creates and displays the Menu
        self.menu = MainMenu(master)
        master.config(menu=self.menu)

        #path of the current file
        self.curFilePath = ''

        #current file
        self.curFileCont = ''

        self.text = tk.Text(self, wrap="none")
        #color scheme
        self.text.config(bg='#282c34',fg='#abb2bf', selectbackground='#3e4451')
        self.text.grid(row=0, column=0, sticky="nsew")
        self.rowconfigure(0, weight=1)
        self.columnconfigure(0, weight=1)

        self.scroll_bar_config()
        self.menu_command_config()
        self.key_binds_config()


    #scrollbar configurations for self.text widget
    def scroll_bar_config(self):

        self.scrollY = AutoScrollbar(self, orient="vertical", command=self.text.yview)
        self.scrollY.grid(row=0, column=1, sticky="nsew")
        self.text['yscrollcommand'] = self.scrollY.set

        self.scrollX = AutoScrollbar(self, orient="horizontal", command=self.text.xview)
        self.scrollX.grid(row=1, column=0, sticky="nsew")
        self.text['xscrollcommand'] = self.scrollX.set

    def menu_command_config(self):
        #New
        self.menu.file.entryconfig(0, command=lambda : self.master.save_before_leave(self.new_file))
        #Open...
        self.menu.file.entryconfig(1, command=lambda : self.master.save_before_leave(self.open_file))
        #Save
        self.menu.file.entryconfig(2, command=self.save_file)
        #Save as
        self.menu.file.entryconfig(3, command=self.save_as_file)

        #Cut
        self.menu.edit.entryconfig(0, command=self.cut)
        #Copy
        self.menu.edit.entryconfig(1, command=self.copy)
        #Paste
        self.menu.edit.entryconfig(2, command=self.paste)
        #Delete
        self.menu.edit.entryconfig(3, command=self.delete)

    def key_binds_config(self):
        self.text.bind('<Control-n>', lambda event : self.master.save_before_leave(self.new_file))
        self.text.bind('<Control-N>', lambda event : self.master.save_before_leave(self.new_file))
        self.text.bind('<Control-o>', lambda event : self.master.save_before_leave(self.open_file))
        self.text.bind('<Control-O>', lambda event : self.master.save_before_leave(self.open_file))
        self.text.bind('<Control-s>', self.save_file)
        self.text.bind('<Control-S>', self.save_file)
        self.text.bind('<Control-Shift-s>', self.save_as_file)
        self.text.bind('<Control-Shift-S>', self.save_as_file)

    def new_file(self, *args):
        self.text.delete("1.0", "end")
        self.curFilePath = ''
        self.curFileCont = ''

        #update window name to file name BAD NAE BAD
        self.master.title("Untitled - NaePad")


    def open_file(self, *args):

        #get filepath from user with gui
        filePath = filedialog.askopenfilename(filetypes=(("Text files", "*.txt"),("All files", "*.*")))

        # if filePath is selected
        if filePath:
            try:
                #open only UTF-8 encoded files
                with open(filePath, encoding="UTF-8") as f:
                    self.text.delete("1.0", "end")
                    self.curFileCont = f.read()
                    self.text.insert("1.0", self.curFileCont)
            #if it's not UTF-8 then
            except UnicodeDecodeError:
                #open as ANSI encoding
                with open(filePath, encoding="ANSI") as f:
                    self.text.delete("1.0", "end")
                    self.curFileCont = f.read()
                    self.text.insert("1.0", self.curFileCont)

            #update window name to file name BAD NAE BAD
            self.master.title(os.path.basename(f.name) + " - NaePad")

            #update current file path
            self.curFilePath = filePath

    def save_file(self, *args):
        #if there's already a file
        if self.curFilePath:
            try:
                #open only UTF-8 encoded files
                with open(self.curFilePath, 'w', encoding="UTF-8") as f:
                    self.curFileCont = self.text.get('1.0', 'end-1c')
                    f.write(self.curFileCont)
            #if it's not UTF-8 then
            except UnicodeDecodeError:
                #open as ANSI encoding
                with open(self.curFilePath, 'w', encoding="ANSI") as f:
                    self.curFileCont = self.text.get('1.0', 'end-1c')
                    f.write(self.curFileCont)

            #update window name to file name BAD NAE BAD
            self.master.title(os.path.basename(f.name) + " - NaePad")

        else:
            self.save_as_file()

    def save_as_file(self, *args):
        self.curFilePath = filedialog.asksaveasfilename(defaultextension=".txt", filetypes=(("Text files", "*.txt"),("All files", "*.*")))
        #Checks wether a file is selected or not
        if self.curFilePath:
            self.save_file()


    def cut(self):
        #Is anything selected?
        if self.text.tag_ranges(tk.SEL):
            self.text.clipboard_clear()
            #append to cleared clipboard the (selection)
            self.text.clipboard_append(self.text.get(tk.SEL_FIRST, tk. SEL_LAST))
            self.text.delete(tk.SEL_FIRST, tk. SEL_LAST)

    def copy(self):
        #Is anything selected?
        if self.text.tag_ranges(tk.SEL):
            self.text.clipboard_clear()
            #append to cleared clipboard the (selection)
            self.text.clipboard_append(self.text.get(tk.SEL_FIRST, tk. SEL_LAST))

    def paste(self):
        #Is anything selected?
        if self.text.tag_ranges(tk.SEL):
            #keeping a reference on where the selection starts
            selFirstIndex = self.text.index(tk.SEL_FIRST)
            #removing selection first
            self.text.delete(tk.SEL_FIRST, tk. SEL_LAST)
            #copying from clipboard
            self.text.insert(selFirstIndex, self.text.clipboard_get())

        else:
            self.text.insert(tk.INSERT, self.text.clipboard_get())

    def delete(self):
        #Is anything selected?
        if self.text.tag_ranges(tk.SEL):
            self.text.delete(tk.SEL_FIRST, tk. SEL_LAST)


#MainMenu object that contains Sub-menus
class MainMenu(tk.Menu):
    def __init__(self, master):
        super().__init__(master, tearoff=0)
        self.master = master

        #create Menu Options
        self.file = FileMenu(self)
        self.add_cascade(label="File", menu=self.file)

        self.edit = EditMenu(self)
        self.add_cascade(label="Edit", menu=self.edit)


#Menu class that handles File Operations
class FileMenu(tk.Menu):
    def __init__(self, master):
        super().__init__(master, tearoff=0)
        self.master = master

        self.add_command(label="New                Ctrl + N")
        self.add_command(label="Open...          Ctrl + O")
        self.add_command(label="Save               Ctrl + S")
        self.add_command(label="Save As...      Ctrl + Shift + S")


        #add the line before exit
        self.add_separator()
        #destroy's the grandparent, which is assumed to be a toplevel BAD NAE BAD
        self.add_command(label="Exit                 Alt + F4", command=lambda : master.master.save_before_leave(self.master.master.destroy))


#Menu class that handles editorial operations
class EditMenu(tk.Menu):
    def __init__(self, master):
        super().__init__(master, tearoff=0)

        self.add_command(label="Cut              Ctrl + X")
        self.add_command(label="Copy           Ctrl + C")
        self.add_command(label="Paste          Ctrl + V")
        self.add_command(label="Delete         Delete")


#http://effbot.org/zone/tkinter-autoscrollbar.htm
class AutoScrollbar(tk.Scrollbar):
    # a scrollbar that hides itself if it's not needed.  only
    # works if you use the grid geometry manager.
    def set(self, lo, hi):
        if float(lo) <= 0.0 and float(hi) >= 1.0:
            self.grid_remove()
        else:
            self.grid()
        tk.Scrollbar.set(self, lo, hi)


def test():
    with open(__file__, "rU") as f:
        root.mainFrame.text.insert("1.0", f.read())


if __name__ == "__main__":
    root = MainWindow()
    #test()
    root.mainloop()

Review Concern(s):

  • My main concern is to code in a, easy to read, efficient, and well structured manner while still learning the language and concepts such as OOP. Feel free to mention tiniest issue or improvement that comes to your mind, as I am a beginner and I probably need it.

My own critique:

There are some inconsistencies between arguments to similar methods such as insert("1.0",...) and insert('1.0', ...) I want to structure my "" and '' usage so that I can better write and understand code but I am undecided which one to use in some situations.

I question the very necessity of MainFrame class, I think I should have removed it and assigned its children directly to MainWindow class instead.

Using self.master.something feels pretty wrong let alone using master.master.something but I am not entirely sure on what to do when there's a method that needs access to both a parent and a grandchildren's parameters, I would assume that's a parent's method than a children's.

I am most proud of the save_file and save_as_file methods as I believe they co-depend on each other very well. Still there's the issue of their level though.

There's the issue of encodings, which turned out to be a much harder problem than I initially anticipated. I had to use try, except for an easy way out.

\$\endgroup\$
  • 1
    \$\begingroup\$ You missed a line in the Rollback. Did you know there's an actual 'rollback' button in every revision of your question except the last? \$\endgroup\$ – Mast Dec 4 '17 at 15:05
  • \$\begingroup\$ @Mast Thanks for the help but I've read it's available if you have full edit privilege. Is it not? Though according to this the right thing to do is to flag as other and ask for a rollback. \$\endgroup\$ – Nae Dec 4 '17 at 15:42
  • 1
    \$\begingroup\$ I thought there was an exception for questions you own, but you could well be correct. Anyway, we got it nicely sorted out now. Feel free to find us in chat if you ever need help :-) \$\endgroup\$ – Mast Dec 4 '17 at 16:46
2
\$\begingroup\$

Few things I would bring your attention to:

  • naming. It is recommended by the PEP8 styleguide and generally agreed to follow the lower_case_with_underscores variable and function naming style. You have violated it on multiple occasions - e.g. mainFrame, curFilePath, curFileCont etc.
  • comments are over-used. Code without comments might feel like an extreme and somewhat opinion-based, but it is generally a great way of thinking about self-documented code. Just to highlight a few spots - I don't see the usefulness of #path of the current file before the self.curFilePath = '' line - especially if you would name a variable current_file_path. And, you can and should convert some of the comments into proper Documentation Strings
  • code repetition. Don't repeat yourself. Think of extracting a method out of the save_before_leave() method - look how the bodies of the two if blocks are identical to each other
\$\endgroup\$
  • 1
    \$\begingroup\$ Based on your last point I noticed that they can be identical(initially they called different save methods) and then written a new method response_handle for that part, and updated my code. Will work on other points as well. Thanks a lot for the review. \$\endgroup\$ – Nae Dec 4 '17 at 14:44
  • 3
    \$\begingroup\$ @Nae Please see this Meta post, you might want to rollback your last edit codereview.meta.stackexchange.com/questions/1763/… \$\endgroup\$ – Ludisposed Dec 4 '17 at 14:52
-1
\$\begingroup\$
#required for anything GUI related
import tkinter as tk
#required for fileopen and save options
from tkinter import filedialog
#required for file's basename
import os
#required for save_before_leave basically
from tkinter import messagebox

#Nae Unicode text entering is missing

class MainWindow(tk.Tk):
    def __init__(self):
        super().__init__()
        self.title("Untitled - NaePad")

        self.mainFrame = MainFrame(self)
        self.mainFrame.pack(fill="both", expand=True)

        self.mainFrame.text.focus_set()
        self.protocol("WM_DELETE_WINDOW", lambda : self.save_before_leave(self.destroy))

    def save_before_leave(self, callback, *args):

        textBuffer = self.mainFrame.text.get('1.0', 'end-1c')

        #if there's a current file
        if self.mainFrame.curFilePath:
            #if it content doesn't match the current buffer
            if self.mainFrame.curFileCont != textBuffer:
                self.response_handle(callback)

            else:
                callback()

        else:
            #if there's any text in the buffer
            if textBuffer:
                self.response_handle(callback)
            else:
                callback()
        #required in order to prevent "tagbinds" from happening
        return "break"

    def response_handle(self, callback):
        response = messagebox.askyesnocancel("NaePad - Unsaved File", "Do you want to save before leaving?")
        #if response is yes
        if response:
            self.mainFrame.save_file()
            callback()
        #if response is no
        elif response is False:
            callback()



class MainFrame(tk.Frame):
    def __init__(self, master):
        super().__init__(master)
        self.master = master

        #creates and displays the Menu
        self.menu = MainMenu(master)
        master.config(menu=self.menu)

        #path of the current file
        self.curFilePath = ''

        #current file
        self.curFileCont = ''

        self.text = tk.Text(self, wrap="none")
        #color scheme
        self.text.config(bg='#282c34',fg='#abb2bf', selectbackground='#3e4451')
        self.text.grid(row=0, column=0, sticky="nsew")
        self.rowconfigure(0, weight=1)
        self.columnconfigure(0, weight=1)

        self.scroll_bar_config()
        self.menu_command_config()
        self.key_binds_config()


    #scrollbar configurations for self.text widget
    def scroll_bar_config(self):

        self.scrollY = AutoScrollbar(self, orient="vertical", command=self.text.yview)
        self.scrollY.grid(row=0, column=1, sticky="nsew")
        self.text['yscrollcommand'] = self.scrollY.set

        self.scrollX = AutoScrollbar(self, orient="horizontal", command=self.text.xview)
        self.scrollX.grid(row=1, column=0, sticky="nsew")
        self.text['xscrollcommand'] = self.scrollX.set

    def menu_command_config(self):
        #New
        self.menu.file.entryconfig(0, command=lambda : self.master.save_before_leave(self.new_file))
        #Open...
        self.menu.file.entryconfig(1, command=lambda : self.master.save_before_leave(self.open_file))
        #Save
        self.menu.file.entryconfig(2, command=self.save_file)
        #Save as
        self.menu.file.entryconfig(3, command=self.save_as_file)

        #Cut
        self.menu.edit.entryconfig(0, command=self.cut)
        #Copy
        self.menu.edit.entryconfig(1, command=self.copy)
        #Paste
        self.menu.edit.entryconfig(2, command=self.paste)
        #Delete
        self.menu.edit.entryconfig(3, command=self.delete)

    def key_binds_config(self):
        self.text.bind('<Control-n>', lambda event : self.master.save_before_leave(self.new_file))
        self.text.bind('<Control-N>', lambda event : self.master.save_before_leave(self.new_file))
        self.text.bind('<Control-o>', lambda event : self.master.save_before_leave(self.open_file))
        self.text.bind('<Control-O>', lambda event : self.master.save_before_leave(self.open_file))
        self.text.bind('<Control-s>', self.save_file)
        self.text.bind('<Control-S>', self.save_file)
        self.text.bind('<Control-Shift-s>', self.save_as_file)
        self.text.bind('<Control-Shift-S>', self.save_as_file)


    #method handling file operations with encoding in mind
    def file_operation(self, file_path, file_opt='r'):
        #if file_path exists
        if file_path:
            try:
                #open only UTF-8 encoded files
                with open(file_path, file_opt, encoding="UTF-8") as f:
                    self.data_operation(f, file_opt)
            #if it's not UTF-8 then
            except UnicodeDecodeError:
                #open as ANSI encoding
                with open(file_path, fil_opt, encoding="ANSI") as f:
                    self.data_operation(f, file_opt)

            #update current file path
            self.curFilePath = file_path

            #update window name to file name BAD NAE BAD
            self.master.title(os.path.basename(f.name) + " - NaePad")

        #If it's a save operation and there's no file_path
        elif ((not file_path) and (file_opt == 'w')):
            self.save_as_file()

    #data writing and reading method
    def data_operation(self, file_data, file_opt):
        #read data
        if file_opt == 'r':
            self.text.delete("1.0", "end")
            self.curFileCont = file_data.read()
            self.text.insert("1.0", self.curFileCont)
        #write data
        elif file_opt == 'w':
            self.curFileCont = self.text.get('1.0', 'end-1c')
            file_data.write(self.curFileCont)


    def new_file(self, *args):
        self.text.delete("1.0", "end")
        self.curFilePath = ''
        self.curFileCont = ''

        #update window name to file name BAD NAE BAD
        self.master.title("Untitled - NaePad")

    def open_file(self, *args):

        #get filepath from user with gui
        filePath = filedialog.askopenfilename(filetypes=(("Text files", "*.txt"),("All files", "*.*")))

        self.file_operation(filePath, 'r')

    def save_file(self, *args):
        self.file_operation(self.curFilePath, 'w')

    def save_as_file(self, *args):
        self.curFilePath = filedialog.asksaveasfilename(defaultextension=".txt", filetypes=(("Text files", "*.txt"),("All files", "*.*")))
        #Checks wether a file is selected or not
        if self.curFilePath:
            self.save_file()


    def cut(self):
        #Is anything selected?
        if self.text.tag_ranges(tk.SEL):
            self.text.clipboard_clear()
            #append to cleared clipboard the (selection)
            self.text.clipboard_append(self.text.get(tk.SEL_FIRST, tk. SEL_LAST))
            self.text.delete(tk.SEL_FIRST, tk. SEL_LAST)

    def copy(self):
        #Is anything selected?
        if self.text.tag_ranges(tk.SEL):
            self.text.clipboard_clear()
            #append to cleared clipboard the (selection)
            self.text.clipboard_append(self.text.get(tk.SEL_FIRST, tk. SEL_LAST))

    def paste(self):
        #Is anything selected?
        if self.text.tag_ranges(tk.SEL):
            #keeping a reference on where the selection starts
            selFirstIndex = self.text.index(tk.SEL_FIRST)
            #removing selection first
            self.text.delete(tk.SEL_FIRST, tk. SEL_LAST)
            #copying from clipboard
            self.text.insert(selFirstIndex, self.text.clipboard_get())

        else:
            self.text.insert(tk.INSERT, self.text.clipboard_get())

    def delete(self):
        #Is anything selected?
        if self.text.tag_ranges(tk.SEL):
            self.text.delete(tk.SEL_FIRST, tk. SEL_LAST)


#MainMenu object that contains Sub-menus
class MainMenu(tk.Menu):
    def __init__(self, master):
        super().__init__(master, tearoff=0)
        self.master = master

        #create Menu Options
        self.file = FileMenu(self)
        self.add_cascade(label="File", menu=self.file)

        self.edit = EditMenu(self)
        self.add_cascade(label="Edit", menu=self.edit)


#Menu class that handles File Operations
class FileMenu(tk.Menu):
    def __init__(self, master):
        super().__init__(master, tearoff=0)
        self.master = master

        self.add_command(label="New                Ctrl + N")
        self.add_command(label="Open...          Ctrl + O")
        self.add_command(label="Save               Ctrl + S")
        self.add_command(label="Save As...      Ctrl + Shift + S")


        #add the line before exit
        self.add_separator()
        #destroy's the grandparent, which is assumed to be a toplevel BAD NAE BAD
        self.add_command(label="Exit                 Alt + F4", command=lambda : master.master.save_before_leave(self.master.master.destroy))


#Menu class that handles editorial operations
class EditMenu(tk.Menu):
    def __init__(self, master):
        super().__init__(master, tearoff=0)

        self.add_command(label="Cut              Ctrl + X")
        self.add_command(label="Copy           Ctrl + C")
        self.add_command(label="Paste          Ctrl + V")
        self.add_command(label="Delete         Delete")


#http://effbot.org/zone/tkinter-autoscrollbar.htm
class AutoScrollbar(tk.Scrollbar):
    # a scrollbar that hides itself if it's not needed.  only
    # works if you use the grid geometry manager.
    def set(self, lo, hi):
        if float(lo) <= 0.0 and float(hi) >= 1.0:
            self.grid_remove()
        else:
            self.grid()
        tk.Scrollbar.set(self, lo, hi)


def test():
    with open(__file__, "rU") as f:
        root.mainFrame.text.insert("1.0", f.read())


if __name__ == "__main__":
    root = MainWindow()
    #test()
    root.mainloop()

Update 1: I have written a new method response_handle for the response handle to be used in 2 places in order to reduce repetition as suggested in this answer. Also for a similar reason I've written file_operation and data_operation methods to be used in save_file and open_file methods as they were very similar.

\$\endgroup\$
  • 1
    \$\begingroup\$ This is CodeReview you post alternative code, but don't post a review of the code. codereview.meta.stackexchange.com/questions/8403/… \$\endgroup\$ – Ludisposed Dec 4 '17 at 17:17
  • \$\begingroup\$ @Ludisposed I don't understand how this is unfitting in accordance to your link and your other link? \$\endgroup\$ – Nae Dec 4 '17 at 17:32
  • 1
    \$\begingroup\$ You can answer your own code, but don't just dump it here. Explain your reasoning behind the changes should be enough. You can link to points in @alexce answer \$\endgroup\$ – Ludisposed Dec 4 '17 at 17:42

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.