Enhance font management with Google Fonts integration, automatic clea… #1
Conversation
…nup of temporary files, and improved preview generation
There was a problem hiding this comment.
Pull request overview
This pull request introduces Google Fonts integration, automatic server cleanup, and enhanced font management capabilities for PyMontage v1.0.1. The changes add significant new functionality for font searching and downloading, along with comprehensive cleanup mechanisms for temporary files.
Changes:
- Added Google Fonts API integration with search and download capabilities
- Implemented automatic cleanup system for temporary files, cache, and Python bytecode on server shutdown
- Enhanced font resolution with multi-tier fallback system (absolute path → local cache → Windows fonts → Google Fonts download)
- Added preview video generation feature with low-quality fast rendering
- Moved HEIC/HEIF support initialization to module level with better error handling
- Updated frontend with live font search UI and technical preview section
Reviewed changes
Copilot reviewed 4 out of 6 changed files in this pull request and generated 22 comments.
Show a summary per file
| File | Description |
|---|---|
| video_engine.py | Added font download/resolution functions, HEIC support, preview generation, silent audio fallback |
| app.py | Added cleanup system with signal handlers, font search/download endpoints, preview endpoint |
| templates/index.html | Added font search UI, technical preview section with statistics, JavaScript handlers for new features |
| requirements.txt | Added requests library dependency for API calls |
| README.md | Updated version to 1.0.1, documented new font and cleanup features |
| .gitignore | Added patterns for downloaded fonts, Python cache files, and MoviePy temp files |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for file in files: | ||
| if file.filename: | ||
| file.save(os.path.join(session_images_path, file.filename)) |
There was a problem hiding this comment.
The uploaded filenames are used directly to save files without sanitization. This could allow directory traversal attacks if a malicious filename like '../../sensitive.mp3' is uploaded. Sanitize filenames using a function like werkzeug.utils.secure_filename() before saving.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
video_engine.py
Outdated
| if ttf_urls: | ||
| # Download the first TTF file | ||
| ttf_response = requests.get(ttf_urls[0], timeout=30) | ||
| if ttf_response.status_code == 200: | ||
| font_path = os.path.join(save_dir, f"{font_name}-Regular.ttf") |
There was a problem hiding this comment.
The font file is saved with a hardcoded '-Regular.ttf' suffix, but the actual font weight or style returned from Google Fonts CSS may vary. The CSS might return a Bold, Italic, or other variant depending on what Google serves. This could result in the wrong font file being saved or mismatches between the saved filename and the actual font style. Consider parsing the font URL to extract the actual variant or handling multiple font files.
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
|
|
||
| path = install_google_font(name) | ||
| if path and os.path.exists(path): | ||
| return jsonify({'success': True, 'path': path, 'name': name}) |
There was a problem hiding this comment.
The font download endpoint returns the absolute local filesystem path to the frontend. This exposes internal server paths to clients. Consider returning just the filename or a relative path instead, and handle path resolution on the server side when processing the font selection.
| return jsonify({'success': True, 'path': path, 'name': name}) | |
| # Return a relative path instead of an absolute filesystem path | |
| relative_path = os.path.relpath(path, start=os.getcwd()) | |
| return jsonify({'success': True, 'path': relative_path, 'name': name}) |
| try: | ||
| from pillow_heif import register_heif_opener | ||
| register_heif_opener() | ||
| print("✅ HEIC/HEIF support enabled") |
There was a problem hiding this comment.
Print statement may execute during import.
| register_heif_opener() | ||
| print("✅ HEIC/HEIF support enabled") | ||
| except ImportError: | ||
| print("⚠️ pillow-heif not installed, HEIC/HEIF files won't be supported") |
There was a problem hiding this comment.
Print statement may execute during import.
| except ImportError: | ||
| print("⚠️ pillow-heif not installed, HEIC/HEIF files won't be supported") | ||
| except Exception as e: | ||
| print(f"⚠️ Could not enable HEIC/HEIF support: {e}") |
There was a problem hiding this comment.
Print statement may execute during import.
|
@Ariel-Gal I've opened a new pull request, #2, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@Ariel-Gal I've opened a new pull request, #3, to work on those changes. Once the pull request is ready, I'll request review from you. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Ariel-Gal <82096633+Ariel-Gal@users.noreply.github.com>
Co-authored-by: Ariel-Gal <82096633+Ariel-Gal@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 6 changed files in this pull request and generated 13 comments.
Comments suppressed due to low confidence (1)
video_engine.py:1953
- The code declares
temp_audio_pathat line 1906 before the try block, but then only writes to it at line 1921 inside the try block. If the audio creation fails and falls back to a silent track (lines 1912-1917), the silent audio clip is still written totemp_audio_pathat line 1921. However, the cleanup at lines 1947-1953 tries to close and remove this file. Ifmixed_audiois a lambda-basedAudioClip(from the fallback), callingmixed_audio.close()might not work as expected or could raise an error. Consider wrapping the close operation in a try-except or checking if the audio clip has a close method.
temp_audio_path = os.path.join(os.path.dirname(OUTPUT_FILE_PATH), 'temp_mixed_audio.mp3')
try:
if audio_paths:
mixed_audio = create_mixed_audio(audio_paths, fade_duration=crossfade_duration)
else:
raise ValueError("No audio paths provided")
except Exception as e:
fallback_duration_per_weight = config.get('silent_duration_per_weight', 3.0) if config else 3.0
intro_time = FIXED_INTRO_TIME
estimated_duration = max(intro_time + total_weighted_slides * fallback_duration_per_weight, intro_time + fallback_duration_per_weight)
print(f" ⚠ Audio unavailable ({e}); using silent track ({estimated_duration:.1f}s)")
mixed_audio = AudioClip(lambda t: 0.0, duration=estimated_duration, fps=44100)
audio_duration = mixed_audio.duration
# Save mixed audio temporarily for compose_final_video
mixed_audio.write_audiofile(temp_audio_path, verbose=False, logger=None)
AUDIO_FILE_PATH = temp_audio_path
# Step 3: Timing
print("Step 3: Calculating exact timing...")
duration_per_weight = calculate_exact_slide_duration(media_groups, audio_duration, USE_GRID_2X2)
# Step 4: Creating clips
print("Step 4: Creating clips...")
target_size = (TARGET_WIDTH, TARGET_HEIGHT)
image_clips, video_clips, _, _ = create_slideshow_clips(
media_groups, duration_per_weight, target_size, TRANSITION_DURATION
)
# Step 5: Titles & Pause
print("Step 5: Creating title cards...")
intro_clip = create_title_card(INTRO_TEXT, INTRO_CARD_DURATION, target_size) if INTRO_TEXT else None
outro_clip = create_title_card(OUTRO_TEXT, OUTRO_CARD_DURATION, target_size) if OUTRO_TEXT else None
opening_pause = create_pause_clip(OPENING_PAUSE_DURATION, target_size, fade_in=True, fade_out=False)
closing_pause = create_pause_clip(CLOSING_PAUSE_DURATION, target_size, fade_in=False, fade_out=True)
# Step 6: Rendering
print("Step 6: Rendering video...")
compose_final_video(image_clips, video_clips, AUDIO_FILE_PATH, OUTPUT_FILE_PATH, target_size, intro_clip, outro_clip, opening_pause, closing_pause)
# Cleanup temporary audio file
if os.path.exists(temp_audio_path):
try:
mixed_audio.close()
os.remove(temp_audio_path)
print("✓ Cleaned up temporary audio file")
except Exception as e:
print(f"⚠ Warning: Could not delete temporary audio file: {e}")
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try: | ||
| os.remove(filepath) | ||
| except: | ||
| pass | ||
| print(" ✓ Removed .pyc and .pyo files") | ||
|
|
||
| # Remove moviepy temp files | ||
| temp_dir = tempfile.gettempdir() | ||
| for file in os.listdir(temp_dir): | ||
| if 'mpy' in file.lower(): | ||
| try: | ||
| filepath = os.path.join(temp_dir, file) | ||
| if os.path.isfile(filepath): | ||
| os.remove(filepath) | ||
| except: | ||
| pass |
There was a problem hiding this comment.
Bare except: clauses at lines 48 and 60 silently swallow all exceptions including KeyboardInterrupt and SystemExit. This makes debugging difficult and can mask serious errors. Consider catching specific exceptions like OSError or PermissionError instead, or at minimum use except Exception: to avoid catching system-level exceptions.
video_engine.py
Outdated
| Downloads a font from Google Fonts, extracts it, and saves to assets/fonts directory. | ||
| Uses multiple fallback methods to download the font. |
There was a problem hiding this comment.
The docstring at line 50 states the function "extracts it" but the actual implementation doesn't extract from a ZIP file. It downloads TTF files directly from the Google Fonts CSS API. The docstring should be updated to accurately reflect that it downloads TTF files directly, not extracts from archives.
| Downloads a font from Google Fonts, extracts it, and saves to assets/fonts directory. | |
| Uses multiple fallback methods to download the font. | |
| Downloads a TrueType font (.ttf) from the Google Fonts CSS API and saves it to the assets/fonts directory. | |
| Attempts to download the first available regular TTF variant; on failure, instructs manual installation. |
| if 'mpy' in file.lower(): | ||
| try: | ||
| filepath = os.path.join(temp_dir, file) | ||
| if os.path.isfile(filepath): | ||
| os.remove(filepath) |
There was a problem hiding this comment.
The cleanup logic for MoviePy temp files at lines 54-61 is too broad and potentially dangerous. It removes any file in the system temp directory containing 'mpy' in the filename (case-insensitive). This could accidentally delete unrelated files from other applications or user data that happens to contain 'mpy' in the name. Consider using a more specific pattern like 'moviepy_' or checking for specific MoviePy temporary file patterns.
| if 'mpy' in file.lower(): | |
| try: | |
| filepath = os.path.join(temp_dir, file) | |
| if os.path.isfile(filepath): | |
| os.remove(filepath) | |
| filepath = os.path.join(temp_dir, file) | |
| # Only remove files that match MoviePy's known temp filename prefix | |
| if os.path.isfile(filepath) and file.lower().startswith('moviepy_'): | |
| try: | |
| os.remove(filepath) |
| let audioDuration = 0; | ||
| selectedAudio.forEach(file => { | ||
| // Rough estimate: 128 kbps = 16 KB/s | ||
| const estimatedSeconds = (file.size / 16384) * 1; |
There was a problem hiding this comment.
The audio duration estimation formula at line 483 has a redundant multiplication by 1: (file.size / 16384) * 1. This can be simplified to just file.size / 16384. While this doesn't affect functionality, removing the unnecessary operation improves code clarity.
| const estimatedSeconds = (file.size / 16384) * 1; | |
| const estimatedSeconds = file.size / 16384; |
| # Build absolute paths and ensure the target stays within OUTPUT_FOLDER | ||
| output_folder = os.path.abspath(app.config['OUTPUT_FOLDER']) | ||
| preview_path = os.path.abspath(os.path.join(output_folder, filename)) | ||
|
|
||
| if not preview_path.startswith(output_folder + os.path.sep): |
There was a problem hiding this comment.
The path traversal validation logic has an issue. On Windows, os.path.sep is typically \, so the check at line 194 if not preview_path.startswith(output_folder + os.path.sep) would fail for the last file in the directory if output_folder doesn't end with a separator. A safer approach is to use os.path.commonpath() or ensure both paths are normalized with trailing separators. Additionally, os.path.altsep is None on many platforms, so the check if os.path.altsep and os.path.altsep in filename could be simplified.
| const res = await fetch(`/fonts/search?q=${encodeURIComponent(query)}`); | ||
| const data = await res.json(); | ||
| resultsContainer.innerHTML = ''; | ||
| if (!data.fonts || data.fonts.length === 0) { | ||
| resultsContainer.textContent = 'No matches'; | ||
| return; | ||
| } |
There was a problem hiding this comment.
The searchFonts function doesn't check the HTTP response status either. If the server returns an error status, the code will still try to parse JSON and access data.fonts, which could fail. Add a check for res.ok before parsing the response.
|
|
||
| if response.status_code == 200: | ||
| # Parse CSS to find TTF URL | ||
| import re |
There was a problem hiding this comment.
The re module is imported inside the try block at line 84 but it's only used within this function. Consider importing re at the top of the file with other imports for consistency and better practice.
| try: | ||
| os.remove(file_path) | ||
| print(f"🗑️ Cleaned up preview: {f}") | ||
| except: |
There was a problem hiding this comment.
Similar to the cleanup handler, this code uses a bare except: clause at line 210 that catches all exceptions including system-level ones. Use except Exception: or specific exception types like OSError instead.
| except: | |
| except OSError: |
| # Simple zoom in effect | ||
| def zoom_in(t): | ||
| scale = 1 + (zoom_intensity * t / image_duration) | ||
| return scale | ||
| clip = clip.resize(lambda t: zoom_in(t)) |
There was a problem hiding this comment.
The zoom_in function defined at lines 2060-2062 creates a closure that captures zoom_intensity and image_duration from the outer scope. However, these variables are shared across all iterations of the loop. In Python, closures capture variables by reference, not by value. This means that if these variables were to change (which they don't in this code), all zoom functions would use the latest values. While this specific code doesn't have a bug because the variables don't change, it's a common closure pitfall. Consider either: 1) defining the function outside the loop and passing parameters, or 2) using default arguments to capture values at definition time.
| # Simple zoom in effect | |
| def zoom_in(t): | |
| scale = 1 + (zoom_intensity * t / image_duration) | |
| return scale | |
| clip = clip.resize(lambda t: zoom_in(t)) | |
| # Simple zoom in effect; capture configuration values at definition time | |
| clip = clip.resize( | |
| lambda t, _zoom_intensity=zoom_intensity, _image_duration=image_duration: # noqa: E501 | |
| 1 + (_zoom_intensity * t / _image_duration) | |
| ) |
| @app.route('/preview', methods=['POST']) | ||
| def preview(): | ||
| """Generate a preview video""" | ||
| session_id = str(uuid.uuid4()) | ||
| session_upload_path = os.path.join(app.config['UPLOAD_FOLDER'], session_id) | ||
| session_images_path = os.path.join(session_upload_path, 'images') | ||
| os.makedirs(session_images_path, exist_ok=True) | ||
|
|
||
| try: | ||
| # Save audio file | ||
| if 'audio' not in request.files: | ||
| return jsonify({'success': False, 'message': 'No audio file'}), 400 | ||
| audio_file = request.files['audio'] | ||
| audio_path = os.path.join(session_upload_path, 'music.mp3') | ||
| audio_file.save(audio_path) | ||
|
|
||
| # Save images | ||
| if 'images' not in request.files: | ||
| return jsonify({'success': False, 'message': 'No images'}), 400 | ||
| files = request.files.getlist('images') | ||
| for file in files: | ||
| if file.filename: | ||
| file.save(os.path.join(session_images_path, file.filename)) | ||
|
|
||
| # Generate preview | ||
| success, message = generate_video_from_web( | ||
| image_folder=session_images_path, | ||
| audio_paths=[audio_path], | ||
| output_path=os.path.join(app.config['OUTPUT_FOLDER'], f'preview_{session_id}.mp4'), | ||
| intro_text=request.form.get('intro_text', 'Preview'), | ||
| outro_text=request.form.get('outro_text', 'End'), | ||
| config={} | ||
| ) | ||
| if success: | ||
| return jsonify({'success': True, 'filename': f'preview_{session_id}.mp4'}) | ||
| else: | ||
| return jsonify({'success': False, 'message': message}), 500 | ||
|
|
||
| except Exception as e: | ||
| print(f"Preview error: {e}") | ||
| return jsonify({'success': False, 'message': str(e)}), 500 | ||
| finally: | ||
| # Clean up uploads | ||
| try: | ||
| if os.path.exists(session_upload_path): | ||
| shutil.rmtree(session_upload_path) | ||
| except Exception: | ||
| pass | ||
|
|
||
| @app.route('/get_preview/<filename>') | ||
| def get_preview(filename): | ||
| """Stream preview video""" | ||
| # Validate the filename to prevent path traversal | ||
| if not filename: | ||
| return "Invalid filename", 400 | ||
|
|
||
| # Disallow any path separators or parent directory references | ||
| if os.path.sep in filename or (os.path.altsep and os.path.altsep in filename) or '..' in filename: | ||
| return "Invalid filename", 400 | ||
|
|
||
| # Enforce expected naming pattern for preview files | ||
| if not (filename.startswith('preview_') and filename.endswith('.mp4')): | ||
| return "Invalid filename", 400 | ||
|
|
||
| # Build absolute paths and ensure the target stays within OUTPUT_FOLDER | ||
| output_folder = os.path.abspath(app.config['OUTPUT_FOLDER']) | ||
| preview_path = os.path.abspath(os.path.join(output_folder, filename)) | ||
|
|
||
| if not preview_path.startswith(output_folder + os.path.sep): | ||
| return "Invalid filename", 400 | ||
| if os.path.exists(preview_path): | ||
| return send_file(preview_path, mimetype='video/mp4') | ||
| return "Preview not found", 404 |
There was a problem hiding this comment.
The /preview endpoint (lines 126-173) and /get_preview/<filename> endpoint (lines 175-198) are defined but don't appear to be called from the frontend HTML. Additionally, these endpoints call generate_video_from_web instead of the new generate_video_preview function that was added to video_engine.py. This suggests either: 1) the endpoints are incomplete/untested, 2) they're intended for future use but shouldn't be in this release, or 3) the frontend integration is missing. Consider either completing the feature integration or removing these endpoints until they're fully implemented.
Fix font variant detection in Google Fonts downloader
🎯 PyMontage v1.0.1 - Font Management & Cleanup System
📋 Description
Release of version 1.0.1 with major improvements to font management, server cleanup, and code maintenance.
✨ New Features
Google Fonts Integration: Search and download any font directly from Google Fonts library
Enhanced Font System:
assets/fonts/directory🧹 Improvements
Auto Cleanup System: Server now cleans up all temporary files on shutdown
temp_uploads/andtemp_outputs/directories__pycache__/,.pytest_cache/)*.pyc,*.pyo)Updated .gitignore: Added entries for all cleanup targets
*.pyo,.pytest_cache/for Python cacheassets/fonts/for downloaded fonts (regenerated on demand)TEMP/,*.tmpfor MoviePy temporary files📝 Changes
app.py:
cleanup_temp_folders()function with comprehensive cleanup/fonts/searchendpoint (Google Fonts metadata API)/fonts/downloadendpoint (font installation)video_engine.py:
install_google_font()with Google Fonts API integrationresolve_font_path()with priority-based fallbacktemplates/index.html:
updateVideoDurationEstimate())README.md: Updated documentation for v1.0.1 with new features
✅ Testing
assets/fonts/and persists across sessions📚 Documentation
See README.md for updated documentation on font features and configuration.