[newbie] advice and comment wanted on first tkinter program

[Peter Otten]

Jean Dupont wrote:

> Dear all,
> I made a simple gui with tkinter. I can imagine there are things which I
> did which are "not optimal". So what I ask is to comment on my code
> preferable with snippets of code which show how to do improve my code.
> #!/usr/bin/env python
> import Tkinter
> import time
> import RPi.GPIO as GPIO
> GPIO.setmode(GPIO.BOARD)
> GPIO.setup(26,GPIO.OUT)
> GPIO.setup(24,GPIO.OUT)
> #hardware : connect 2 leds:
> #board-pin 26 on/off led; control with buttons
> #board-pin 24 led with pwm dimming and frequency; control via sliders
> top = Tkinter.Tk()
> top.geometry("600x400+310+290")
> var1 = DoubleVar()
> var2 = DoubleVar()
> i=0
> p=GPIO.PWM(24,1)
> p.start(50)
> def btn_on_cmd():
>         text3.configure(bg = "#00FF00")
>         text3.delete(0.1,END)
>         text3.insert("0.1","ON ")
>         GPIO.output(26,True)
> def btn_off_cmd():
>         text3.configure(bg = "#FF4000")
>         text3.delete(0.1,END)
>         text3.insert("0.1","OFF")
>         GPIO.output(26, False)
> def timer0():
>         global i
>         i=i+1
>         text1.delete(0.1,END)
>         text1.insert(4.2,"Timer: " + str(i))
>         label1.configure(text=time.strftime("%H:%M:%S"))
>         top.after(1000, timer0)
> def Set_PWM(var1):
>         DC = float(var1)
>         p.ChangeDutyCycle(DC)
> def Set_FREQ(var2):
>         FR = float(var2)
>         p.ChangeFrequency(FR)
> btn_on = Button(top, text ="On", command = btn_on_cmd)
> btn_on.place(x=10,y=100)
> btn_off = Button(top, text ="Off", command = btn_off_cmd)
> btn_off.place(x=100,y=100)
> text1 =Text(top, bg = "#009BFF", font=("Helvetica",14), height = 1, width
> = 15)
> text1.place(x=5,y=5)
> text3=Text(top, bg = "red", font=("Helvetica",12),height = 1, width = 4)
> text3.place(x=60,y=60)
> label1 = Label(top,relief=RAISED,bg =
> "#EFF980",font=("Helvetica",14),height = 1, width = 15)
> label1.place(x=5,y=350)
> label2= Label(top,relief=RAISED,bg =
> "#BFBFBF",font=("Helvetica",10),height = 1, text= "Freq (Hz)")
> label2.place(x=420,y=320)
> label3= Label(top,relief=RAISED,bg =
> "#BFBFBF",font=("Helvetica",10),height = 1, text= "DC %")
> label3.place(x=520,y=320)
> slider1 = Scale(top,variable = var1,length = 300,resolution = 1,command  =
> Set_PWM)
> slider1 = Scale(top,variable = var1,length = 300,resolution = 1,command  =
> Set_PWM) slider1.place(x=500,y=5)
> slider1.set(50)
> slider2 = Scale(top,variable = var2,length = 300,from_= 0.1, to =
> 50,resolution = 0.1,command  = Set_FREQ) slider2.place(x=400,y=5)
> slider2.set(2)
> timer0()
> top.mainloop()
> GPIO.cleanup()
> 
> 
> thanks in advance
> jean

First and foremost a program has to do what the author wants it to do. 
Everything else is secondary. You are likely to have such a program on your 
machine, so congrats :)

However, the version you posted does not run, probably because you started 
to replace

from Tkinter import *
top = Tk()
...
var1 = DoubleVar()

with the -- better --

import Tkinter
top = Tkinter.Tk()
...

but stopped too early, so that the line

var1 = DoubleVar

will raise a NameError. The fix is mechanical: run the program, go to the 
line with the NameError and add the 'Tkinter.' prefix.

Once you have done that you should take the time to find good variable 
names. var1? I have no idea what value that could hold until I've read the 
whole program. That's feasible here, but program size may grow over time, 
and can you still tell me what var1 means next week? I recommend names that 
reflect the problem domain, e. g. `var_dutycycle` or just `dutycycle`.

Next you should consider grouping the code by topic -- not necessarily into 
functions; having a section that does the setup for the dutycycle data and 
widgets and one for the time etc., visually separated by one or two empty 
lines should be sufficient.

If you're ambitious you should read up on the grid layout manager. I allows 
widget size to change depending on the window size.

The Text widget can be used to write whole Editors (like IDLE) -- it does no 
harm here, but seems a bit heavyweight for just an On/Off display. I would 
go with a Label or Entry. 

What does a red widget with no text mean, by the way? On, off, or undefined?
Personally, I like to start with a defined state. An easy way to achieve 
this is to call 

button_off_cmd() # or button_on_cmd()

manually before your program enters the mainloop() -- just like you did with 
timer0().

PS: An easy way to get an idea of what a script does is to run it. I'd guess 
that by keeping the Rasperry-Pi-specific code in you are reducing the number 
of readers who can do that by a few orders of magnitude. I managed to get it 
to run with the following ad-hoc changes:

$ diff -u raspberry_orig.py raspberry_mock.py
--- raspberry_orig.py   2014-01-17 16:10:20.843334832 +0100
+++ raspberry_mock.py   2014-01-17 16:10:58.970855503 +0100
@@ -1,7 +1,36 @@
 #!/usr/bin/env python
 import Tkinter
+from Tkinter import *
 import time
-import RPi.GPIO as GPIO
+
+try:
+    import RPi.GPIO as GPIO
+except ImportError:
+    class Name(str):
+        def __repr__(self):
+            return self
+
+    class GPIO:
+        def __init__(self, prefix):
+            self.prefix = prefix
+        def __getattr__(self, name):
+            if name in ["BOARD", "OUT"]:
+                return Name(name)
+            if name == "PWM":
+                return GPIO("PWM")
+            if name == "__call__":
+                def c(*args):
+                    print "Initializing {}{}".format(self.prefix, args)
+                    return self
+                return c
+            def f(*args):
+                print "Calling {}.{}{}".format(self.prefix, name, args)
+            return f
+
+    GPIO = GPIO("GPIO")
+
+
+
 GPIO.setmode(GPIO.BOARD)
 GPIO.setup(26,GPIO.OUT)
 GPIO.setup(24,GPIO.OUT)