7
\$\begingroup\$

First off

I'm quite new to Python, there will be a lot of messy/overcomplicated code, that's why I'm posting on this site.

This code is written in Python (2.7) using the Tkinter library.

Questions

To allow for viewing/editing large files, I am loading the file and saving it to a variable, then displaying only the text that can be seen. Is this the best way of going about this?

I have chosen to use a class here, because there are a lot of global variables. I also want to allow for scalability, so is this a good choice?

If I came back to this in a year or so, would I be able to understand what is going on?

Any and all criticism to do with formatting and Pythonicness is welcome.

Code

class Window():
    def __init__(self):
        """imports and define global vars"""
        import binascii, Tkinter, tkFileDialog
        self.binascii = binascii
        self.Tkinter = Tkinter
        self.tkFileDialog = tkFileDialog
        self.root = self.Tkinter.Tk()
        self.lineNumber = 0
        self.fileName = ""
        self.lines = []
        self.width = 47
        self.height = 20
        self.lastLine = 0.0
        self.rawData = ""
        self.defaultFiles = ( 
                             ("Hexadecimal Files",   "*.hex"),
                             ("Windows Executables", "*.exe"),
                             ("Linux Binaries",      "*.elf"),
                             ("all files",           "*.*")
                            )

    def resize(self, event = None):
        """called when the window is resized. Re-calculates
            the chars on each row"""
        self.width  = self.mainText.winfo_width() / 8
        self.height = self.mainText.winfo_height() / 16
        if not self.width / 3 == 0:
            self.data = self.binascii.hexlify(self.rawData)[2:]
            dataSave = self.data
            lines = []
            chars = self.width - (self.width / 3)
            while len(self.data) > 0:
                if len(self.data) >= chars:
                    lines.append(self.data[:chars])
                    self.data = self.data[chars:]
                else:
                    lines.append(self.data)
                    self.data = ""
            self.data = dataSave
            self.lines = lines
            self.mainText.delete("1.0","end")
            self.mainText.insert("1.0", self.getBlock(self.lineNumber))

    def openFile(self, filename):
        """Opens a file and displays the contents"""
        self.fileName = filename
        with open(filename,"rb") as f:
            rawData = chr(0) + f.read()
        self.rawData = rawData
        self.data = self.binascii.hexlify(rawData)[2:]
        dataSave = self.data 
        lines = []
        chars = self.width - (self.width / 3)
        print self.width
        while len(self.data) > 0:
            if len(self.data) >= chars:
                lines.append(self.data[:chars])
                self.data = self.data[chars:]
            else:
                lines.append(self.data)
                self.data = ""
        self.data = dataSave
        self.lines = lines
        self.mainText.delete("1.0","end")
        self.mainText.insert("1.0", self.getBlock(0))
        self.lineNumber = 0


    def saveFile(self, filename, data = None):
        """saves the 'lines' variable (keeps track
            of the data) to a file"""
        if data is None:
            data = "".join(self.lines)
        with open(filename, "wb") as f:
            f.write(self.binascii.unhexlify(data))

    def saveAll(self, event = None):
        """saves a file (for binding a key to)"""
        self.setBlock(self.mainText.get("1.0","end"),self.lineNumber)
        self.saveFile(self.fileName)

    def saveClose(self, event = None):
        """Saves and closes (for binding a key to"""
        self.saveAll()
        self.root.destroy()

    def saveAsWindow(self, event = None):
        """Opens the 'save as' popup"""
        f = self.tkFileDialog.asksaveasfilename(filetypes = self.defaultFiles)
        if f is None or f is "":
            return
        else:
            self.saveFile(f)
            self.fileName = f

    def openWindow(self, event = None):
        """Opens the 'open' popup"""
        f = self.tkFileDialog.askopenfilename(filetypes = self.defaultFiles)
        if f is None  or f is "":
            return
        else:
            self.openFile(f)

    def q(self, event = None):
        """quits (for binding a key to"""
        self.root.destroy()

    def neatify(self,data):
        """adds a space every 2 chars (splitss
           into bytes)"""
        out = ""
        for line in data:
            count = 0
            for char in line:
                if count == 2:
                    count = 0
                    out += " " + char
                else:
                    out += char
                count += 1
            out += "\n"
        return out

    def getBlock(self, lineNum):
        """gets a block of text with the line number
           corresponding to the top line"""
        self.formattedData = self.neatify(self.lines[lineNum:lineNum+self.height])
        return self.formattedData

    def setBlock(self, data, lineNum):
        """sets a block (same as getBlock but sets)"""
        rawData = data.replace(" ","").split("\n")
        data = []
        for line in rawData:
            if not line == "":
                data.append(line)
        if len(data) < self.height:
            extra = len(data)
        else:
             extra = self.height

        for i in range(lineNum,lineNum + extra):
            self.lines[i] = data[i - lineNum]

    def scrollTextUp(self, event = None):
        """Some may argue 'scrollTextDown' but
            this is what happens when you press
            the up arrow"""
        if not self.lineNumber <= 0:
            self.setBlock(self.mainText.get("1.0","end"),self.lineNumber)
            self.lineNumber -= 1
            self.mainText.delete("1.0","end")
            self.mainText.insert("1.0", self.getBlock(self.lineNumber))

    def scrollTextDown(self, event = None):
        """same as above except the opposite"""
        if not self.lineNumber >= len(self.lines) - self.height:
            self.setBlock(self.mainText.get("1.0","end"),self.lineNumber)
            self.lineNumber += 1
            self.mainText.delete("1.0","end")
            self.mainText.insert("1.0", self.getBlock(self.lineNumber))

    def scroll(self, event = None, direction = None):
        """calls the correct scroll function"""
        if self.mainText.index("insert").split(".")[0] == str(self.height + 1):
            self.scrollTextDown()
        elif self.mainText.index("insert").split(".")[0] == "1":
            cursorPos = self.mainText.index("insert")
            self.scrollTextUp()
            self.mainText.mark_set("insert", cursorPos)

    def defineWidgets(self):
        """defines the widgets"""
        self.menu = self.Tkinter.Menu(self.root)

        self.filemenu = self.Tkinter.Menu(self.menu, tearoff = 0)
        self.filemenu.add_command(label = "Save", command = self.saveAll, accelerator = "Ctrl-s")
        self.filemenu.add_command(label = "Save as...", command = self.saveAsWindow, accelerator = "Ctrl-S")
        self.filemenu.add_command(label = "Open...", command = self.openWindow, accelerator = "Ctrl-o")
        self.filemenu.add_separator()
        self.filemenu.add_command(label = "Quit", command = self.saveClose, accelerator = "Ctrl-q")
        self.filemenu.add_command(label = "Quit without saving", command = self.root.destroy, accelerator = "Ctrl-Q")

        self.menu.add_cascade(label = "File", menu = self.filemenu)

        self.mainText = self.Tkinter.Text(self.root, width = 47, height = 20)

    def initWidgets(self):
        """initialises the widgets. Also key bindings etc"""
        self.mainText.pack(fill = "both", expand = 1)
        self.mainText.insert("1.0", self.getBlock(0))

        self.root.config(menu = self.menu)

        #up and down bound to the scroll function to check if the text should scroll
        self.root.bind("<Down>", self.scroll)
        self.root.bind("<Up>", self.scroll)
        self.root.bind("<Control-s>", self.saveAll)
        self.root.bind("<Control-o>", self.openWindow)
        self.root.bind("<Control-S>", self.saveAsWindow)
        self.root.bind("<Control-q>", self.saveClose)
        self.root.bind("<Control-Q>", self.q)
        self.root.bind("<Configure>", self.resize)
        self.root.protocol('WM_DELETE_WINDOW', self.saveClose)


win = Window()
win.defineWidgets()
win.initWidgets()
win.root.mainloop()
\$\endgroup\$
  • 1
    \$\begingroup\$ This is the first time I come across the practice, so maybe I am missing something, but what are the benefits of binding modules to instance attributes? \$\endgroup\$ – Mathias Ettinger Sep 6 '18 at 7:25
  • \$\begingroup\$ @MathiasEttinger TBH I don't really know. I think I thought that if I just imported them in __init__ they wouldn't be available globally but I may change that as I believe it's not true. \$\endgroup\$ – Jachdich Sep 6 '18 at 7:51
  • 1
    \$\begingroup\$ Yes, if you import it in __init__ their name visibility will be limited to this method. \$\endgroup\$ – Mathias Ettinger Sep 6 '18 at 7:57
  • 3
    \$\begingroup\$ Don't add content from answers to your original question. Revise your code and post a new question instead. \$\endgroup\$ – yuri Sep 8 '18 at 16:23
  • 3
    \$\begingroup\$ For those in the reopen queue: note that the content about an error was added after advice from the answer was taken into consideration- please look at the revision history \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Sep 8 '18 at 16:33
4
\$\begingroup\$

Style

PEP8 is the de-facto standard style guide for Python and adhering to it will make your code look like Python code to others:

  • variable and method names should be snake_case;
  • imports should come at the top of the file ordered standard lib modules first and third party modules later;
  • arguments with default value should be defined without a space around the = sign.

You should also put the top-level code under an if __name__ == '__main__' guard to ease testing and reusability.

Also:

  • this print in the middle of the code feels like debugging information, you should remove it;
  • Tkinter is usually imported as tk;
  • some of the docstrings are just repeating the method names and are not usefull, besides their formatting feels weird. See PEP257 for hindsights.

Code organization

You have several place where code is duplicated and could benefit from refactoring, such as opening a file — resizing the window, scrolling up — scrolling down, saving the current content of mainText into memory…

You also have the defineWidgets and initWidgets functions that need to be called by the users of your class before doing anything with it. You should avoid such situation by calling them yourself in your constructor.

I would also try to organize the method of your class by logical groups so it is easier to follow. Widget-related stuff, file-content related stuff, popup-related stuff, and view-window related stuff can be a good hierarchy.

Processing file content

In two places, you need to create groups of data of a certain length (when you open a file/resize the window and in neatify). There is a neat itertools recipe for that: grouper. If you adapt it to work only with characters, it can become:

def character_grouper(iterable, n):
    """Group consecutive n values of iterable into tuples.

    Pad the last tuple with '' if need be.

    >>> list(character_grouper('This is a test', 3))
    [('T', 'h', 'i'), ('s', ' ', 'i'), ('s', ' ', 'a'), (' ', 't', 'e'), ('s', 't', '')]
    """
    args = [iter(iterable)] * n
    return itertools.izip_longest(*args, fillvalue='')

Proposed improvements

from binascii import hexlify, unhexlify
from itertools import izip_longest
import Tkinter as tk

import tkFileDialog as tk_file_dialog


DEFAULT_FILE_TYPES = (
        ("Hexadecimal Files",   "*.hex"),
        ("Windows Executables", "*.exe"),
        ("Linux Binaries",      "*.elf"),
        ("all files",           "*.*")
)


def character_grouper(iterable, n):
    """Group consecutive n values of iterable into tuples.

    Pad the last tuple with '' if need be.

    >>> list(character_grouper('This is a test', 3))
    [('T', 'h', 'i'), ('s', ' ', 'i'), ('s', ' ', 'a'), (' ', 't', 'e'), ('s', 't', '')]
    """
    args = [iter(iterable)] * n
    return izip_longest(*args, fillvalue='')


class Window():
    def __init__(self, width=47, height=20):
        """Create an editor window.

        Editor will allow you to select a file to inspect and
        modify its content as hexadecimal values.
        """
        self.root = tk.Tk()
        self.width = width
        self.height = height
        self.filename = ""
        self.raw_data = ""
        self.lines = []
        self.line_number = 0
        self.create_widgets()

    def run(self):
        """Start the Tkinter main loop on this window and wait for its destruction"""
        self.root.mainloop()

    def create_widgets(self):
        self.menu = tk.Menu(self.root)

        self.filemenu = tk.Menu(self.menu, tearoff=0)
        self.filemenu.add_command(label="Save", command=self.save_file, accelerator="Ctrl-s")
        self.filemenu.add_command(label="Save as...", command=self.saveas_window, accelerator="Ctrl-S")
        self.filemenu.add_command(label="Open...", command=self.open_window, accelerator="Ctrl-o")
        self.filemenu.add_separator()
        self.filemenu.add_command(label="Quit", command=self.save_and_close, accelerator="Ctrl-q")
        self.filemenu.add_command(label="Quit without saving", command=self.root.destroy, accelerator="Ctrl-Q")

        self.menu.add_cascade(label="File", menu=self.filemenu)

        self.main_text = tk.Text(self.root, width=self.width, height=self.height)

        self.main_text.pack(fill="both", expand=1)
        self.main_text.insert("1.0", self.format_current_buffer())

        self.root.config(menu=self.menu)
        self.root.bind("<Down>", self.scroll)
        self.root.bind("<Up>", self.scroll)
        self.root.bind("<Control-s>", self.save_file)
        self.root.bind("<Control-o>", self.open_window)
        self.root.bind("<Control-S>", self.saveas_window)
        self.root.bind("<Control-q>", self.save_and_close)
        self.root.bind("<Control-Q>", self.close)
        self.root.bind("<Configure>", self.resize)
        self.root.protocol('WM_DELETE_WINDOW', self.save_and_close)

    def resize(self, event=None):
        """Update the amount of characters on each row when the window is resized"""
        self.width = self.main_text.winfo_width() / 8
        self.height = self.main_text.winfo_height() / 16
        if self.width / 3 != 0:
            self._preprocess_raw_data()

    def open_file(self, filename):
        """Open a file and display the content"""
        self.filename = filename
        with open(filename, "rb") as f:
            self.raw_data = chr(0) + f.read()
        self.line_number = 0
        self._preprocess_raw_data()

    def _preprocess_raw_data(self):
        """Convert the content of a file to a list of lines
        suitable for the current width.
        """
        data = hexlify(self.raw_data)[2:]
        chars = self.width - (self.width / 3)
        self.lines = [
                "".join(line)
                for line in character_grouper(data, chars)
        ]
        self.main_text.delete("1.0", "end")
        self.main_text.insert("1.0", self.format_current_buffer())

    def save_file(self, event=None):
        """Save the current modifications into the current file"""
        self.update_current_buffer()
        with open(self.filename, "wb") as f:
            f.write(unhexlify("".join(self.lines)))

    def save_and_close(self, event=None):
        self.save_file()
        self.close()

    def close(self, event=None):
        self.root.destroy()

    def saveas_window(self, event=None):
        """Open the 'save as' popup"""
        f = tk_file_dialog.asksaveasfilename(filetypes=DEFAULT_FILE_TYPES)
        if f:
            self.filename = f
            self.save_file()

    def open_window(self, event=None):
        """Open the 'open' popup"""
        f = tk_file_dialog.askopenfilename(filetypes=DEFAULT_FILE_TYPES)
        if f:
            self.open_file(f)

    def format_current_buffer(self):
        """Create the text to display in the main text area.

        Each line of the current view window ("height" lines from current
        line) is formatted by inserting a space every two characters.
        """
        content = self.lines[self.line_number:self.line_number + self.height]
        return "\n".join(" ".join(map("".join, character_grouper(line, 2))) for line in content)

    def update_current_buffer(self):
        """Save the modification made in the main text area into memory"""
        content = self.main_text.get("1.0", "end").replace(" ", "").split("\n")
        for i, line in enumerate(filter(bool, content)):
            self.lines[i + self.line_number] = line

    def scroll(self, event=None, direction=None):
        """Scroll up or down depending on the current position"""
        cursor_position = self.main_text.index("insert")
        current_line = int(cursor_position.split(".")[0])
        if current_line == self.height + 1:
            line_movement = 1
        elif current_line == 1:
            line_movement = -1
        else:
            return

        if 0 < self.line_number < len(self.lines) - self.height:
            self.update_current_buffer()
            self.line_number += line_movement
            self.main_text.delete("1.0", "end")
            self.main_text.insert("1.0", self.format_current_buffer())
            self.main_text.mark_set("insert", cursor_position)


if __name__ == '__main__':
    Window().run()

Side note

If you are new to Python, then I highly recommend to use Python 3 instead of Python 2 whose support is reaching end of life. You will benefit from the latest modules and features.

\$\endgroup\$
  • \$\begingroup\$ Wow. Thanks for this! Two things I want to say here: The print statement was a mistake on my part: It was a debug and I forgot to remove it. Also I'm considering switching to Python 3 so I have tried to make the code as cross-compatible as possible. EDIT: There may be a slight bug, I have edited my question to include it. \$\endgroup\$ – Jachdich Sep 7 '18 at 13:53
  • 1
    \$\begingroup\$ @Jachdich There was an issue with what used to be neatify. This should be fixed now. \$\endgroup\$ – Mathias Ettinger Sep 8 '18 at 18:46

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.