Release/v1.1 #8
Open
Release/v1.1 #8
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's GuideThis PR bumps the backend wheel dependency from version 1.1.10 to 1.1.11 and extends the NSIS frontend installer to automatically detect and install the Archivo font on Windows if it’s not already present. Flow diagram for Archivo font installation logic in frontend installerflowchart TD
A[Start Frontend Installation] --> B{Is Frontend Installed?}
B -- Yes --> C[Skip Download]
B -- No --> D[Download Frontend]
C --> E[Check for Archivo Font]
D --> E[Check for Archivo Font]
E -- Found --> F[Finish]
E -- Not Found --> G[Copy Archivo Font File]
G --> H[Copy to Windows Fonts]
H --> I[Add Registry Entry]
I --> J[Remove Temp Font File]
J --> F[Finish]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @jansaldo - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `scripts/headers/install_frontend.nsh:64` </location>
<code_context>
+
+ install_archivo_font:
+ ; Check if any Archivo font already exists in Windows Fonts
+ nsExec::ExecToStack 'powershell -NoProfile -ExecutionPolicy Bypass -Command "$props = (Get-ItemProperty -Path \"HKLM:\SOFTWARE\Microsoft\Windows NT\CurrentVersion\Fonts\").PSObject.Properties.Name | Where-Object { $_ -like \"Archivo*\" }; if ($props) { exit 0 } else { exit 1 }"'
+ Pop $0
+ StrCmp $0 "0" finish archivo_font_install
+
+ archivo_font_install:
</code_context>
<issue_to_address>
PowerShell registry check may require elevation and could fail silently.
Since HKLM access often requires admin rights, the registry check may fail or give incorrect results if run without elevation. Please ensure permission errors are detected and communicated to the user.
</issue_to_address>
### Comment 2
<location> `scripts/headers/install_frontend.nsh:75` </location>
<code_context>
+ File "${ARCHIVO_FONT}"
+
+ ; Copy Archivo font file to Windows Fonts
+ nsExec::ExecToLog 'powershell -NoProfile -ExecutionPolicy Bypass -Command "Copy-Item -Path \"$INSTDIR\Archivo-Regular.ttf\" -Destination \"C:\Windows\Fonts\" -Force"'
+
+ ; Add registry entry for the font
</code_context>
<issue_to_address>
Copying directly to C:\Windows\Fonts may fail without admin rights.
Check for administrator privileges before copying the font, or implement a fallback if elevation is not available.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
scripts/headers/install_frontend.nsh
Outdated
Comment on lines
64
to
66
| nsExec::ExecToStack 'powershell -NoProfile -ExecutionPolicy Bypass -Command "$props = (Get-ItemProperty -Path \"HKLM:\SOFTWARE\Microsoft\Windows NT\CurrentVersion\Fonts\").PSObject.Properties.Name | Where-Object { $_ -like \"Archivo*\" }; if ($props) { exit 0 } else { exit 1 }"' | ||
| Pop $0 | ||
| StrCmp $0 "0" finish archivo_font_install |
There was a problem hiding this comment.
issue (bug_risk): PowerShell registry check may require elevation and could fail silently.
Since HKLM access often requires admin rights, the registry check may fail or give incorrect results if run without elevation. Please ensure permission errors are detected and communicated to the user.
scripts/headers/install_frontend.nsh
Outdated
| File "${ARCHIVO_FONT}" | ||
|
|
||
| ; Copy Archivo font file to Windows Fonts | ||
| nsExec::ExecToLog 'powershell -NoProfile -ExecutionPolicy Bypass -Command "Copy-Item -Path \"$INSTDIR\Archivo-Regular.ttf\" -Destination \"C:\Windows\Fonts\" -Force"' |
There was a problem hiding this comment.
issue (bug_risk): Copying directly to C:\Windows\Fonts may fail without admin rights.
Check for administrator privileges before copying the font, or implement a fallback if elevation is not available.
Contributor
There was a problem hiding this comment.
Pull Request Overview
This pull request updates the backend dependency from version 1.1.10 to 1.1.11 and adds automatic Archivo font installation to improve the frontend user experience. The changes ensure users have the required font for proper application display.
- Updated backend dependency version across installation scripts and environment configuration
- Added automatic Archivo font detection and installation during frontend setup
- Enhanced installer cleanup to remove temporary font files after installation
Reviewed Changes
Copilot reviewed 3 out of 5 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| scripts/headers/install_frontend.nsh | Added Archivo font detection, installation, and cleanup logic |
| scripts/headers/install_backend.nsh | Updated backend wheel file references from v1.1.10 to v1.1.11 |
| resources/environment.yml | Updated pip dependency to use the new backend wheel version |
scripts/headers/install_frontend.nsh
Outdated
|
|
||
| install_archivo_font: | ||
| ; Check if any Archivo font already exists in Windows Fonts | ||
| nsExec::ExecToStack 'powershell -NoProfile -ExecutionPolicy Bypass -Command "$props = (Get-ItemProperty -Path \"HKLM:\SOFTWARE\Microsoft\Windows NT\CurrentVersion\Fonts\").PSObject.Properties.Name | Where-Object { $_ -like \"Archivo*\" }; if ($props) { exit 0 } else { exit 1 }"' |
There was a problem hiding this comment.
Using -ExecutionPolicy Bypass disables PowerShell's execution policy security feature. Consider using a more restrictive execution policy or implementing alternative font detection methods.
Suggested change
| nsExec::ExecToStack 'powershell -NoProfile -ExecutionPolicy Bypass -Command "$props = (Get-ItemProperty -Path \"HKLM:\SOFTWARE\Microsoft\Windows NT\CurrentVersion\Fonts\").PSObject.Properties.Name | Where-Object { $_ -like \"Archivo*\" }; if ($props) { exit 0 } else { exit 1 }"' | |
| nsExec::ExecToStack 'powershell -NoProfile -ExecutionPolicy RemoteSigned -Command "$props = (Get-ItemProperty -Path \"HKLM:\SOFTWARE\Microsoft\Windows NT\CurrentVersion\Fonts\").PSObject.Properties.Name | Where-Object { $_ -like \"Archivo*\" }; if ($props) { exit 0 } else { exit 1 }"' |
scripts/headers/install_frontend.nsh
Outdated
| File "${ARCHIVO_FONT}" | ||
|
|
||
| ; Copy Archivo font file to Windows Fonts | ||
| nsExec::ExecToLog 'powershell -NoProfile -ExecutionPolicy Bypass -Command "Copy-Item -Path \"$INSTDIR\Archivo-Regular.ttf\" -Destination \"C:\Windows\Fonts\" -Force"' |
There was a problem hiding this comment.
Hard-coded system path 'C:\Windows\Fonts' may not work on all Windows installations. Consider using environment variables like '%WINDIR%\Fonts' or PowerShell's $env:SystemRoot for better compatibility.
Suggested change
| nsExec::ExecToLog 'powershell -NoProfile -ExecutionPolicy Bypass -Command "Copy-Item -Path \"$INSTDIR\Archivo-Regular.ttf\" -Destination \"C:\Windows\Fonts\" -Force"' | |
| nsExec::ExecToLog 'powershell -NoProfile -ExecutionPolicy Bypass -Command "Copy-Item -Path \"$INSTDIR\Archivo-Regular.ttf\" -Destination \"$env:SystemRoot\Fonts\" -Force"' |
scripts/headers/install_frontend.nsh
Outdated
Comment on lines
68
to
84
| archivo_font_install: | ||
| ; Copy Archivo font file | ||
| DetailPrint "Installing Archivo font..." | ||
| SetOutPath $INSTDIR | ||
| File "${ARCHIVO_FONT}" | ||
|
|
||
| ; Copy Archivo font file to Windows Fonts | ||
| nsExec::ExecToLog 'powershell -NoProfile -ExecutionPolicy Bypass -Command "Copy-Item -Path \"$INSTDIR\Archivo-Regular.ttf\" -Destination \"C:\Windows\Fonts\" -Force"' | ||
|
|
||
| ; Add registry entry for the font | ||
| nsExec::ExecToLog 'powershell -NoProfile -ExecutionPolicy Bypass -Command "New-ItemProperty -Path \"HKLM:\SOFTWARE\Microsoft\Windows NT\CurrentVersion\Fonts\" -Name \"Archivo (TrueType)\" -PropertyType String -Value \"Archivo-Regular.ttf\" -Force"' | ||
| DetailPrint "Archivo font installed successfully." | ||
|
|
||
| ; Remove Archivo font file | ||
| DetailPrint "Removing temporary Archivo font file..." | ||
| Delete "$INSTDIR\Archivo-Regular.ttf" | ||
|
|
There was a problem hiding this comment.
Modifying HKLM registry requires administrator privileges. The installation should verify elevated permissions before attempting registry modifications, or handle potential access denied errors gracefully.
Suggested change
| archivo_font_install: | |
| ; Copy Archivo font file | |
| DetailPrint "Installing Archivo font..." | |
| SetOutPath $INSTDIR | |
| File "${ARCHIVO_FONT}" | |
| ; Copy Archivo font file to Windows Fonts | |
| nsExec::ExecToLog 'powershell -NoProfile -ExecutionPolicy Bypass -Command "Copy-Item -Path \"$INSTDIR\Archivo-Regular.ttf\" -Destination \"C:\Windows\Fonts\" -Force"' | |
| ; Add registry entry for the font | |
| nsExec::ExecToLog 'powershell -NoProfile -ExecutionPolicy Bypass -Command "New-ItemProperty -Path \"HKLM:\SOFTWARE\Microsoft\Windows NT\CurrentVersion\Fonts\" -Name \"Archivo (TrueType)\" -PropertyType String -Value \"Archivo-Regular.ttf\" -Force"' | |
| DetailPrint "Archivo font installed successfully." | |
| ; Remove Archivo font file | |
| DetailPrint "Removing temporary Archivo font file..." | |
| Delete "$INSTDIR\Archivo-Regular.ttf" | |
| archivo_font_install: | |
| ; Check for administrator privileges before modifying HKLM | |
| UserInfo::GetAccountType | |
| Pop $0 | |
| StrCmp $0 "admin" 0 not_admin | |
| ; Copy Archivo font file | |
| DetailPrint "Installing Archivo font..." | |
| SetOutPath $INSTDIR | |
| File "${ARCHIVO_FONT}" | |
| ; Copy Archivo font file to Windows Fonts | |
| nsExec::ExecToLog 'powershell -NoProfile -ExecutionPolicy Bypass -Command "Copy-Item -Path \"$INSTDIR\Archivo-Regular.ttf\" -Destination \"C:\Windows\Fonts\" -Force"' | |
| ; Add registry entry for the font | |
| nsExec::ExecToStack 'powershell -NoProfile -ExecutionPolicy Bypass -Command "New-ItemProperty -Path \"HKLM:\SOFTWARE\Microsoft\Windows NT\CurrentVersion\Fonts\" -Name \"Archivo (TrueType)\" -PropertyType String -Value \"Archivo-Regular.ttf\" -Force"' | |
| Pop $1 | |
| StrCmp $1 "0" font_reg_success font_reg_fail | |
| font_reg_fail: | |
| MessageBox MB_OK|MB_ICONEXCLAMATION "Failed to add font registry entry. Please run the installer as administrator." | |
| Abort | |
| font_reg_success: | |
| DetailPrint "Archivo font installed successfully." | |
| ; Remove Archivo font file | |
| DetailPrint "Removing temporary Archivo font file..." | |
| Delete "$INSTDIR\Archivo-Regular.ttf" | |
| Goto finish | |
| not_admin: | |
| MessageBox MB_OK|MB_ICONEXCLAMATION "Administrator privileges are required to install the Archivo font. Please run the installer as administrator." | |
| Abort | |
scripts/headers/install_frontend.nsh
Outdated
Comment on lines
71
to
84
| SetOutPath $INSTDIR | ||
| File "${ARCHIVO_FONT}" | ||
|
|
||
| ; Copy Archivo font file to Windows Fonts | ||
| nsExec::ExecToLog 'powershell -NoProfile -ExecutionPolicy Bypass -Command "Copy-Item -Path \"$INSTDIR\Archivo-Regular.ttf\" -Destination \"C:\Windows\Fonts\" -Force"' | ||
|
|
||
| ; Add registry entry for the font | ||
| nsExec::ExecToLog 'powershell -NoProfile -ExecutionPolicy Bypass -Command "New-ItemProperty -Path \"HKLM:\SOFTWARE\Microsoft\Windows NT\CurrentVersion\Fonts\" -Name \"Archivo (TrueType)\" -PropertyType String -Value \"Archivo-Regular.ttf\" -Force"' | ||
| DetailPrint "Archivo font installed successfully." | ||
|
|
||
| ; Remove Archivo font file | ||
| DetailPrint "Removing temporary Archivo font file..." | ||
| Delete "$INSTDIR\Archivo-Regular.ttf" | ||
|
|
There was a problem hiding this comment.
The font file is copied to $INSTDIR temporarily and then deleted. Consider copying directly to the destination to avoid unnecessary file operations and potential cleanup issues if the installation fails.
Suggested change
| SetOutPath $INSTDIR | |
| File "${ARCHIVO_FONT}" | |
| ; Copy Archivo font file to Windows Fonts | |
| nsExec::ExecToLog 'powershell -NoProfile -ExecutionPolicy Bypass -Command "Copy-Item -Path \"$INSTDIR\Archivo-Regular.ttf\" -Destination \"C:\Windows\Fonts\" -Force"' | |
| ; Add registry entry for the font | |
| nsExec::ExecToLog 'powershell -NoProfile -ExecutionPolicy Bypass -Command "New-ItemProperty -Path \"HKLM:\SOFTWARE\Microsoft\Windows NT\CurrentVersion\Fonts\" -Name \"Archivo (TrueType)\" -PropertyType String -Value \"Archivo-Regular.ttf\" -Force"' | |
| DetailPrint "Archivo font installed successfully." | |
| ; Remove Archivo font file | |
| DetailPrint "Removing temporary Archivo font file..." | |
| Delete "$INSTDIR\Archivo-Regular.ttf" | |
| ; Copy Archivo font file to Windows Fonts | |
| nsExec::ExecToLog 'powershell -NoProfile -ExecutionPolicy Bypass -Command "Copy-Item -Path \"${ARCHIVO_FONT}\" -Destination \"C:\Windows\Fonts\" -Force"' | |
| ; Add registry entry for the font | |
| nsExec::ExecToLog 'powershell -NoProfile -ExecutionPolicy Bypass -Command "New-ItemProperty -Path \"HKLM:\SOFTWARE\Microsoft\Windows NT\CurrentVersion\Fonts\" -Name \"Archivo (TrueType)\" -PropertyType String -Value \"Archivo-Regular.ttf\" -Force"' | |
| DetailPrint "Archivo font installed successfully." | |
…d environment configuration
…s it's no longer needed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This pull request updates the backend dependency to a new version and enhances the frontend installation process by ensuring the required Archivo font is installed if not already present. The changes improve both dependency management and user experience during installation.
Dependency update:
aymurai-1.1.10-py3-none-any.whltoaymurai-1.1.11-py3-none-any.whlinresources/environment.yml,scripts/headers/install_backend.nsh(installation and cleanup steps). [1] [2] [3]Installer enhancements:
scripts/headers/install_frontend.nshto check for the presence of any Archivo font in the Windows Fonts directory; if not found, the Archivo font is copied, registered, and cleaned up after installation. [1] [2]Summary by Sourcery
Update the backend package to the latest release and enhance the frontend installation process to auto-install the required Archivo font if not present
New Features:
Enhancements: