Add l_display.py #16
Add l_display.py #16
Conversation
README updated to describe what l_display does and how to use it. In summary, it allows us to
| **`l_display.py`** | ||
| =============== | ||
|
|
||
| Many of the Liber Usualis files have typographical errors in their textual content. `l_display.py1` allows one to visualise the text content and edit it |
There was a problem hiding this comment.
Why the l_display.py name? Could it be something more descriptive?
There was a problem hiding this comment.
It displays <l> elements
There was a problem hiding this comment.
Something like "lyric_editor_gui.py" would say more about what it does...
| import tkinter as tk | ||
| from tkinter import ttk | ||
| from PIL import Image, ImageTk, ImageDraw | ||
| import lxml.etree as ET |
There was a problem hiding this comment.
New dependencies introduced here...can you document somehow?
There was a problem hiding this comment.
lxml is the only external dependency, and I assumed that anyone using this would have installed it for use with the other scripts in here
There was a problem hiding this comment.
Are you using pylint? There are unused arguments and variables in this file.
| mei_folder = "Liber Usualis - mei5" | ||
| image_folder = "YOUR FOLDER HERE" |
There was a problem hiding this comment.
This would be the perfect case for a command line argument... so that you don't introduce any file changes (and therefore potentially conflicts with pulls, etc) just to direct this to the folder containing your image.
There was a problem hiding this comment.
If you keep these as constants, I would define them outside of the function.
|
|
||
| # Find the first existing image file | ||
| while True: | ||
| image_file = f"liber_{index:04d}.jpg" |
There was a problem hiding this comment.
This file naming convention feels like a variable? Or a requirement to be documented...
| if index > 2340: # assume max 9999 images | ||
| print("No image files found!") | ||
| return |
There was a problem hiding this comment.
What's going on with the numbers in the if clause and the comment? Should they be different? Why not just check if there are any *.jpg files in the given folder?
| scale_label.pack(fill="x", padx=5, pady=5) | ||
|
|
||
| # Function to update the image and MEI file based on the index | ||
| def update_image(event=None): |
There was a problem hiding this comment.
Do you need the nested functions? Why not just define them outside of the other function?
| for elem in root.findall(".//{http://www.music-encoding.org/ns/mei}l"): | ||
| facs = elem.get("facs").strip("#") | ||
| i += 1 | ||
|
|
||
| for zone in zones: | ||
|
|
||
| if ( | ||
| zone.get("{http://www.w3.org/XML/1998/namespace}id") | ||
| and zone.get("{http://www.w3.org/XML/1998/namespace}id") == facs | ||
| ): | ||
| ulx, uly, lrx, lry = ( | ||
| int(int(zone.get("ulx")) * scale) // 4, | ||
| int(int(zone.get("uly")) * scale) // 4, | ||
| int(int(zone.get("lrx")) * scale) // 4, | ||
| int(int(zone.get("lry")) * scale) // 4, | ||
| ) | ||
| draw.rectangle( | ||
| [(ulx, uly), (lrx, lry)], outline=colours[i % len(colours)] |
There was a problem hiding this comment.
I don't think one needs to optimize for this purpose but if one were to, it strikes me that this could just be an xpath-ish query of the tree rather than a double loop.
| label = ttk.Label( | ||
| text_frame, text=f"Line {i+1} ({colours[i%len(colours)]}):" | ||
| ) | ||
| label.grid(row=i, column=0, padx=5, pady=5) | ||
| # Create a colored frame to wrap the Entry widget | ||
| text_field = ttk.Entry(text_frame, width=50) | ||
| text_field.insert(0, elem.text) | ||
| text_field.grid(row=i, column=1, padx=5, pady=5) | ||
| text_fields.append(text_field) # Function to increment the index | ||
| index_label.config(text=f"Page: {index:04d}") | ||
| scale_label.config(text=f"Scale: {scale_slider.get():.2f}") |
There was a problem hiding this comment.
Could you do this in the loop you already did of <l> elements?
| update_image() | ||
| break | ||
| else: | ||
| index = 2341 |
There was a problem hiding this comment.
There are 2340 pages in the Liber Usualis. I went with 2341 to give myself a buffer
There was a problem hiding this comment.
Fine... I think like most magic numbers, and especially because you use it multiple times in the script, it would benefit from being defined by a single variable MAX_NUM_IMAGES or some such at the top of the file and everywhere where that number is used just use the variable.
Better for the code now, and better if we ever expand this for non-liber files.
| index += 1 | ||
| image_file = f"liber_{index:04d}.jpg" | ||
| image_path = os.path.join(image_folder, image_file) |
There was a problem hiding this comment.
You're doing a lot of the same string manipulation over and over again ... do you need to? For example, here you do it to check that the file exists, and then you do it again immediately after when you call update_image
| if os.path.exists(image_path): | ||
| update_image() | ||
| break | ||
| elif index > 2341: # assume max 2341 images |
| for i, elem in enumerate( | ||
| root.findall(".//{http://www.music-encoding.org/ns/mei}l") | ||
| ): | ||
| label = ttk.Label(text_frame, text=f"Line {i+1}:") | ||
| label.grid(row=i, column=0, padx=5, pady=5) | ||
|
|
||
| text_field = ttk.Entry(text_frame, width=50) | ||
| text_field.insert(0, elem.text) | ||
| text_field.grid(row=i, column=1, padx=5, pady=5) | ||
| text_fields.append(text_field) |
There was a problem hiding this comment.
This looks like a copy-paste of what happens above with the update_image function...could they be combined?
There was a problem hiding this comment.
I don't think so, this code is for the initial image, and the update image is called when the index or scale is changed
There was a problem hiding this comment.
But why couldn't you put lines 235-244 in a function called create_text_fields and call that function in both places?
|
@CThierrin Should we close this? |
|
@dchiller Maybe? Ich seemed ameable to the idea of working on this eventually, if we can get it to interface with an LLM |
|
Ok. I am gonna create an "on hold" label and label it as that for now. |
l_display.py is a GUI that allows us to modify the text of the mei files. Further details provided in the updated README.