Conversation
There was a problem hiding this comment.
Heya @f0rdprefect
I've been asked to review your PR as I'm a Python dev.
Thanks for your contribution. I've added some comments.
Note that this is in no way critic on your work. I realize this has been vibecoded and the editor would work for your purposes. This is mostly a code review before @kefcom merges the PR.
Some changes needed for type safety, installation instructions, etc. See code reviews in diff for details.
| ```bash | ||
| cd hackerjeopardy_editor | ||
| python3 main.py | ||
| ``` |
There was a problem hiding this comment.
Note that tkinter is NOT installed by default in the majority of Linux distro's. For Windows, it's required to install tkinter when installing Python (this is a checkbox in the installer). See error message below (this is on Linux Mint 22.2 Zara)
/home/sgilissen/_venvs/hackerjeopardy_editor/bin/python /home/sgilissen/gitdir/_others/hackerJeopardy/hackerjeopardy_editor/main.py
Traceback (most recent call last):
File "/home/sgilissen/gitdir/_others/hackerJeopardy/hackerjeopardy_editor/main.py", line 1, in <module>
import tkinter as tk
ModuleNotFoundError: No module named 'tkinter'
Suggestion: Amend installation instruction in README.md to include tkinter installation
|
|
||
| # Main layout - 3 panes | ||
| main_frame = ttk.Frame(self.root) | ||
| main_frame.pack(fill=tk.BOTH, expand=True, padx=5, pady=5) |
There was a problem hiding this comment.
Type safety. While using TK stub types technically works, for type safety these should be replaced by their string literal types.
e.g.: instead of
main_frame.pack(fill=tk.BOTH, expand=True, padx=5, pady=5)
consider using the following:
main_frame.pack(fill="both", expand=True, padx=5, pady=5)
More examples:
- tk.BOTH → "both"
- tk.X → "x"
- tk.Y → "y"
- tk.NONE → "none"
- tk.LEFT → "left"
- tk.RIGHT → "right"
- etc.
You can have a look at the Tkinter sources (e.g. /usr/lib/python3.x/tkinter)
|
|
||
| # Quiz info frame at top | ||
| quiz_frame = ttk.LabelFrame(main_frame, text="Quiz Information") | ||
| quiz_frame.pack(fill=tk.X, pady=(0, 5)) |
There was a problem hiding this comment.
Type safety, see line 35 review comment
|
|
||
| # Content frame for 2 panes (Matrix + Editor) | ||
| content_frame = ttk.Frame(main_frame) | ||
| content_frame.pack(fill=tk.BOTH, expand=True) |
There was a problem hiding this comment.
Type safety, see line 35 review comment
|
|
||
| # Matrix controls | ||
| matrix_controls = ttk.Frame(matrix_frame) | ||
| matrix_controls.pack(fill=tk.X, padx=5, pady=5) |
There was a problem hiding this comment.
Type safety, see line 35 review comment
| self.current_question.answerMediaPath = self.q_answer_media_var.get() or None | ||
| self.create_matrix() # Refresh matrix to show changes | ||
|
|
||
| def on_question_text_changed(self, event): |
There was a problem hiding this comment.
Same issue as line 295. Instead of "event", use "_event".
Pythonic idiom indicating "callback is passed by called function, but not used in this particular function".
| self.current_question.question = self.q_question_text.get("1.0", tk.END).strip() | ||
| self.create_matrix() # Refresh matrix to show new question text | ||
|
|
||
| def on_answer_text_changed(self, event): |
There was a problem hiding this comment.
Same issue as line 295. Instead of "event", use "_event".
Pythonic idiom indicating "callback is passed by called function, but not used in this particular function".
| if self.current_question: | ||
| self.current_question.answer = self.q_answer_text.get("1.0", tk.END).strip() | ||
|
|
||
| def on_note_text_changed(self, event): |
There was a problem hiding this comment.
Same issue as line 295. Instead of "event", use "_event".
Pythonic idiom indicating "callback is passed by called function, but not used in this particular function".
| for col in range(len(self.quiz.categories) + 1): | ||
| grid_frame.columnconfigure(col, weight=1) | ||
|
|
||
| def find_question_by_value(self, category, value): |
There was a problem hiding this comment.
nitpick: This is a static method.
Clean approach: use @staticmethod decorator, remove self keyword.
Example:
@staticmethod
def find_question_by_value(category, value):
"""Find a question in a category by its value"""
for question in category.questions:
if question.value == value:
return question
return None
| return question | ||
| return None | ||
|
|
||
| def lighten_color(self, hex_color): |
2 participants
I planned to use this nice project on a company event. The unity editor was kind of slow and hard to keep an overview so I let the AI reverse engineer the
.jeopardyformat and come up with a python editor instead.Maybe you find this useful to merge upstream.