A GUI Youtube Player (2)

Question

Link to newer version: A GUI Multimedia Player with acces to the Youtube Database (3)

Image of UI

I am working on a GUI Youtube Player. When I had a first working version, I put it here on CodeReview, getting many helpful comments, and I made now a kind of version 1.0, with many improvements, in code structure, functionality and design.

Here is a link to the original post.

And link to the Github page I made: https://github.com/floris-vos/gui-youtube-player

And here is the new code:

from tkinter import (
 Tk, Menu, Frame, Label, Button, Scale, BOTH, Entry, DISABLED, END,NORMAL,StringVar, HORIZONTAL, VERTICAL, Text
)
from youtube_search import YoutubeSearch
import re,random, vlc,datetime, time,yt_dlp, csv, os, sys, json
from csv import writer
win = Tk()
win.geometry("500x150")
def quit():
 win.after(500,win.destroy)
win.protocol('WM_DELETE_WINDOW', quit)
YOUTUBE_BASE_URL = 'https://www.youtube.com'
NON_CHARS = r'[^a-zA-Z0-9 .,:;+-=!?/()öäßü]'
shuffle=0
prefered_quality=2
media_qualities = [
 'ultralow',
 'low',
 'medium',
 '144p',
 '240p',
 '360p',
 '480p',
 '720p',
 '1080p',
 '1440p',
 '2160p'
]
query_index = 0; timescale_var =0
play_index=-1
volume_var= 100
query_list = []; playlist = []
win.title("Youtube Player")
instance = vlc.Instance('--no-xlib -q > /dev/null 2>&1')
player = instance.media_player_new() 
#sending the entry variable for a youtube search
def query_songs(event):
 playlist_frame.focus_set()
 global query_index, query_list
 query_list = song_search(str(search_box.get()))
 query_index = 0
 update_query_title()
#making the search
def song_search(search_querie):
 results = YoutubeSearch(search_querie, max_results=40).to_dict()
 query_list=[]
 for result in results: 
 query_list.append({
 'url': ''.join([YOUTUBE_BASE_URL, result['url_suffix']]),
 'title': re.sub(NON_CHARS, '', result['title'])
 })
 return(query_list)
#moving through the songlist
def query_prev():
 if query_list:
 global query_index
 query_index-=1
 if query_index < 0:
 query_index =len(query_list) - 1
 update_query_title()
#moving through the songlist
def query_next():
 if query_list:
 global query_index
 query_index += 1
 if query_index == len(query_list):
 query_index = 0
 update_query_title()
#update the title of the query list
def update_query_title():
 song_query.config(state="normal")
 song_query.delete(0, END)
 song_query.insert(0, query_list[query_index]['title'])
 song_query.config(state="disable")
 return
#updating the title of the currently playing song
def update_currently_playing(TITLE):
 currently_playing.config(state="normal")
 currently_playing.delete(0, END)
 currently_playing.insert(0, TITLE)
 currently_playing.config(state="disable")
 return
#update the song description
def update_description_label(description):
 description_label.config(state="normal")
 description_label.delete(0, END)
 description_label.insert(0, str(description))
 description_label.config(state="disable")
 return
#update the title of the playlist
def update_playlist_title():
 playlist_item.config(state="normal")
 playlist_item.delete(0, END)
 try:
 playlist_item.insert(0, playlist[play_index+1]['title'])
 except: 
 playlist_item.insert(0, "")
 playlist_item.config(state="disable")
 return
#this function keeps track of time and moves the timescale
def update_time():
 length = timescale['to']
 place = player.get_time()
 if (length - place) < 10000 and player.is_playing() == 0:
 if shuffle == 1 and len(playlist) > 1:
 random()
 start_new_song(playlist[play_index]['url'])
 elif play_index < (len(playlist) -1): 
 playlist_go()
 elif player.is_playing() == 1:
 time_info =str(datetime.timedelta(seconds = round(place/1000))) + " / " + str(datetime.timedelta(seconds = round(length/1000)))
 time_label.config(text=time_info)
 timescale.set(place)
 win.after(1000,update_time)
#to delete the old items in the song quality menu
def delete_quality_menu():
 for i in media_qualities:
 try:
 current_quality_menu.delete(i)
 except:
 continue
 return
#this is to generate a list of stream URLs from a youtube URL
def generate_stream_url(URL):
 audio = []
 ydl_opts = {"quiet":True }
 with yt_dlp.YoutubeDL(ydl_opts) as ydl:
 info = ydl.extract_info(URL, download=False)
 formats = info['formats']
 songduration = 1000 * info['duration']
 songtitle = info['title']
 description = "".join([s for s in info['description'].strip().splitlines(True) if s.strip()])
 url_list = []
 counter = -1
 for format in info['formats']:
 video_format = format['format']
 video_format_short = video_format[video_format.find("(")+1:video_format.find(")")] 
 if (video_format[2]==" " or "audio only" in video_format) and not("DASH" in video_format) and not(counter > -1 and video_format_short == url_list[counter]['video_format']):
 url_list.append({
 'stream_url':format['url'],
 'video_format':video_format_short
 })
 if "audio" in video_format:
 quality_label = f"audio only ({video_format_short})"
 else:
 quality_label = f"video ({video_format_short})"
 current_quality_menu.add_command(label=quality_label,command=(lambda url = format['url'], title=video_format_short: set_player(url,title)))
 if counter>-1 and "audio" in video_format and not("audio" in url_list[counter]):
 prefered_quality_menu.entryconfigure(1, command=(lambda url = format['url']: set_player(url)))
 
 counter +=1
 
 prefered_quality_menu.entryconfigure(0, command=(lambda url = url_list[len(url_list)-1]['stream_url']: set_player(url)))
 prefered_quality_menu.entryconfigure(2, command=(lambda url = url_list[0]['stream_url']: set_player(url)))
 return(url_list, songduration, description, songtitle)
#getting the stream with the desired quality
def select_stream(audio):
 break_out_flag = False
 for index in range(prefered_quality):
 if break_out_flag:
 break
 for item in audio:
 if item['video_format'] == media_qualities[prefered_quality-index]:
 url = item['stream_url']
 break_out_flag = True
 break
 return(url)
#putting the media to the player
def set_player(playurl,quality_wish=""):
 global prefered_quality
 if quality_wish!="":
 prefered_quality= media_qualities.index(quality_wish)
 time = player.get_time()
 media=instance.media_new(playurl)
 media.get_mrl()
 player.set_media(media)
 player.play()
 player.set_time(time)
 
#setting up the new song
def start_new_song(URL):
 if query_list or playlist:
 delete_quality_menu()
 audio, songduration, description, songtitle = generate_stream_url(URL)
 playurl = select_stream(audio)
 set_player(playurl) 
 pause_button.config(text="||")
 timescale.config(to = songduration)
 timescale.set(0)
 player.set_time(0)
 win.title(songtitle)
 update_description_label(description)
 update_currently_playing(songtitle)
 update_playlist_title()
 return
#this is to select the next song in the list
def add_song():
 if query_list:
 playlist.append(query_list[query_index])
 update_playlist_title()
#this function is for downloading the song
def download():
 song_url = query_list[query_index]['url']
 song_title = query_list[query_index]['title']
 outtmpl = song_title + '.%(ext)s'
 ydl_opts = {
 'format': 'bestaudio/best',
 'outtmpl': outtmpl,
 'postprocessors': [
 {'key': 'FFmpegExtractAudio','preferredcodec': 'mp3',
 'preferredquality': '192',
 },
 {'key': 'FFmpegMetadata'},
 ],
 }
 with yt_dlp.YoutubeDL(ydl_opts) as ydl:
 info_dict = ydl.extract_info(YOUTUBE_BASE_URL+ song_url, download=True) 
#this is to select a random song from the playlist
def random():
 global play_index
 counttemp = play_index
 while counttemp == play_index:
 play_index = random.randrange(len(playlist)-1)
 return
#moving through the scale for time
def timescale_move(timescale_var):
 place = player.get_time()
 if abs(int(timescale_var) - place) > 4000:
 player.set_time(int(timescale_var))
#to pause by keypress (space)
def toggle_pause1(event):
 if str(win.focus_get()) != str(".!entry"):
 toggle_pause2()
 
#to pause by keypress or click
def toggle_pause2():
 pause = player.is_playing()
 player.set_pause(pause)
 pause_button.config(text=["||","▶"][int(pause)])
#import all songs from querie
def all_songs_to_playlist():
 if query_list:
 playlist.extend(query_list)
 update_playlist_title()
#controlling the volume
def change_volume(volume_var):
 player.audio_set_volume(int(volume_var))
#clear playlist
def clear_playlist():
 global playlist
 play_index = 0
 playlist = []
 update_playlist_title()
 return
def save_playlist():
 name = search_box.get()
 if len(playlist) >1 and name != "":
 newrow = [name + "§", playlist]
 with open(os.path.join(sys.path[0], "AntiTubePlaylists.txt"), "a") as f_object:
 writer_object = writer(f_object)
 writer_object.writerow(newrow)
 f_object.close()
 playlist_list_menu.add_command(label=name, command=(lambda playlistt = playlist: import_playlist(playlistt)))
#for browsing through the playlist
def playlist_next():
 if playlist:
 global play_index
 if play_index < (len(playlist)-2):
 play_index+=1
 update_playlist_title()
#for browsing through the playlist
def playlist_prev():
 if playlist:
 global play_index
 if play_index >-1:
 play_index-=1
 update_playlist_title()
 return
#for removing item from playlist
def playlist_remove():
 if playlist:
 del playlist[play_index+1]
 playlist_prev()
 update_playlist_title()
#for selecting song from playlist
def playlist_go():
 if playlist:
 global play_index
 if play_index < (len(playlist)-1):
 play_index+=1
 start_new_song(playlist[play_index]['url'])
#for importing a playlist
def import_playlist(playlist_import):
 global playlist, play_index
 playlist = playlist_import
 play_index = 0
 start_new_song(playlist[play_index]['url'])
#turning shuffle on and off
def toggle_shuffle():
 global shuffle
 shuffle = not(shuffle)
 on_or_off = ["on","off"][int(shuffle)]
 playlist_menu.entryconfigure(0, label=f"turn {on_or_off} shuffle")
#selecting prefered quality as best video
def set_prefered_quality_best_vid():
 global prefered_quality
 prefered_quality=10
 if player.get_time() > 1000:
 select_stream
#selecting prefered quality as best audio
def set_prefered_quality_best_aud():
 global prefered_quality
 prefered_quality=2
#selecting prefered quality as least audio
def set_prefered_quality_least_aud():
 global prefered_quality
 prefered_quality=0
main_frame = Frame(win, width = 320, height=600)
main_frame.grid(row=1, column=1, padx=0, sticky='new')
search_frame=Frame(main_frame, width = 150)
search_frame.grid(row=1, column=1, padx=0, sticky='new',rowspan=3)
playlist_frame=Frame(main_frame, width = 150)
playlist_frame.grid(row=1, column=2, padx=0, sticky='new',rowspan=3)
time_frame=Frame(main_frame,width=320)
time_frame.grid(row=4, column=1, padx=0, sticky='new',columnspan=2)
description_frame=Frame(main_frame,width=320, height = 100)
description_frame.grid(row=5, column=1, padx=0, sticky='nesw',columnspan=3, rowspan=4)
playlist_frame.bind('<space>',toggle_pause1)
search_frame.bind('<space>',toggle_pause1)
search_frame.bind("<Button-1>", lambda event: playlist_frame.focus_set())
playlist_frame.bind("<Button-1>", lambda event: playlist_frame.focus_set())
time_frame.bind("<Button-1>", lambda event: playlist_frame.focus_set())
search_box = Entry(search_frame)
search_box.pack(side='top',anchor = "w")
search_box.bind('<Return>', query_songs)
song_query = Entry(search_frame,state = DISABLED)
song_query.pack(side='top',anchor = "w")
currently_playing = Entry(playlist_frame,state = DISABLED)
currently_playing.pack(side='top',anchor = "w")
playlist_item = Entry(playlist_frame,state = DISABLED)
playlist_item.pack(side='top',anchor = "w")
volume_scale = Scale(main_frame, from_=200, to=0, orient=VERTICAL, variable = volume_var, showvalue=0, command = change_volume)
volume_scale.set(100)
volume_scale.grid(row=1, column=3, padx=0, sticky='nsew',rowspan=4)
pause_button= Button(time_frame, text = "||", command =toggle_pause2)
pause_button.pack(side='left')
button_dict = [
 Button(playlist_frame, text = "←",command = playlist_prev),
 Button(playlist_frame, text = "→",command = playlist_next),
 Button(playlist_frame, text = "X",command = playlist_remove),
 Button(playlist_frame, text = "OK",command = playlist_go),
 Button(search_frame, text = "←",command = query_prev),
 Button(search_frame, text = "→",command = query_next),
 Button(search_frame, text = "OK", command =(lambda: start_new_song(query_list[query_index]['url']))),
 Button(search_frame, text = "+", command = add_song),
 Button(search_frame, text = "↓", command =download)
 ]
for button in button_dict:
 button.pack(side='left')
 button.bind("<Button-1>", lambda event: playlist_frame.focus_set())
time_label = Label(time_frame, text = "0:00:00 / 0:00:00")
timescale = Scale(time_frame, from_=0, to=1000, orient=HORIZONTAL, length=267,variable = timescale_var, showvalue=0, command = timescale_move)
description_label = Entry(description_frame, state = DISABLED)
description_label.pack(side='top',anchor = "w",fill=BOTH)
time_label.pack(side='left')
timescale.pack(side='left')
menubar = Menu(win)
playlist_menu = Menu(menubar, tearoff=0)
playlist_list_menu = Menu(menubar, tearoff=0)
playlist_menu.add_command(label="shuffle on",command=toggle_shuffle)
playlist_menu.add_command(label="all results to playlist", command=all_songs_to_playlist)
playlist_menu.add_command(label="clear playlist", command=clear_playlist)
playlist_menu.add_command(label="save playlist", command=save_playlist)
quality_menu = Menu(menubar, tearoff=0)
current_quality_menu = Menu(menubar, tearoff=0)
prefered_quality_menu = Menu(menubar, tearoff=0)
prefered_quality_menu.add_command(label="best video format", command=set_prefered_quality_best_vid)
prefered_quality_menu.add_command(label="best audio format", command=set_prefered_quality_best_aud)
prefered_quality_menu.add_command(label="least audio format", command=set_prefered_quality_least_aud)
try:
 with open(os.path.join(sys.path[0], "AntiTubePlaylists.txt"), "r") as playlist_file:
 csv_reader = csv.reader(playlist_file, delimiter='§')
 for row in csv_reader:
 try:
 s = row[1][2:][:-1].replace("'",'"')
 playlist_import = json.loads(s)
 playlist_list_menu.add_command(label=row[0], command=(lambda playlist_import = playlist_import: import_playlist(playlist_import)))
 except:
 continue
except:
 pass
menubar.add_cascade(label="Playlists", menu=playlist_menu)
menubar.add_cascade(label="Quality", menu=quality_menu)
quality_menu.add_cascade(label="Set prefered quality", menu=prefered_quality_menu)
quality_menu.add_cascade(label="Set current song quality", menu=current_quality_menu)
playlist_menu.add_cascade(label="Playlists", menu=playlist_list_menu)
win.config(menu=menubar)
search_box.focus()
if __name__ == '__main__':
 update_time()
win.mainloop()

And Requirements:

python_vlc==3.0.7110
youtube_search==2.1.2
yt_dlp==2022年11月11日

Possible helpful comments could concern my way of making the gui with Grid and pack, but also generally readability of the code, or more efficient ways to do certain things, etc. I already know that classes, and object oriented programming might improve the code, and in the previous question, there is already a refactored version in object oriented programming style. I am however as of yet not able to program like that.

One bug I found!! Is in the quality menu deleting function, should be:

def delete_quality_menu():
 for i in media_qualities:
 try:
 current_quality_menu.delete(f"audio only ({i})")
 except:
 try:
 current_quality_menu.delete(f"video ({i})")
 except:
 continue
 return

Answer 1

Thanks for bravely offering up your code. I'm sure it will emerge in better shape.

---

Each development project has its own standards. If they aren't written down in the ReadMe, then we tend to assume community standards. In the case of python source, that would include PEP-8, which suggests that flake8 would probably lint clean.

It doesn't. But fear not! Here is a pair of purely mechanical transformations I have successfully used in more than a hundred development projects:

• isort .
• black -S (a quoting option, short for: --skip-string-normalization)

I am referring to such transformed code when I review your source. It normalizes whitespace and, for example, elides redundant trailing ; semicolons and similar distractions.

Consider adopting similar practices. I find it convenient to throw them in a Makefile and then type $ make lint every so often. Github Actions can automate some of the administrivia.

---

Ground rules

I look for two things in code I review:

1. is it correct? (in the sense of obviously correct)
2. is it maintainable?

Every summer we inevitably hire a few undergrad interns, and ask them to maintain code, meaning add features / fix bugs. Sometimes the author is no longer available. If they have need of chatting with the author, then the code was "not maintainable".

---

Ok, on to the main event. I am reading this:

NON_CHARS = r'[^a-z...]'

Consider renaming to INVALID_CHARS. Because those are, you know, literally "characters". Also, negated identifiers tend to be a bit more tricky for the reader to wrap his head around, so they make me wary, I try to avoid them.

As a separate matter, this is a constant, and there's no need to compile to regex more than once. Might as well do it here.

NON_CHARS = re.compile(r'[^a-z...]')

Also, thank you for the r-string. Habitually using raw strings for regexes makes lots of sense. Even if it doesn't affect the current string, you never know when some future maintainer will add a special character to the mix.

---

This is an interesting expression:

 ... vlc.Instance('--no-xlib -q > /dev/null 2>&1')

Apparently a shell handles that. The 2>&1 is lovely. Consider sending output to /tmp/youtube.log or similar, perhaps after | egrep -v "boring|redundant" noise lines have been removed.

---

# sending the entry variable for a youtube search

You went to the trouble of writing this comment, so I guess it is helpful, valuable, and should stay. Please promote it to a """docstring""".

Similarly for other such comments.

(And no, I do not feel that every function needs a docstring. Refrain from adding one if the name + signature already makes things perfectly clear.)

---

def query_songs(event):
 playlist_frame.focus_set()
 global query_index, query_list

In software engineering, "coupling" is a concern. You kind of have a ton of variable all thrown together in the module namespace. Maybe for *_frame it makes sense. The global keyword seems like a bad code smell. It suggests that things might hold together a bit better if we put this in a class and used self.query_foo to access those variables.

If you were using classes, you might find that there's more than one that would make sense. That is, the goal would be to have less than a dozen instance variables per class, rather than the profusion of globals we see now.

---

def song_search(search_querie):

Consider telling us about the return value, with an optional type hint.

nit: typo

def song_search(search_query: str) -> list:

nit: Consider returning a list comprehension of dicts. Whatever. The current code is perfectly clear, just slightly verbose, as it names a temp var that is returned. I feel it is mis-named, as it contains query results rather than queries, so making it anonymous would make that detail go away. There is unfortunate name collision between the local and module query_list identifiers.

---

DRY. Consider extracting a tiny helper for query_{prev,next}, which accepts a +/- 1 increment. You could use % modulo here.

That query_list data structure is calling out to you, it wants to be a separate object in its own class.

---

def update_currently_playing(TITLE):

Please don't do that, it's very confusing. It's not a MANIFEST_CONSTANT. Try to conform to PEP-8.

def update_currently_playing(title):

The various update_foo() functions are crying out: "extract helper!"

I confess that, as a potential future maintainer, I do not exactly understand what .delete(0, END) is all about. I think the goal is to trim to zero length and then replace with what we're inserting? But I'm not sure, and there's no comment spelling it out.

If we were to add a helpful comment, where would add it? First occurrence? All of them? That's why "extract helper" is so important! If the code happened just once, then we'd know exactly where to write the docstring or comment.

---

All the update_foo functions end with a vacuous return. Please don't do that. We evaluate the function for side effects, it has no return value that callers care about. (Python defaults to return None when you fall off the end of a function, same as the return that you wrote.) We write return when we're exiting early, perhaps in an if or a while, but not in this situation.

---

def update_playlist_title():
 ...
 except:

No.

Just don't. You're not actually trying to trap CTRL/C there. Rather than all exception types, catch only the ones you intended:

def update_playlist_title():
 ...
 except Exception:

For extra credit, spell out the one or two specific error types which commonly cause trouble here if they go uncaught.

I am concerned that it is IndexError due to the playlist[play_index + 1] dereference. And we're not really doing anything to improve matters, we're just papering over the issue, nothing to see here folks, keep moving on. If the list + index were in an instance of a class, we'd be in a better position to write down a class invariant that we're trying to maintain.

---

def update_time():
 if ... player.is_playing() == 0:

zomg, the boolean function is_playing returns an int?!?

I was hoping we could just say

 if ... player.is_playing():

The is_ prefix is lovely. It strongly suggests that a boolean will come back, or at least that we can safely examine truthiness. The comparison to 0 is slightly jarring, looks a bit like C.

---

def delete_quality_menu():
 for i in media_qualities:
 try:
 current_quality_menu.delete(i)
 except:
 continue
 return

No.

We've discussed bare except and vacuous return already. Let's focus on that continue.

It means "early exit from current iteration of for loop. What you intended there was pass. (Yes, I know, the effect is the same. But there's no need to plant landmines for future maintainers who might add code after the except clause.)

---

def generate_stream_url(URL):

Fix the signature, please, lowercase url. Consider adding optional type hinting on the input argument and the return value.

Consider adopting these spellings: song_duration & song_title.

I am reading this expression:

 if (
 (video_format[2] == " " or "audio only" in video_format)
 and not ("DASH" in video_format)
 and not (
 counter > -1
 and video_format_short == url_list[counter]['video_format']
 )
 ):

It is, ummm, a trifle on the looonnggg side. Consider breaking it out into a helper predicate, which has the virtue of being amenable to some trivial unit test. Definitely re-phrase and not ... to be and ("DASH" not in video_format). The "not in" advice is a PEP-8 thing, which flake8 would have been happy to tell you about.

I am reading this expression:

 current_quality_menu.add_command(
 label=quality_label,
 command=(
 lambda url=format['url'], title=video_format_short: set_player(
 url, title
 )
 ),
 )

There's a time and a place for anonymous lambda expressions, I suppose. Consider giving this one a name, with a def. The kwarg defaults are needlessly confusing. It's not at all clear to me how many args (0?, 1?, 2?) the caller will routinely supply. That's the sort of concern that a """docstring""" can immediately clear up, except the way you've phrased it there's no convenient place to put such documentation.

 if (
 counter > -1
 and "audio" in video_format
 and not ("audio" in url_list[counter])
 ):
 prefered_quality_menu.entryconfigure(
 1, command=(lambda url=format['url']: set_player(url))
 )

Oh, wait, now I think I'm starting to see it. Your intent is to curry the format['url'] dereference. Hmmm. It's an unfamiliar idiom you're using to bind the current value to the future call, but I suppose one could grow accustomed to it. Consider preferring the standard library's approach instead. Consider computing a temp var url and passing in just that simple url reference.

---

def select_stream(audio):
 break_out_flag = False

Conventional name for this is simply done.

 if item['video_format'] == media_qualities[prefered_quality - index]:
 url = item['stream_url']
 break_out_flag = True
 break

Yikes!

No need for a flag. Just return the url already.

Ok, this function is too crazy, you really need to rewrite it. What it does is so simple, yet the implementation is so complex. I can't even explain why we need nested for loops to implement it.

Could we maybe use a sorted list to prioritize our favorite stream? Or use index() and compare that it is within 2?

Write a unit test for this function. Verify it is Green, before & after your refactor.

---

Ok, I'm going to skip ahead a bit, to this one:

def add_song():
 if query_list:
 playlist.append(query_list[query_index])
 update_playlist_title()

That doesn't feel right.

I mean, didn't we just consume a song. And so we should increment query_index?

Seems like yet another place that would benefit from having list + index sequestered in some class which documents how they relate to one another.

---

def toggle_pause2():
 ...
 pause_button.config(text=["||", "▶"][int(pause)])

Kudos, I love it! That's a beautiful dereference expression. Clear and concise.

---

XXX: probable bug!

def set_prefered_quality_best_vid():
 ...
 if player.get_time() > 1000:
 select_stream

That seems like it is Just Wrong. You're computing an expression and discarding it. Definitely you're returning None in all cases. Did you want to call select_stream()? And maybe give it argument?

---

timescale = Scale(
 ...
 showvalue=0,

TkInter was apparently looking for a bool rather than an int there.

---

 with open(os.path.join(sys.path[0], "AntiTubePlaylists.txt"), "r") as playlist_file:

This is deeply problematic.

On your system the zero-th element might just happen to be the package install directory. But that absolutely is not guaranteed. Exporting a revised value of PYTHONPATH, for example, could easily disturb that. Sometimes people append, or prepend, a . dot current-working-directory entry to that env var.

You were looking for __file__, which works in the subset of cases that your code is installed as separate files. If import found your code via a compressed ZIP file, then it won't do what you want.

Use from pathlib import path, and use a parent of Path(__file__) as default directory. Use os.getenv( ... ) as way for users to override that when it's not what they want.

Or google for how to ship resources in a package and then reliably access them.

 for row in csv_reader:
 ...
 s = row[1][2:][:-1].replace("'", '"')

That's not a terrific expression, it's opaque. Later you mention row[0]. Let's clarify that:

 for foo, bar in csv_reader:

Yeah, I know, not great identifiers, IDK what good ones would be. But that's my point. When you use 0 / 1 indices, you are not communicating much meaning to the reader. Take advantage of the opportunity to name things. Maybe "title" is one of the names?

And then we have a chained bar[2:][:-1], which isn't great, especially when phrased as an apparent triply-indexed row[1][2:][:-1]. The mixture of deref / slice / slice doesn't serve the goal of clarity. Pick out bar and then maybe use a regex on it in the next line. Or a single [2:-1] slice. Or replace the [:-1] with an .rstrip() expression. Yes, it "works" as written. No, it's not the best way to communicate a technical idea to a collaborator. I view it as a (minor) maintenance burden. Extracting the expression into a tiny helper would offer an excellent documentation opportunity.

 except:
 continue

No. As discussed above, prefer:

 except Exception:
 pass

or perhaps narrow it to your favorite one or two errors.

---

Here is a generic rule you might use when considering if you should break out a class.

Search across all functions for references to some global variable foo. (Sometimes it's easy to find candidate variables when they're announced by global foo.)

Consider whether you'd like to make the relationships more explicit, by passing in the variable as part of the function signature.

Now count up how many changes would be needed, and look at how bloated the signatures might become. Do they accept more than a few input args? Would you be happier grouping related variables together and just passing in self ?

After reviewing that, now you're in a good position to decide whether certain variables "always go together" and should be passed around as a single object rather than separately.

---

This is quite curious:

if __name__ == '__main__':
 update_time()
win.mainloop()

I think you intended to indent both of those.

But there's already a ton of side effects, such as when assigning win / instance / player, so it's probably moot.

Usually we want to see only class / def definitions at top level, plus some variable assignments that are free of side effects. Why? So that other modules, perhaps a unit test, can silently import the source file.

Six hundred lines is getting to be long enough that you could sensibly banish a few things to this or that new module. That's independent of whether you define a new class or not.

You have a fair number of variables defined, with many of them being widgets. It would be useful to carve out some rectangular portion of the UI and put all contained widgets into another module.

In general, the more variables are in scope for a module / class / method / function, the harder the maintenance task will be for someone down the road. So if several variables cohere together, put them in a new scope. Yes, there's a cost to doing that, but it's a way of paying down technical debt.

---

Overall?

Yes, I had many remarks. It seems that I always do.

But the code follows a consistent pattern and seems fairly well written, fairly easy to understand.

There are some minor stylistic details to attend to. My chief concerns relate to coupling, and to documenting the invariants we're trying to maintain among several variables.

• Willem van Houten: Thank you very much! Already a lot of clear points where I can improve and am improving the code. (for instance: I didn't know that functions automatically have Return None at the end!) Also a few things I don't understand yet, I might ask some questions later if I dont figure everything out by myself.
• Willem van Houten: One more point! Thank you for another thing; I got the point of cutting out a rectangle and making it into a class. Also with thanks to @BrunoVermeulen, I used some of his code to see how making classes is done. So I just made for the first time a class and it works, and got rid of 2 global vars!. Probably gonna make the rest of my code into classes now too.
• Caridorc: The answer given is very well made, detailed and complete. It is exemplary in showing many comments on readability and best practices. As such it surely deserves a bounty as an encouragement for the answerer to share his knowledge further on this site. Thanks a lot for your contribution to the site @J_H (I must wait a day before awarding the bounty)
• Willem van Houten: @J_H I have now pretty much made a (for now) satisfying version of the code (made it into 4 classes). will take a day at least for finding any more bugs. But I have a question! I used flake8, and got 6 pages of comments on some spaces in the wrong place and such. And my question is, is there some automatic way to clean this up? I did not understand your suggestion about isort . and black -S! What do they mean?
• Willem van Houten: I finished my code, but I will not put it here yet. While making a first Object Oriented version, I gradually understood what an object is. And so suddenly I found myself no longer coding, but opening a sketchbook and making diagrams of what I am making, and what kind of objects it requires. And so I realise I have to rearrange my code one more time – probably the last time! One major insight is that dependency should always be one way, i.e. if a class includes an instance of another class, then the secondary class may not refer to the primary class, only the other way around.