Replacing Skype, reinventing the chat client

Question

I'm creating a chat client and chat server after a several month programming hiatus. The goal is to make one with features I want but it's also a learning project. Although it's mostly because I'm annoyed at cough skype.

I've realised I don't know if anything I've written currently is any good (never had anyone critique my code) so I'd like to know if the structure of the program is any good (especially how I handle tkinter and classes) and my use of control flow etc.

The code currently has 2 classes, one that contains the GUI elements (tkinter) and the other handles the networking side (threading and socket). It creates a thread for the listen method so it is constantly awaiting data from the server. Whilst the initial thread/process is used for the tkinter mainloop.

You can get/see the tkHyperlinkManager file here and the server here

try: #if python3
    from tkinter import *
    import  tkinter.font as tkFont
except: #if python2
    from Tkinter import *
    import tkFont

from socket import *
from threading import *
import time,sys, json,tkHyperlinkManager, webbrowser #tkHyperlinkManager is another file, I didn't write it.

class chatGUI: #Class That Handles GUI related tasks.
    def __init__(self):#Initializing window settings
        self.bgColour = "#2a2a2a" #First COlour was: "#607D8B"
        window.title("Oke's Chat Client")
        window.configure(bg=self.bgColour)
        self.chat = Frame(window,bg=self.bgColour)
        self.menu = Frame(window,bg=self.bgColour)
        self.checkFile() #Check for options/settings configuration
        menuBar = Menu(window,foreground=self.bgColour,activeforeground=self.bgColour)
        menuBar.add_command(label="Options", command=self.optionMenu)
        window.config(menu=menuBar)
        self.chatFrame()#Loads Chat GUI into memory.
        #Load the first Menu
        self.menuFrame()#Loads the MENU GUI into memory.
        self.menu.place(y=0,x=0,height=500,width=500)
        self.hyperList = ["http://","www.","https://","ftp://"]#Contains hyperlink triggers.

    def menuFrame(self):#Load in all widgets on main menu
        #BG IMAGE
        bgImg = PhotoImage(file="Images/bgImg.gif")
        bgLabel = Label(self.menu,image=bgImg,bg=self.bgColour)
        bgLabel.image=bgImg
        bgLabel.place(x=0,y=0)
        #Error
        self.Error = Label(self.menu,text="Unable to connect to server\n")
        #Label Msg
        labelImg = PhotoImage(file="Images/menu_text.gif")
        label = Label(self.menu,image=labelImg,bg=self.bgColour)
        label.image = labelImg
        label.place(y=75,x=100)
        #Entry Widget
        self.aliasEntry = Entry(self.menu)
        self.aliasEntry.place(y=100,x=190)
        #Connect Button
        buttonImg =PhotoImage(file="Images/buttonImg.gif")
        button = Button(self.menu,text="Connect",command=self.switchToChat,image=buttonImg,bg=self.bgColour,borderwidth=0)
        button.image = buttonImg
        button.place(y=235,x=200)
        #Window Settings
        window.resizable(width=FALSE, height=FALSE)
        window.minsize(500,500)

    def chatFrame(self):#Load in all chat GUI widgets and related objects
        #Set up fonts and StringVars
        bold_font = tkFont.Font(family="Helvetica",size=10,weight="bold")
        norm_font = tkFont.Font()
        sv= StringVar() #stringVar to hold string from Entry Widget
        sv.trace("w", lambda name, index, mode, sv=sv: self.checkEntry(sv)) #Calls self.checkEntry when ever entryStr String Var is changed.
        #Scrollbar and the chatText widgets
        self.scrollBar = Scrollbar(self.chat)
        self.scrollBar.grid(column=1,row=0,sticky=N+S+E+W)
        self.mainText = Text(self.chat, wrap=WORD,bg=self.bgColour,state=DISABLED,yscrollcommand=self.scrollBar.set)
        self.mainText.grid(column=0,row=0,sticky=N+S+E+W)
        self.mainText.tag_configure("bold", font=bold_font, foreground="#7dbcc1")
        self.mainText.tag_configure("normal", font=norm_font, foreground ="white")
        self.scrollBar.config(command=self.mainText.yview)
        #Userbar
        self.userBar = Text(self.chat,width=20,bg=self.bgColour,fg="white",state=DISABLED)
        self.userBar.grid(column=2, row=0,sticky=N+S+E+W)
        self.userBar.tag_configure("bold", font=bold_font, foreground="#7dbcc1")
        #TextEntry
        self.textEntry = Entry(self.chat,textvariable=sv,bg =self.bgColour,fg="white")#Note, textvar set to entryStr, text entered is stored in that var.
        self.textEntry.grid(row=1,column=0,sticky=N+S+E+W,rowspan=2)
        #make hyperlink class
        self.hyperlink = tkHyperlinkManager.HyperlinkManager(self.mainText)
        #Send button
        Button(self.chat, text="Send",command= lambda: loadNet.speak(self.alias, self.textEntry),bg=self.bgColour,fg="white").grid(column=2,row=1,sticky=NW)
        #Grid Geometry Config to make only chat box resizable.
        self.chat.columnconfigure(0,weight=1)
        self.chat.columnconfigure(2,minsize=140)
        self.chat.rowconfigure(0,weight=1)  

    def optionMenu(self):#Load in all widgets on option Menu
        self.timeStamp = IntVar()
        self.hourStamp = IntVar()
        self.timeStamp.set(self.optionData["timeStamp"])
        self.hourStamp.set(self.optionData["timeSet"])
        self.optionWindow = Toplevel(bg=self.bgColour,height=200,width=200)
        self.optionWindow.title("ChatClient Options")
        Checkbutton(self.optionWindow, text="TimeStamp", variable=self.timeStamp).pack()
        Checkbutton(self.optionWindow, text="Use 24 Hour timestamp", variable=self.hourStamp).pack()
        Button(self.optionWindow,text="Apply", command=self.saveSettings).pack()

    def switchToChat(self): #handles closing menu and switching to chat and calls connect function
        self.alias = self.aliasEntry.get() #Grabs alias entered at menu
        if self.alias.isspace() == False and self.alias != "" and len(self.alias) < 16:#make sure name is under 16 chars and isnt whitespace.
            try:#Try Connect to Server.        
                    loadNet.connect(self.alias)#Replace this line with pass if you wish to run without a server.
            except:#Unpack chat, repack menu.
                    print("Unable to connect to server")
                    self.chat.pack_forget()
                    self.menu.place(y=0,x=0,height=500,width=500)
                    self.Error.pack()
            else:#Called only if try works, if it connects.
                window.bind("<Return>", lambda event: loadNet.speak(self.alias, self.textEntry))
                self.menu.place_forget() #Remove menu
                self.chat.pack(fill=BOTH, expand=YES)#put chat GUI in.
                window.resizable(width=TRUE, height=TRUE)
                window.minsize(500,410)


    def checkFile(self): #Function handles reading in the settings from txt file, if try fails it meants it's unreadable or doesn't exist and makes a new file.
        try:
            optionFile = open("options.txt")
            self.optionData = json.loads(optionFile.read())
        except:
            print("Options Configuration File Missing. Or Unreadable. \n Creating new file...")
            optionFile = open("options.txt","w+")
            optionFile.truncate()
            Dict = {
                "timeStamp": 1,
                "timeSet": 1
            }
            Dict = json.dumps(Dict)
            optionFile.write(Dict)
            optionFile.close()
            optionFile = open("options.txt")
            self.optionData = json.loads(optionFile.read())
        optionFile.close()

    def saveSettings(self): #Save setting vars to txt file. Called by Apply button push.
        self.optionData["timeStamp"] = self.timeStamp.get()
        self.optionData["timeSet"] = self.hourStamp.get()
        optionFile = open("options.txt","w+")
        optionFile.truncate()
        optionFile.write(json.dumps(self.optionData))
        optionFile.close()        

    def checkEntry(self, entryStr): #checks the text entry,sets the contents of Entry widget to max of 1000 chars, called when ever text is entered.
        c = entryStr.get()[0:1000]
        entryStr.set(c)

    def displayData(self,data):#Handles displaying message data.
        print(data)
        self.mainText.config(state=NORMAL)
        if self.optionData["timeStamp"]:#if using timestamp
            if self.optionData["timeSet"]:#if using 24HR
                self.mainText.insert(END, time.strftime('%H:%M', time.localtime()) + " - "+str(data[0]), 'bold')#24 hour
            else:
                self.mainText.insert(END, time.strftime('%I:%M', time.localtime()) + " - "+str(data[0]), 'bold')#12 HOUR
        else:#No timestamp
            self.mainText.insert(END, str(data[0]), 'bold')#No TimeStamp
        msgSplit = data[1].split()#Split message up for analysis of hyperlinks
        for msgDat in msgSplit:#Check message for hyperlinks.
            for hyperID in self.hyperList:# check for different types of hyper link triggers
                if msgDat.startswith(hyperID):
                    self.mainText.insert(END, str(msgDat), self.hyperlink.add(lambda:webbrowser.open(msgDat)))#Make the message a hyperlink that opens webbrowser
                    self.mainText.insert(END, " ")
                    break
            else:
                self.mainText.insert(END, str(msgDat)+" ", "normal")
        self.mainText.insert(END, "\n")        
        self.mainText.config(state=DISABLED)
        self.mainText.see("end") #Makes view of text box at bottom so you don't have to scroll down. Looking at you skype >:(

    def modUserBar(self,userList):#Called when someone connects/disconnects. Modifies the userbar which is a text widget.
        self.userBar.config(state=NORMAL)
        self.userBar.delete(1.0, END)#Empty it out
        for user in userList:
            self.userBar.insert(END, str(user)+"\n", 'bold')
        self.userBar.config(state=DISABLED)


class netMan:#Handles network related part of program
    def __init__(self):
        self.s = socket(AF_INET, SOCK_STREAM)#Make socket object

    def connect(self,alias):
        self.s.connect(('192.168.1.97',11116))
        self.s.send(alias.encode('utf-8'))#Send alias
        listenThread = Thread(target=self.listen)#Define the listening thread, required because window.mainloop :(
        listenThread.start()#Start the thread!

    def listen(self):#Loop forever in thread
        while True:
            try:
                data =  self.s.recv(3000)
            except:
                print("Disconnected from server")
                break
            try:
                dataDecode = json.loads(data.decode('utf-8'))
            except:
                print("json decoding error\n")
            else:
                if dataDecode[0] == 1:
                    loadGUI.displayData(dataDecode[1:])
                elif dataDecode[0] == 0:
                    loadGUI.modUserBar(dataDecode[1])
        self.s.close()

    def speak(self, alias, textEntry, event=None): #Called when ever send button pushed or Enter is pressed
        msg = textEntry.get()
        if msg != "": #no whitespace!
            packet= json.dumps([1,alias+": ",msg],ensure_ascii=False)
            textEntry.delete(0,END)
            try:
                self.s.send(packet.encode('utf-8'))
            except:
                print("unable to reach server...?\n")

window = Tk()
loadNet = netMan()
loadGUI = chatGUI()

window.mainloop()

Answer 1

In your chatGUI's __init__ your setting up some constants, that should really just be defined as constants at the class level:

class chatGUI: #Class That Handles GUI related tasks.
    BGCOLOUR = "#2a2a2a"  # First Colour was: "#607D8B"
    # Contains hyperlink triggers.
    HYPER_LIST = ("http://", "www.", "https://", "ftp://")
    WINDOW_TITLE = "Oke's Chat Client"

    def __init__(self):#Initializing window settings
        window.title(self.WINDOW_TITLE)
        window.configure(bg=self.BGCOLOUR)
        self.chat = Frame(window, bg=self.BGCOLOUR)
        self.menu = Frame(window, bg=self.BGCOLOUR)

This makes it clearer that these are constants that are not dependent on how an instance is initialised, they are always the same. There are others that could definitely go here too, not just things from __init__. The path to your bgImg for instance, the size and position for your menu. You should use constants for most values that are fixed and predefined. Having them at the top of the class or top of the script makes it easy for people to refer back to what they all are, and allows for easier tweaking if you need to adjust them later.

Now you have some commenting issues. All of the comments in the code above could probably be edited or removed. First, use docstrings when you want to explain what a class or function is:

class chatGUI: 
    """Handles GUI related tasks."""

This is the convention, and it's accessible programmatically so it's more helpful than a comment. Next, does a user need to know what the "First Colour" was? If there's a reason it's not entirely clear. You don't need to comment about something unless it's actually relevant and necessary.

Instead of commenting # Contains hyperlink triggers rename your list HYPERLINK_TRIGGERS so that it's always clear what it contains. Lastly people generally know what __init__ is so there's not much need to comment what that does. Only note if something is unusual. Though in this case I might argue that you should explain where window comes from. You use it heavily in this initialisation and it confused me at first. It may just be a tkinter thing (I'm not familiar with tkinter) but if this isn't common you should explain it, or better yet pass window to __init__ to make it explicitly connected.

In switchToChat you use some unusual logic in your if call. Instead of using == False it's best to use not.

    if not self.alias.isspace() and 

You can test self.alias != "" just using Python's truthiness, where if self.alias will evaluate as True for any non empty string, which is exactly what you need. But since you're using len anyway, it would be easier to just compare there:

    if not self.alias.isspace() and 0 < len(self.alias) < 16:

Now you're ensuring len is between 0 and 16 characters. One note, you're not accounting for trailing whitespace. If I enter "SuperBiasedMan " then I'll fail the check, but commonly people strip out the whitespace from inputs. I'd recommend calling .strip() on alias, which also means you could remove the isspace() test and just check the length. It's also odd that you're not doing anything for an invalid alias. You should at least print an explanation, ideally saying what parameters are valid so they can re-enter them.

You should not use a bare except to swallow exceptions. That means literally any error will be ignored. If there are many unpredictable possible errors, at least print the error so the user can read what happened

except Exception as error:
    print("Unable to connect to server")
    print(error.message)

In checkFile you should use with when opening a file. with ensures that the file is closed regardless of what exceptions may occur. It's the safest way to open and close your file. You also use json.loads(optionFile.read()) but you can juse use json.load(optionFile) and then the json module can read the file itself.

    try:
        with open("options.txt") as optionFile:
            self.optionData = json.load(optionFile)

You can do the same with json.dump to not have to write the file directly:

        with open("options.txt", "w") as optionFile:
            self.optionData = {
                            "timeStamp": 1,
                            "timeSet": 1
                         }
            json.dump(self.optionData, optionFile)

Note I removed the truncating, changed to "w" mode and skipped the file reading because they seemed unnecessary. You don't need to read the information you've just written, you still have the dictionary. There's also no need to call truncate, the whole file is wiped when you open it with write mode anyway. And 'w+' mode is just when you want to open a file to read and to write, which you don't need here. You can similarly trim this in saveSettings

def saveSettings(self):
    """Save setting vars to txt file. Called by Apply button push."""

    with open("options.txt", "w") as optionFile:
        json.dump(self.optionData, optionFile)

In displayData you have two long very similar lines with small differences. It would be more readable to only adjust the small difference in the if else blocks and then keep the full line after:

    if self.optionData["timeStamp"]:
        if self.optionData["timeSet"]:  # Using 24HR
            time_format = '%H:%M'
        else:
            time_format = '%I:%M'
        self.mainText.insert(END, time.strftime(time_format, time.localtime()) + " - "+str(data[0]), 'bold')

Answer 2

Don't use wildcard imports

Instead of this:

from tkinter import *

Do this:

import tkinter as tk

With the latter, you then need to prefix all of your tk commands and classes with tk. (eg: tk.Tk()). This is a tiny bit of extra typing, but makes your code more readable. At some point you may choose to use ttk widgets which defines many classes with the same name as tkinter classes. By prefixing your classes you make it explicit which library you're pulling from.

PEP8 explicitly states that wildcard imports should be avoided, and the Zen of Python says that explicit is better than implicit.

Don't use place

pack and grid are almost always a better choice than place if you're concerned about a GUI that behaves well when resized, when run with different fonts, and when run on systems with different resolutions.

---

Separate widget creation from widget layout

You have lots of places where you will create a widget, configure it, call place, create another widget, configure it, call place, etc. This type of code organization makes the GUI difficult to visualize, and difficult to modify. I recommend separating the creation of widgets from the layout of widgets:

bgLabel = Label(self.menu,image=bgImg,bg=self.bgColour)
self.Error = Label(self.menu,text="Unable to connect to server\n")
label = Label(self.menu,image=labelImg,bg=self.bgColour)
...
bgLabel.place(x=0,y=0)
label.place(y=75,x=100)
...

Use more whitespace

Your code uses very little extra whitespace, making the code dense and difficult to read. Break up your code into logical sections separated by a blank line.

Give your functions docstrings

Instead of this:

def optionMenu(self):#Load in all widgets on option Menu

Do this:

def optionMenu(self):
    """This creates the chat client options menu"""

---

Use explicit path to options.txt

You are opening the file options.txt without any path information. The user could use this program from anywhere, do you really intend for "options.txt" to be written wherever the user starts the program? You should add an explicit path to the user's home directory.

Answer 3

Apart from the issues pointed out by other people, there are numerous problems with the way you use TCP sockets. There are many intricacies to using raw sockets which you might not be aware of.

TCP is stream oriented

One send call on one end does not equal one return value from recv on the other end. You might get a part of what you sent, or a combination of multiple sent messages, or anything inbetween. The only guarantee made by TCP (to an extent) is that the bytes you sent arrive unmodified in the exact same order. But there are no reliable packet boundaries visible to you.

If you are running this on localhost or a LAN it might seem that one sent message equals one packet and one recv return value - but this is not guaranteed in any way.

To be able to tell messages apart you need a framing protocol. This could be as simple as a linefeed between each message that you can look for - provided that you never have linefeeds in your messages (JSON doesn't contain unexpected linefeeds if you turn pretty printing off).

The return value of send is meaningful

send may not send the entire string, so you need to check the return value which indicates the number of bytes sent and send the rest later if not everything was sent.

Alternatively you can use Python's sendall convenience method instead. Do note that this may block for a long time (unlikely to be significant for a chat client, but good to know in general)

Disconnection is not reliably detected

In practice any TCP based protocol will probably need a "ping" message that is sent back and forth every once in a while to make sure the connection is still alive. A non-graceful disconnection of a TCP socket cannot be detected unless you try to send something, and even then the OS might silently try to resend for an absurdly long time before reporting an error. Without pings the connection can die without warning.

Also, recv returns an empty string on a graceful disconnection. It does not throw an exception. You don't seem to handle this case.

There is no reconnection logic

This is essential in practice or your program is going to be very annoying to use in the long run. Server restarts, dropped wi-fi connections, let alone mobile connections cause TCP connections to drop all the time and users generally expect programs to reconnect seamlessly.