Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add l_display.py #16

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,25 @@ This repository contains a collection of scripts designed to update MEI 3 files
3. Run the `liberbatch.py` script

This will update the Liber Usualis files from MEI 3 to MEI 5.



**`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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo in file name?


**Usage**
-----

1. Perform the steps listed above.
2. In the `l_display.py` script, replace the image_folder variable with the path to the folder containing your image files.
3. Run the `l_display.py` script.
4. You should see, from left to right:
* An image of a page with bounding boxes drawn on it
* A list of text boxes showcasing what the MEI file claims is the content of each bounding box.
* Two buttons, labelled `-` and `+` respectively, and one button labelled Save.
5. If need be, the slider at the top can be used to adjust the scale of the bounding boxes.
6. The text boxes showcase what should be the content of their associated box. Which bounding box is associated with which text box can be ascertained by its colour. If their is a mistake in the text, simply edit the content of the text box.
7. Once all the text boxes have been modified to your liking, hit the Save button. This will not edit the content of the MEI file, but rather generate a new one.
8. Use the `-` and `+` buttons, or the arrow keys, to go to the previous or next page, respectively.
295 changes: 295 additions & 0 deletions l_display.py
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the l_display.py name? Could it be something more descriptive?

Copy link
Contributor Author

@CThierrin CThierrin Jul 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It displays <l> elements

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like "lyric_editor_gui.py" would say more about what it does...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you using pylint? There are unused arguments and variables in this file.

Original file line number Diff line number Diff line change
@@ -0,0 +1,295 @@
import tkinter as tk
from tkinter import ttk
from PIL import Image, ImageTk, ImageDraw
import lxml.etree as ET
Comment on lines +1 to +4
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New dependencies introduced here...can you document somehow?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't Pillow?

import os


def create_gui():
"""
Create the graphical user interface (GUI) for editing MEI files.

This function creates a GUI with an image display, text fields for editing,
and buttons for navigating and saving changes.
"""
mei_folder = "Liber Usualis - mei5"
image_folder = "YOUR FOLDER HERE"
Comment on lines +15 to +16
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you keep these as constants, I would define them outside of the function.

index = 1

# Find the first existing image file
while True:
image_file = f"liber_{index:04d}.jpg"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file naming convention feels like a variable? Or a requirement to be documented...

image_path = os.path.join(image_folder, image_file)
if os.path.exists(image_path):
break
index += 1
if index > 2340: # assume max 9999 images
print("No image files found!")
return
Comment on lines +26 to +28
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?


# Create the GUI
window = tk.Tk()
window.title("MEI Editor")

# Create a label to display the current index
index_label = ttk.Label(window, text=f"Page {index:04d}")
index_label.pack(fill="x", padx=5, pady=5)

# Create a slider to change the size of the rectangles
scale_slider = ttk.Scale(window, from_=0.1, to=5, orient="horizontal")
scale_slider.set(1) # Initial value
scale_slider.pack(fill="x", padx=5, pady=5)
# Create a label to display the current scale
scale_label = ttk.Label(window, text=f"Scale: {scale_slider.get():.2f}")
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):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you need the nested functions? Why not just define them outside of the other function?

"""
Update the image and MEI file based on the current index.

This function is called when the user navigates to a new page or adjusts the scale.
It updates the image display, text fields, and scale label.
"""
nonlocal index
mei_file = f"{index:04d}_corr - mei5.mei"
mei_path = os.path.join(mei_folder, mei_file)
tree = ET.parse(mei_path)
root = tree.getroot()

image_file = f"liber_{index:04d}.jpg"
image_path = os.path.join(image_folder, image_file)
image = Image.open(image_path)
image = image.resize(
(image.width // 4, image.height // 4)
) # Scale the image down by the current scale
if image.mode != "RGB":
image = image.convert("RGB")

# Draw rectangles on the image based on the zone elements

zones = root.findall(".//{http://www.music-encoding.org/ns/mei}zone")
draw = ImageDraw.Draw(image)
scale = scale_slider.get() # Get the current scale from the slider
i = -1
colours = [
"red",
"yellow",
"green",
"brown",
# "scarlet"
"black",
# "ochre"
# "peach"
# "ruby"
"olive",
"violet",
# "fawn"
# "lilac"
"gold",
"chocolate",
# "mauve"
# "cream"
"crimson",
"silver",
# "rose"
"azure",
# "lemon"
# "russet"
"grey",
"purple",
# "white"
"pink",
"orange",
"blue",
]
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)]
Comment on lines +106 to +123
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

)

photo = ImageTk.PhotoImage(image)

# Update the image label
image_label.config(image=photo)
image_label.image = photo

# Clear the text frame
for widget in text_frame.winfo_children():
widget.destroy()

# Update the text fields
text_fields.clear()
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} ({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}")
Comment on lines +141 to +151
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you do this in the loop you already did of <l> elements?


def increment_index():
"""
Increment the index to display the next page.

This function is called when the user clicks the "+" button or presses the right arrow key.
"""
nonlocal index
while True:
index += 1
image_file = f"liber_{index:04d}.jpg"
image_path = os.path.join(image_folder, image_file)
Comment on lines +161 to +163
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

magic number...

index = 1
update_image()
break

def decrement_index():
"""
Decrement the index to display the previous page.

This function is called when the user clicks the "-" button or presses the left arrow key.
"""
nonlocal index
while True:
index -= 1
if index < 1:
image_file = "liber_0001.jpg"
image_path = os.path.join(image_folder, image_file)
if os.path.exists(image_path):
update_image()
break
else:
index = 2341
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Magic number?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are 2340 pages in the Liber Usualis. I went with 2341 to give myself a buffer

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

image_file = f"liber_{index:04d}.jpg"
image_path = os.path.join(image_folder, image_file)
if os.path.exists(image_path):
update_image()
break

scale_slider.bind("<ButtonRelease-1>", lambda event: update_image())
# Bind arrow keys to increment and decrement index
window.bind("<Right>", lambda event: increment_index())
window.bind("<Left>", lambda event: decrement_index())

# Load the initial image
mei_file = f"{index:04d}_corr - mei5.mei"
mei_path = os.path.join(mei_folder, mei_file)
tree = ET.parse(mei_path)
root = tree.getroot()

image_file = f"liber_{index:04d}.jpg"
image_path = os.path.join(image_folder, image_file)
image = Image.open(image_path)
image = image.resize(
(image.width // 4, image.height // 4)
) # Scale the image down by a factor of two

photo = ImageTk.PhotoImage(image)

# Create a label to display the image
image_label = ttk.Label(window, image=photo)
image_label.image = photo
image_label.pack(side=tk.LEFT, padx=5, pady=5)

# Create a canvas and a frame inside the canvas
text_canvas = tk.Canvas(window, width=400, height=400)
text_canvas.pack(side=tk.LEFT, fill="both", expand=True)

text_frame = ttk.Frame(text_canvas)
text_frame_id = text_canvas.create_window((0, 0), window=text_frame, anchor="nw")

# Create a scrollbar and associate it with the canvas
text_scrollbar = ttk.Scrollbar(window, orient="vertical", command=text_canvas.yview)
text_scrollbar.pack(side=tk.LEFT, fill="y")

text_canvas.configure(yscrollcommand=text_scrollbar.set)

# Iterate over the <l> elements and create a text field for each one
text_fields = []
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)
Comment on lines +235 to +244
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a copy-paste of what happens above with the update_image function...could they be combined?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But why couldn't you put lines 235-244 in a function called create_text_fields and call that function in both places?


# Update the scroll region of the canvas
text_frame.update_idletasks()
text_canvas.configure(scrollregion=text_canvas.bbox("all"))

# Add buttons to increment and decrement the index
index_frame = ttk.Frame(window)
index_frame.pack(fill="x")

decrement_button = ttk.Button(index_frame, text="-", command=decrement_index)
decrement_button.pack(side=tk.LEFT, padx=5, pady=5)

increment_button = ttk.Button(index_frame, text="+", command=increment_index)
increment_button.pack(side=tk.LEFT, padx=5, pady=5)

# Add a button to save the changes
save_button = ttk.Button(
index_frame,
text="Save",
command=lambda: save_changes(
window, tree, text_fields, f"{mei_file[:-4]}_TEXT.mei"
),
)
save_button.pack(side=tk.LEFT, padx=5, pady=5)

# Start the GUI event loop
window.mainloop()


def save_changes(window, tree, text_fields, filename):
"""
Save the edited text fields to a new MEI file.

This function is called when the user clicks the "Save" button.
It updates the MEI file with the edited text fields and saves it to a new file.
"""
# Iterate over the text fields and update the corresponding <l> elements
for i, elem in enumerate(
tree.findall(".//{http://www.music-encoding.org/ns/mei}l")
):
elem.text = text_fields[i].get()

# Create a new MEI file with the updated contents
new_tree = ET.ElementTree(tree.getroot())
new_tree.write(
f"{filename[:-4]} new-text.mei", encoding="utf8", xml_declaration=True
)


# Example usage
create_gui()