Skip to content

Conversation

@gcasa
Copy link
Member

@gcasa gcasa commented Sep 4, 2025

This PR provides an implementation of NSFontAssetRequest.

The test for this is here:

https://github.com/gcasa/NSFontAssetRequest_test

@gcasa gcasa requested a review from fredkiefer as a code owner September 4, 2025 06:20
@gcasa
Copy link
Member Author

gcasa commented Sep 7, 2025

@fredkiefer I have tested this and it is working. Please take a look and let me know if it is acceptable.

@gcasa
Copy link
Member Author

gcasa commented Sep 11, 2025

@fredkiefer Please review when ready. This is tested and working. The font is properly added (at least the ones I have tested).

@fredkiefer
Copy link
Member

I am going on a two week holiday trip right now and won't have the time to review this properly. If you want a quick review, feel free to ask @rfm .

@gcasa
Copy link
Member Author

gcasa commented Sep 12, 2025

I am going on a two week holiday trip right now and won't have the time to review this properly. If you want a quick review, feel free to ask @rfm .

Okay. Have a nice trip. :)

@gcasa
Copy link
Member Author

gcasa commented Sep 12, 2025

@rfm Please take a look. I have addressed @fredkiefer 's latest comments.

@rfm
Copy link
Contributor

rfm commented Sep 13, 2025

I'm away from home for the next week and a half, and don't think I'll be able to look at this until I'm back.

@gcasa gcasa requested a review from Copilot September 13, 2025 06:40
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements NSFontAssetRequest, a class for downloading and installing font assets dynamically. The implementation provides a complete font downloading system with support for CSS-based font URLs (like Google Fonts), direct font file URLs, and cross-platform font installation.

  • Implements NSFontAssetRequest with progress tracking and completion handlers
  • Adds GSFontAssetDownloader as a pluggable backend for font downloading and installation
  • Integrates font cache refresh notifications to update the font panel when new fonts are installed

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
Source/NSFontAssetRequest.m Complete implementation of NSFontAssetRequest class with download orchestration
Source/GSFontAssetDownloader.m New font downloading backend with CSS parsing, validation, and installation
Source/NSFontManager.m Adds font cache refresh method with notification support
Source/NSFontPanel.m Integrates font manager notifications to refresh panel when fonts change
Source/GSFontInfo.m Adds font cache refresh functionality to font enumerator
Headers/AppKit/NSFontAssetRequest.h Header updates with GNUstep extensions
Headers/Additions/GNUstepGUI/GSFontAssetDownloader.h New header for font downloader class
Headers/AppKit/NSProgressIndicator.h Adds modern style constants for compatibility
Headers/AppKit/NSFontManager.h Adds private method and notification declarations
Headers/Additions/GNUstepGUI/GSFontInfo.h Adds refreshFontCache method declaration
Source/GNUmakefile Updates build system to include new GSFontAssetDownloader files

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@gcasa
Copy link
Member Author

gcasa commented Sep 15, 2025

The NSFontAssetRequest_test has evolved into something akin to Font Book.app on macOS.
font_preview

@gcasa
Copy link
Member Author

gcasa commented Sep 15, 2025

I think this one is ready to merge @rfm and @fredkiefer. Please review and let me know.

@gcasa
Copy link
Member Author

gcasa commented Sep 24, 2025

Hey guys, I think this PR is ready @rfm , @fredkiefer

@gcasa
Copy link
Member Author

gcasa commented Sep 27, 2025

I realize you guys are busy. Unless there is an objection, I will go ahead and merge this given that the functionality is working and since it is absent in the baseline. @fredkiefer @rfm

@gcasa
Copy link
Member Author

gcasa commented Sep 27, 2025

Please let me know.

@gcasa
Copy link
Member Author

gcasa commented Sep 28, 2025

All concerns with this PR have been addressed. I am merging and closing this PR.

@gcasa gcasa merged commit e6bbd85 into master Sep 28, 2025
4 checks passed
@gcasa gcasa deleted the NSFontAssetRequest_branch branch September 28, 2025 03:43
@rfm
Copy link
Contributor

rfm commented Sep 28, 2025 via email

@fredkiefer
Copy link
Member

Most of this PR looks OK to me (although I would prefer if we do large format changes in files separately). But the part about font installation looks wrong and dangerous to me and actually I would prefer if we could revert this merge.
Why do I think it is wrong? Here we have operations that normally happen in the backend and our backends support very different types of font management, whereas the code here is specific to fontconfig and even requires hardcoded paths. This will not work on many machines and some old backends.
Why do I think it is dangerous? Fonts are basically executable code. Installing a new font file, especially permanent and system wide, is something that should be treated very carefully. Here we are adding code in every GNUstep application that is capable of doing so in the background. This should be discussed in mire detail before adding.

@gcasa
Copy link
Member Author

gcasa commented Sep 29, 2025

Most of this PR looks OK to me (although I would prefer if we do large format changes in files separately). But the part about font installation looks wrong and dangerous to me and actually I would prefer if we could revert this merge.

Why do I think it is wrong? Here we have operations that normally happen in the backend and our backends support very different types of font management, whereas the code here is specific to fontconfig and even requires hardcoded paths. This will not work on many machines and some old backends.

Why do I think it is dangerous? Fonts are basically executable code. Installing a new font file, especially permanent and system wide, is something that should be treated very carefully. Here we are adding code in every GNUstep application that is capable of doing so in the background. This should be discussed in mire detail before adding.

I will revert this in the morning. I hadn't thought about some of these considerations. Thanks @fredkiefer

@gcasa
Copy link
Member Author

gcasa commented Sep 29, 2025

I will make another PR and move the relevant code to the backend.

@fredkiefer
Copy link
Member

Thank you Greg, I think it would be sufficient to just remove the font installation code here and not to revert the whole PR. Th question then is, how should we structure that code in the backend to reduce the security risks? One position would be to move the decision over to the user. If they really want to download and install a font from a dubious website it is their problem.

@gcasa
Copy link
Member Author

gcasa commented Oct 2, 2025

Thank you Greg, I think it would be sufficient to just remove the font installation code here and not to revert the whole PR. Th question then is, how should we structure that code in the backend to reduce the security risks? One position would be to move the decision over to the user. If they really want to download and install a font from a dubious website it is their problem.

I am working to answer these questions right now. I am not sure. I think confirmation by the user is sufficient. The downloader uses google fonts, mainly because I can't think of any other place where I can get the full font list AND then download the fonts for free. :/ I implemented it so that a developer could implement one and use that if needed so the developer isn't locked in. So the user can't just get a font from anywhere. Thanks fred. I hope your vacation was restful. GC

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants