4
\$\begingroup\$

Below is a code which creates a random colour and shows it to the user in a 64 x 64 square. The user must then use the 3 sliders (R, G, B) to match the colour as closely as possible

from tkinter import *
import random

main = Tk()

photo = PhotoImage(width = 64, height = 64)
photo_2 = PhotoImage(width = 64, height = 64)


r_answer = random.randrange(256)
g_answer = random.randrange(256)
b_answer = random.randrange(256)



def pixel(image, pos, colour):
    r,g,b = colour
    x,y = pos
    for i in range(32):
        for j in range(32):
            image.put("#%02x%02x%02x" % (r,g,b),(y+i,x+j))


def show_values(self):


    r = w1.get()
    g = w2.get()
    b = w3.get()


    pixel(photo, (0,0), (r, g, b))


    label = Label(main, image = photo)
    label.grid(row = 0, column = 1)





w1 = Scale(main, from_=0, to=255, command=show_values)
w1.grid(row = 0, column = 0)
w2 = Scale(main, from_=0, to=255, command=show_values)
w2.grid(row = 1, column = 0)
w3 = Scale(main, from_=0, to=255, command=show_values)
w3.grid(row = 2, column = 0)



pixel(photo_2, (0, 0), (r_answer, g_answer, b_answer))

label_2 = Label(main, image = photo_2)
label_2.grid(row = 1, column = 1)


main.mainloop()
\$\endgroup\$
2
\$\begingroup\$

Here are the improvements to your program that I would suggest:

Performance issue

Whenever the user of your program presses one of the colors sliders, show_values() function is invoked and it create a new tk.Label() instance in addition to the formerly existing ones. This means your program consumes unnecessary memory that can be harmful when the user scrolls down and up, several times, the color sliders. This is clearly a performance issue that can incapacitate your program.

Solution to the performance issue

To resolve the performance issue, you can create a tk.Frame() instance to be the parent widget of the tk.Label() instances you create. This will allow you to clear the tk.Frame() parent widget before a new tk.Label() widget is created.

In my solution below, the color surface which results from the user's actions is wrapped by a tk.Frame() instance:

def create_estimated_color_area(self):
    self.frame = tk.Frame(self.master)
    self.frame.grid(row=0, column=1)

The callback function inherent to the tk.Scale() widgets clears the self.frame widget above:

def estimate_color(self, event):  
    # Clear the frame before drawing a new label on it  
    self.clear_widget(self.frame)   
    # ...
    self.calculated_color_image = tk.PhotoImage(width=64, height=64)
    self.colorize_area(self.calculated_color_image, (r, g, b))
    self.calculated_color_area = tk.Label(self.frame, image=self.calculated_color_image)
    self.calculated_color_area.pack()

The clear_widget() function loops over the self.frame widget to look for its children and removes them away totally:

def clear_widget(self, widget):
    for child in widget.winfo_children():
        child.destroy()

Scalability issues

Your program, as designed, is not scalable. This is because you designed it using a mixture of sequential and functional approaches. But you are dealing with a GUI, and user graphical interfaces were the factor that soared the thrive of object-oriented design. Whether there are some philosophical dirty discussions about it, you should adopt an OOP approach to design your GUI: at least this is valid when it comes to Python and Tkinter. This will remedy to the scalability problem your program is suffering from.

Given this fact, my solution below consists in redesigning your program in an OOP approach. Of course, I end up by more code than you provided by now. But the difference is that my solution is open for adding any other future functionalities in the future, if you want to improve and scale your program. It is also a more disciplined and cleaner code. To understand my design below, you can refer to Tkinter best practices article.

Confusing names

I do not want to be rude, but honestly most of the variable and function names you used are confusing. This is the case, for example, of the pixel() function which does not redefine the notion of a pixel or anything like that. You rather used it to colorize a square area. In addition to this, as in all other programming languages, your function name should be a verb, not a noun. Why? Because a function does something. A function is a doer.

I could write a long text about the naming conventions regarding almost all your variables and functions names: no one, for instance, could find it easy to guess and understand without an additional effort, what are these variables supposed to do: w1, w2, and w3. I renamed everything accordingly in my solution (not to perfection, but to better, and surely you can choose better names than I provided below)

pixel() with one parameter less

The pos parameter in pixel() function is useless because, you always called it with the same starting position (0, 0). So simply get rid of it. Advantage? Less parameters in a function leads to simpler unit testing and cleaner code since parameters belong to a level of abstraction different than that of functions.

Solution

Taking in consideration the former remarks, but also other ones I did not mention such as the right way to import Tkinter, below are the new clothes to your program.

Note that the execution of the below program outputs the same result as yours:

enter image description here

As I said, I have more code below, but I provide you clan code which is scalable: which thing means, as your program becomes larger (in the future) you will write less code and in an easier way. Please, feel free if there is something you do not understand:

import tkinter as tk
import random    


class ColorFinder(tk.Frame):

   def __init__(self, master):
       self.master = master
       tk.Frame.__init__(self, self.master)
       self.configure_gui()
       self.create_widgets()

   def configure_gui(self):
       self.master.title('Coulor Finder')

   def create_widgets(self):
       self.create_color_sliders()
       self.create_color_areas()

   def create_color_sliders(self):
       self.red_color_slider = tk.Scale(self.master, from_=0, to=255)
       self.green_color_slider = tk.Scale(self.master, from_=0, to=255)
       self.blue_color_slider = tk.Scale(self.master, from_=0, to=255)
       self.position_color_sliders()
       self.configure_color_sliders()

   def position_color_sliders(self):
       self.red_color_slider.grid(row=0, column=0)
       self.green_color_slider.grid(row=1, column=0)
       self.blue_color_slider.grid(row=2, column=0)

   def configure_color_sliders(self):       
       self.red_color_slider.configure(command=self.estimate_color)
       self.green_color_slider.configure(command=self.estimate_color)
       self.blue_color_slider.configure(command=self.estimate_color)

   def create_color_areas(self):
       self.create_target_color_area()
       self.create_estimated_color_area()

   def create_target_color_area(self):
       r = random.randrange(256)
       g = random.randrange(256)
       b = random.randrange(256)
       self.target_color_image = tk.PhotoImage(width=64, height=64)
       self.colorize_area(self.target_color_image, (r, g, b))
       self.target_color_area = tk.Label(self.master, image=self.target_color_image)
       self.target_color_area.grid(row=1, column=1)

   def create_estimated_color_area(self):
       self.frame = tk.Frame(self.master)
       self.frame.grid(row=0, column=1)

   def estimate_color(self, event):    
       self.clear_widget(self.frame)   
       r = self.red_color_slider.get()
       g = self.green_color_slider.get()
       b = self.blue_color_slider.get()
       self.calculated_color_image = tk.PhotoImage(width=64, height=64)
       self.colorize_area(self.calculated_color_image, (r, g, b))
       self.calculated_color_area = tk.Label(self.frame, image=self.calculated_color_image)
       self.calculated_color_area.pack()

   def colorize_area(self, image, color):
       x = 0
       y = 0
       r, g, b = color
       for i in range(32):
           for j in range(32):
               image.put("#%02x%02x%02x"%(r , g, b), (y+i, x+j))

   def clear_widget(self, widget):
       for child in widget.winfo_children():
           child.destroy()



if __name__ == '__main__':
   root = tk.Tk()
   ColorFinder(root)
   root.mainloop()
\$\endgroup\$

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.