3
\$\begingroup\$

I'm new to OOP and not sure if I'm importing and using my class correctly.

I'm writing a simple app called MyTestApp that uses a tkinter GUI. I have a textbox widget and I created a popup menu that is bound to that widget.

But my code started to look a little messy so I decided to move all the popup related methods and variables to its own class. And also decided to write it that I can reuse my popup class for other projects in the future that will require a popup menu.

I instantiate my popup class like this: self.popup = p(parent)

Then I assign the self.popup.textbox_text instance variable in the popup class to the self.data_textbox instance variable in my MyTestApp class like so:

self.popup.textbox_text = self.data_textbox

Everything works correctly, but I'm not sure if this is the correct way of doing it. I initially was thinking that I should pass self.data_textbox as a parameter when instantiating the class but was having a hard time to get it too work like that.

Any tips or advice I can get will be greatly appreciated. I would prefer to do things the correct way from the start.

main.py:

import tkinter as tk
from tkinter import Tk, ttk, Frame, FALSE, Text, Button, \
    Scrollbar, Entry, END, INSERT
from popup import Popup as p


class MyTestApp(Frame):

    def __init__(self, parent, *args, **kwargs):
        Frame.__init__(self, parent, *args, **kwargs)
        self.root = parent
        self.root.title('MY Test App')
        self.grid(column=0, row=0, sticky='nsew', padx=12, pady=5)

        self.data_textbox = Text(self, borderwidth=2, relief='sunken')
        self.data_textbox.config(height=30, width=80)
        self.data_textbox.grid(row=1, column=0, sticky="new")
        self.exit_btn = ttk.Button(self, text='Exit', command=self.on_exit)
        self.exit_btn.grid(row=2, column=0, sticky='W', pady=15)

        ## Not sure if im doing the below part correct
        ## Instantiate my popup class
        self.popup = p(parent)
        ## Assign self.popup.textbox_text to self.data_textbox
        self.popup.textbox_text = self.data_textbox
        # Add imported popup menu and bind to textbox widget
        self.data_textbox.bind("<Button-3>", self.popup.text_popup)

    # Exit the program. Linked to the Exit Button
    def on_exit(self):
        self.root.destroy()


def main():
    root = Tk()
    MyTestApp(root)
    root.mainloop()


if __name__ == '__main__':
    main()

popup.py:

import tkinter as tk
from tkinter import Frame, Entry, END, INSERT


class Popup(Frame):
    def __init__(self, parent, *args, **kwargs):
        Frame.__init__(self, parent, *args, **kwargs)

        ### Not sure if this below part is correct
        ### Must it rather be set from the init parameter?
        self.textbox_text = ''

        self.text_widget = tk.Menu(self, tearoff=0, relief='sunken')
        self.text_widget.add_command(label="Copy", command=self.text_copy)
        self.text_widget.add_separator()
        self.text_widget.add_command(label="Paste", command=self.text_paste)
        self.text_widget.add_separator()
        self.text_widget.add_command(label="Clear", command=self.text_clear)


    def text_popup(self, event):
        self.text_widget.post(event.x_root, event.y_root)

    def text_copy(self, event=None):
        self.clipboard_clear()
        text = self.textbox_text.get("sel.first", "sel.last")
        self.clipboard_append(text)

    def text_paste(self):
        self.textbox_text.insert(INSERT, self.clipboard_get())

    def text_clear(self):
        self.textbox_text.delete(1.0, END)
\$\endgroup\$
  • \$\begingroup\$ When you say: I instantiate my popup class like this: self.popup = p(parent) that can not work because to do that you have to code self.popup = Popup(parent). Also it seems the Popup() class is in a separate module, which your actual code does not reflect within the MyTestApp() class \$\endgroup\$ – Billal Begueradj Oct 24 '17 at 6:54
  • \$\begingroup\$ @Billal BEGUERADJ I did not use ` self.popup = Popup(parent)` as I imported Popup as p from popup import Popup as p Yes, the popup class is in a separate module called popup. I'm sorry if that was unclear, I should have worded this section better. so I decided to move all the popup related methods and variables to its own class. And also decided to write it that I can reuse my popup class for other projects in the future I'm not sure what you mean when you say it does not reflect in my code within in my MyTestApp() \$\endgroup\$ – Peter Ackermann Oct 26 '17 at 0:52
  • \$\begingroup\$ Sorry, I did not see that import ... now I understand ... on November 1st, I will post my answer. \$\endgroup\$ – Billal Begueradj Oct 30 '17 at 12:09
2
\$\begingroup\$

Fixing the bug

First of all, I would like to address an issue you did not ask about. It is related to the situation where the user tries to copy without selecting any text. That is exactly the scenario I faced. This situation triggers an exception:

Exception in Tkinter callback
Traceback (most recent call last):
  File "/usr/lib/python3.5/tkinter/__init__.py", line 1553, in __call__
    return self.func(*args)
  File "/home/begueradj/Desktop/popupkopy.py", line 26, in text_copy
    text = self.textbox_text.get("sel.first", "sel.last")
  File "/usr/lib/python3.5/tkinter/__init__.py", line 3095, in get
    return self.tk.call(self._w, 'get', index1, index2)
_tkinter.TclError: text doesn't contain any characters tagged with "sel"

To fix this issue, simply check if the user selected a text and then allow him to perform the copy action. This is something you can express within the body of text_copy() function as follows:

def text_copy(self, event=None):
    if self.textbox_text.tag_ranges("sel"):
        self.clipboard_clear()
        text = self.textbox_text.get("sel.first", "sel.last")
        self.clipboard_append(text)

Good decision?

But my code started to look a little messy so I decided to move all the popup related methods and variables to its own class. And also decided to write it that I can reuse my popup class for other projects in the future that will require a popup menu.

A part of the software design, consists in identifying entities (objects/classes) of the problem you are trying to resolve. Thinking of the popup men as a class in itself is a right decision.

Discussing the implementation from the OOP perspective

  1. Inside the Popup() class, you can get rid of this declaration self.textbox_text = '' as it is useless (never used, unlike what you may think) and a bit confusing. If you remove that variable, your program will still function correctly.
  2. I say self.textbox_text = '' is useless because what you actually are using in all Popup() functions is rather the attribute you added to the instance of Popup() inside MyTestApp() when you coded: self.popup.textbox_text = self.data_textbox. That is also why it is confusing.
  3. The remark 2 above means also that MyTestApp() and Popup() classes are not compliant with the OOP philosophy because the first one must not be responsible of "expanding" the list of attributes of the second one. Objects have rather a state (Python attributes, in our case) and a behavior defined through an interface (not the interface in the strict OOP concepts) consisting of a set of methods and functions. The communication between objects occurs through their interfaces (functions/methods). And that is exactly why I am telling you this line: self.popup.textbox_text = self.data_textbox does not comply with the OOP philosophy.
  4. How to fix the issue highlighted via the remark 3 above? Well, by just respecting the principles: a MyTestApp() instance must communicate with a Popup() instance (self.popup, in your case) through methods. I mean, you must find a way to communicate self.data_textbox to self.popup object through a suitable method. This is too easy and clean as you will see in the improved version below.

Few other remarks

The previous section, especially the remark 4 above, responds fully to your questions. However, I would like to add few improvements suggestions to your code which I think you need to take in consideration as they are the widely respected standard. I will list some of them briefly:

  1. Avoid wildcard imports. This is not what I recommend, but what PEP 8 asks us to do:

Wildcard imports (from import *) should be avoided, as they make it unclear which names are present in the namespace, confusing both readers and many automated tools. There is one defensible use case for a wildcard import, which is to republish an internal interface as part of a public API (for example, overwriting a pure Python implementation of an interface with the definitions from an optional accelerator module and exactly which definitions will be overwritten isn't known in advance).

When republishing names this way, the guidelines below regarding public and internal interfaces still apply.

What does this mean in practice? It simply means you should remove these 2 lines: from tkinter import Tk, ttk, Frame, FALSE, Text, Button, \ Scrollbar, Entry, END, INSERT and from tkinter import Frame, Entry, END, INSERT and be content with import tkinter as tk. This way you avoid useless imports you never used, but also you avoid repetition of the imports and, more importantly, in case you need to create a custom class/widget having the same name of one of the tkinter's classes, you will not face names clashes issues.

  1. Choose the right names: while this rule sounds easy to understand, not lot of us respect it even if it is too important for the development process. In my solution below, I renamed several of your variables.
  2. Last but not the least, follow Tkinter best practices.

Improved version

Taking in consideration all what is mentioned through the above sections, I came up with this improved version of your code (which surely can be subject to further improvements):

main.py:

from tkinter import ttk
import tkinter as tk
from popup import RightClickMenu 


class MyTestApp(tk.Frame):
   def __init__(self, parent):
       self.master = parent
       tk.Frame.__init__(self, self.master)        
       self.configure_gui()
       self.create_widgets()
       self.bind_right_click_menu_to_typing_area()

   def configure_gui(self):
       self.master.title('MY Test App')

   def create_widgets(self):
       self.create_text_area()
       self.create_exit_button()

   def create_text_area(self):
       self.text_area = tk.Text(self.master, borderwidth=2, relief='sunken')
       self.text_area.config(height=30, width=80)
       self.text_area.grid(row=0, column=0, sticky="new")

   def create_exit_button(self):
       self.exit_btn = ttk.Button(self.master, text='Exit', command=self.exit_application)
       self.exit_btn.grid(row=1, column=0, sticky='W', pady=15)

   def bind_right_click_menu_to_typing_area(self):
       self.popup = RightClickMenu(self.master, self.text_area)        
       self.text_area.bind("<Button-3>", self.popup.popup_text)

   def exit_application(self):
       self.master.destroy()

def main():
   root = tk.Tk()
   MyTestApp(root)
   root.mainloop()

if __name__ == '__main__':
   main()

popup.py:

import tkinter as tk


class RightClickMenu(tk.Frame):   
   def __init__(self, parent, text_area):
       self.master = parent
       tk.Frame.__init__(self, self.master)  
       self.text_area = text_area
       self.create_widgets()

   def create_widgets(self):
       self.create_right_click_menu()       

   def create_right_click_menu(self):
       self.right_click_menu = tk.Menu(self.master, tearoff=0, relief='sunken')
       self.right_click_menu.add_command(label="Copy", command=self.copy_text)
       self.right_click_menu.add_separator()
       self.right_click_menu.add_command(label="Paste", command=self.paste_text)
       self.right_click_menu.add_separator()
       self.right_click_menu.add_command(label="Clear", command=self.clear_text)

   def popup_text(self, event):
       self.right_click_menu.post(event.x_root, event.y_root)

   def copy_text(self):
       if self.text_area.tag_ranges("sel"):
           text = self.text_area.get("sel.first", "sel.last")
           self.clipboard_append(text)

   def paste_text(self):
       self.text_area.insert(tk.INSERT, self.clipboard_get())

   def clear_text(self):
       self.text_area.delete(1.0, tk.END)
\$\endgroup\$
  • \$\begingroup\$ from ... import ... is not strictly a wildcard import and is acceptable. \$\endgroup\$ – Daniel Nov 1 '17 at 18:18
  • \$\begingroup\$ Thanks so much @Billal BEGUERADJ, for taking the time to respond in detail. This will help me greatly. \$\endgroup\$ – Peter Ackermann Nov 2 '17 at 18:20
  • \$\begingroup\$ You are welcome. If later you encounter something unclear in my answer, feel free to ask for clarifications @PeterAckermann \$\endgroup\$ – Billal Begueradj Nov 2 '17 at 18:22

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.