-
Notifications
You must be signed in to change notification settings - Fork 2
Rename bootstrap to installer #35
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
Rename bootstrap to installer #35
Conversation
Rename binary, class, and all user-facing text from MudletBootstrap to MudletInstaller for a more intuitive Firefox-like naming convention. Changes include: - Renamed source files: MudletBootstrap.h/cpp -> MudletInstaller.h/cpp - Renamed class: MudletBootstrap -> MudletInstaller - Renamed config files: mudletbootstrap.* -> mudletinstaller.* - Updated CMakeLists.txt project and target names - Updated macOS bundle identifier: org.mudlet.bootstrap -> org.mudlet.installer - Updated all CI scripts with new binary names - Updated GitHub workflow with new release naming - Fixed typo in user-facing completion message
📝 WalkthroughWalkthroughThis pull request renames the project and its artifacts from MudletBootstrap to MudletInstaller across the repository. Changes touch CI workflows and scripts, packaging scripts for Windows/macOS/Linux, CMake configuration, C++ source and header files (class, signals/slots, and symbols), desktop and appdmg configurations, NuGet manifest, and release artifact names and paths. The edits are textual and structural renames; no functional logic, control flow, or public API behavior beyond the class/name changes was altered. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CI/package-mac.sh (1)
164-164: Incomplete rename: still referencesMudletBootstrap.This line still uses the old
MudletBootstrappattern in thermcommand, which is inconsistent with the rename.- rm -f ~/Desktop/[mM]udletBootstrap-${gameName}*.dmg + rm -f ~/Desktop/[mM]udletInstaller-${gameName}*.dmg
🧹 Nitpick comments (5)
MudletInstaller.h (1)
1-2: Consider updating the header guard to match the new class name.The header guard still uses
MUDLETDOWNLOADER_H, which doesn't match the new class nameMudletInstalleror the filenameMudletInstaller.h. While not critical, updating it toMUDLETINSTALLER_Hwould improve consistency.Apply this diff to update the header guard:
-#ifndef MUDLETDOWNLOADER_H -#define MUDLETDOWNLOADER_H +#ifndef MUDLETINSTALLER_H +#define MUDLETINSTALLER_HCI/package-mac.sh (2)
125-127: Pre-existing issue:$pgmis undefined.The variable
$pgmon line 126 is referenced but never assigned in this script. This is a pre-existing issue, not introduced by this rename, but consider defining it (e.g.,pgm=$(basename "$0")) or replacing with a literal script name.+pgm=$(basename "$0") + if [ -z "$app" ]; then echo "No MudletInstaller app folder to package given." echo "Usage: $pgm <MudletInstaller app folder to package>"
146-146: Pre-existing shellcheck warning: unquoted command substitution.The
$( [ -n "$DEBUG" ] && echo "--verbose" )should be quoted to prevent word splitting, though in this case it's benign since the output is a single word.- python ${GITHUB_WORKSPACE}/macdeployqtfix.py "${app}/Contents/MacOS/MudletInstaller" "${QT_DIR}" $( [ -n "$DEBUG" ] && echo "--verbose" ) + python "${GITHUB_WORKSPACE}/macdeployqtfix.py" "${app}/Contents/MacOS/MudletInstaller" "${QT_DIR}" $( [ -n "$DEBUG" ] && echo "--verbose" )MudletInstaller.cpp (2)
19-21: Duplicate include:<QTimer>is included twice.This is a pre-existing issue, not introduced by this rename, but worth cleaning up.
#include <QTimer> #include <QVersionNumber> -#include <QTimer>
474-477: Potential use-after-free: accessingcurrentReplyafterdeleteLater().On line 474,
currentReply->deleteLater()is called, but on line 477,currentReply->errorString()is accessed. WhiledeleteLater()defers deletion until the event loop processes it (so it's technically safe within this synchronous block), this is fragile and could break if the code is refactored. Consider capturing the error string before callingdeleteLater().This is a pre-existing issue, not introduced by this rename.
+ QString errorString = currentReply->errorString(); currentReply->deleteLater(); if (!isRetryable) { - statusLabel->setText(QString("Download failed: %1").arg(currentReply->errorString())); + statusLabel->setText(QString("Download failed: %1").arg(errorString));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.github/workflows/build.yml(4 hunks)CI/deploy-win.sh(3 hunks)CI/organize-launchers.sh(3 hunks)CI/package-linux.sh(4 hunks)CI/package-mac.sh(6 hunks)CI/package-win.sh(2 hunks)CMakeLists.txt(5 hunks)MudletInstaller.cpp(18 hunks)MudletInstaller.h(1 hunks)main.cpp(1 hunks)mudletinstaller-appdmg.json(1 hunks)mudletinstaller.desktop(1 hunks)mudletinstaller.nuspec(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
MudletInstaller.h (1)
MudletInstaller.cpp (1)
MudletInstaller(121-150)
MudletInstaller.cpp (1)
MudletInstaller.h (1)
MudletInstaller(20-81)
🪛 Shellcheck (0.11.0)
CI/package-win.sh
[warning] 119-119: ZIP_FILE_NAME appears unused. Verify use (or export if used externally).
(SC2034)
CI/package-mac.sh
[warning] 126-126: pgm is referenced but not assigned.
(SC2154)
[warning] 146-146: Quote this to prevent word splitting.
(SC2046)
🔇 Additional comments (20)
CI/package-win.sh (1)
105-117: LGTM! Consistent renaming of executable references.All references to the executable have been correctly updated from
MudletBootstrap.exetoMudletInstaller.exeacross existence checks, copy operations, debug file handling, and windeployqt usage.CI/organize-launchers.sh (1)
39-76: LGTM! Launcher naming consistently updated across all platforms.The renaming from
MudletBootstraptoMudletInstalleris correctly applied across Windows, macOS, and Linux launcher detection and organization logic. Pattern matching and destination paths follow a consistent naming convention.CI/deploy-win.sh (1)
95-135: LGTM! Windows deployment script fully aligned with MudletInstaller branding.All references to the executable have been correctly updated throughout the deployment workflow, including:
- Executable renaming and signing
- Release packaging with standardized naming
- Upload environment variables
The changes maintain consistency with the broader renaming across the project.
main.cpp (1)
7-15: LGTM! Main entry point correctly updated.The include statement and class instantiation have been properly updated from
MudletBootstraptoMudletInstaller. The changes align with the renamed header file and class definition..github/workflows/build.yml (2)
1-1: LGTM! Workflow name updated to reflect new branding.The workflow name has been correctly updated from the previous Bootstrap naming to
Build MudletInstaller.
247-315: LGTM! User-facing documentation and release metadata updated consistently.All user-facing text in the release body, installation instructions, and release metadata has been systematically updated to reference
MudletInstallerinstead of the previous naming. The changes maintain clarity and consistency across the documentation.mudletinstaller-appdmg.json (1)
2-3: LGTM! macOS DMG configuration aligned with new app name.The DMG title and app bundle path have been correctly updated to reference
MudletInstallerinstead ofMudletBootstrap, ensuring the macOS packaging configuration remains synchronized with the renamed application.mudletinstaller.nuspec (1)
4-5: LGTM! NuGet package metadata updated to reflect new branding.The package ID and title have been correctly updated to
MudletInstaller, ensuring the Windows package manifest aligns with the renamed application across all platforms.MudletInstaller.h (1)
20-24: LGTM! Class declaration correctly renamed.The class name and constructor have been properly updated from
MudletBootstraptoMudletInstaller, matching the implementation in the corresponding .cpp file.mudletinstaller.desktop (1)
1-11: LGTM!The desktop entry fields are consistently updated to reflect the MudletInstaller branding. The
Icon=mudletcorrectly remains unchanged since it references the Mudlet product icon, not the installer-specific branding.CI/package-linux.sh (1)
48-103: LGTM!All references to MudletBootstrap are consistently renamed to MudletInstaller throughout the Linux packaging script. The build paths, desktop file references, AppImage naming, and upload artifacts are all correctly updated.
CI/package-mac.sh (1)
116-198: Rename changes look consistent (with one exception noted above).The
appBaseName, plist entries, appdmg config references, and release DMG naming are all correctly updated to MudletInstaller.CMakeLists.txt (2)
2-184: LGTM!All CMake configuration is consistently updated:
- Project and executable target renamed to
MudletInstaller- Source files, resources, and translation files correctly reference new names
- macOS bundle metadata appropriately updated (
org.mudlet.installer)- All
target_*commands reference the correct target name
58-58: Unable to verify translation file renaming automatically—manual verification required.The repository clone failed, preventing automated verification of the file rename. Please manually confirm that:
MudletInstaller_en_US.tsexists in the repositoryMudletBootstrap_en_US.tshas been removed or no longer referencedMudletInstaller.cpp (6)
1-1: LGTM!Include header correctly updated to
MudletInstaller.h.
121-150: LGTM!Constructor correctly renamed to
MudletInstaller::MudletInstallerwith proper initialization.
157-233: LGTM!State machine initialization and all signal/slot connections are correctly updated to use
MudletInstaller::scope.
706-790: LGTM!The
installApplication()method is correctly renamed, and the user-facing text on line 774 appropriately referencesMudletInstallerto inform users they can delete the installer after installation.
240-504: LGTM!All remaining method definitions (
start,fetchPlatformFeed,onFetchPlatformFeedFinished,checkExistingFile,startDownload,onDownloadProgress,onDownloadFinished,onDownloadError,verifyHash) are correctly renamed toMudletInstaller::scope.
798-848: LGTM!The
handleError,cleanup, andretryDownloadmethods are correctly renamed toMudletInstaller::scope.
- Fix missed rename in CI/package-mac.sh (MudletBootstrap -> MudletInstaller) - Update header guard to MUDLETINSTALLER_H to match filename - Remove duplicate QTimer include - Fix potential use-after-free by capturing errorString before deleteLater - Remove unused ZIP_FILE_NAME variable in package-win.sh - Remove undefined $pgm variable reference in package-mac.sh
Replace undefined $pgm with $(basename "$0") instead of removing the helpful usage hint entirely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CI/package-mac.sh (1)
116-127: Align DMG cleanup pattern with new DMG namingYou now create the DMG as
${HOME}/Desktop/$(basename "${app%.*}").dmg, which forMudletInstaller.appbecomesMudletInstaller.dmg, and later move it as${appBaseName}-${gameName}.dmg. However, the cleanup still targets~/Desktop/[mM]udletInstaller-${gameName}*.dmg.That pattern will no longer match the DMG you actually create on the Desktop, so stale DMGs could be left behind if a run fails before
mv.Consider updating the cleanup to match the actual Desktop filename, e.g.:
rm -f "${HOME}/Desktop/${appBaseName}.dmg" # or, if you want to be flexible: rm -f "${HOME}/Desktop"/[mM]udletInstaller*.dmgThis keeps the cleanup behavior in sync with the new naming.
Also applies to: 161-197
🧹 Nitpick comments (1)
CI/package-mac.sh (1)
145-145: Tighten shell quoting and optional--verbosehandlingThe macdeployqtfix and appdmg-related lines mostly look good after the rename, but there are a couple of small shell robustness improvements you can make:
- Line 145: Shellcheck (SC2046) is right that the unquoted command substitution could lead to accidental word-splitting if it ever emitted more than one token. A simple, readable pattern is:
python "${GITHUB_WORKSPACE}/macdeployqtfix.py" \ "${app}/Contents/MacOS/MudletInstaller" \ "${QT_DIR}" ${DEBUG:+--verbose}
- Lines 172–179 and 184: Since
${SOURCE_DIR}and${BUILD_DIR}are filesystem paths, it’s slightly safer to quote them to guard against spaces, for example:cp "${SOURCE_DIR}/mudletinstaller-appdmg.json" "${BUILD_DIR}" perl -pi -e "s|../source/build/.*MudletInstaller.*\\.app|${BUILD_DIR}/${app}|i" "${BUILD_DIR}/mudletinstaller-appdmg.json" perl -pi -e "s|../source/src/icons/.*\\.icns|${SOURCE_DIR}/mudlet.icns|i" "${BUILD_DIR}/mudletinstaller-appdmg.json" appdmg "${BUILD_DIR}/mudletinstaller-appdmg.json" "${HOME}/Desktop/$(basename "${app%.*}").dmg"None of this is blocking, but it will quiet shellcheck and make the script more robust to odd paths.
Also applies to: 172-179, 184-185
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CI/package-mac.sh(7 hunks)CI/package-win.sh(2 hunks)MudletInstaller.cpp(19 hunks)MudletInstaller.h(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
MudletInstaller.h (1)
MudletInstaller.cpp (1)
MudletInstaller(120-149)
🪛 Shellcheck (0.11.0)
CI/package-mac.sh
[warning] 145-145: Quote this to prevent word splitting.
(SC2046)
🔇 Additional comments (11)
CI/package-win.sh (2)
105-117: Binary renaming is consistent and complete.All references to the executable—including the existence check, file copy operations, debug file handling, and windeployqt6 call—have been correctly updated from
MudletBootstrap.exetoMudletInstaller.exe. The changes align with the PR objective and maintain proper integration with the packaging workflow.
133-133: Comments updated to match renamed binary.Inline documentation correctly references
MudletInstallerinstead ofMudletBootstrap.Also applies to: 135-135
MudletInstaller.h (1)
1-2: Header guard and class rename are consistent
MUDLETINSTALLER_HandMudletInstaller/constructor signatures line up with the implementation and the rest of the PR; no issues seen.Also applies to: 20-25
MudletInstaller.cpp (8)
1-1: Rename toMudletInstallerand state-machine wiring look consistentThe switch to
#include "MudletInstaller.h", theMudletInstallerconstructor, and theinitStateMachine/startwiring all use the new class name consistently. State transitions reference the correctMudletInstallersignals/slots, and the ownership (new QStateMachine(this)) remains intact. No behavioral regressions spotted here.Also applies to: 120-149, 156-202, 239-241
247-271: Platform feed fetching matches helper utilitiesThe
MudletInstaller::fetchPlatformFeed()implementation correctly reusesQSettings,getPlatformFeedMap(), anddetectOS()to derive the feed URL, and hooksQNetworkReply::finishedtoMudletInstaller::onFetchPlatformFeedFinished. The error handling anderrorOccurredemit remain the same, and progress UI is shown as before. Looks good.
345-362: Download start/progress/completion logic unchanged and coherent
checkExistingFile(),startDownload(),onDownloadProgress(), andonDownloadFinished()all appear behaviorally unchanged aside from references toMudletInstaller. The resume logic viaRangeheader andbytesAlreadyDownloaded, progress updates, file append semantics, and reset ofretryCount/bytesAlreadyDownloadedon success are consistent and reasonable. No new issues noticed.Also applies to: 369-405, 413-444
446-484: Improved error handling avoids use-after-free and distinguishes retryable errorsThe updated
MudletInstaller::onDownloadErrornow:
- Captures
errorStringbefore callingcurrentReply->deleteLater(), avoiding any risk of dereferencing a deletedQNetworkReply.- Classifies errors into retryable vs. non-retryable via a
switchonQNetworkReply::NetworkError.- Sets
retryCount = MAX_RETRIESfor non-retryable errors so theretryDownloadstate immediately gives up and transitions to the error state, while still updating the status label with a useful error message.This is a solid improvement in robustness and diagnostics.
492-505: Hash verification wiring and status updates look good
MudletInstaller::verifyHash()is correctly tied into the new class name, updates the status label, and usesverifyFileSha256()to emithashValidorhashInvalidaccordingly. No issues from the rename or behavior standpoint.
707-791: Install flow and updated completion message are consistent with new naming
MudletInstaller::installApplication()still:
- Propagates
MUDLET_PROFILESfromlaunch.iniinto the environment.- Creates the autologin file with the same
QDataStreamversioning logic.- Dispatches to
installAndRunExe/installAndRunDmg/installAndRunAppImagedepending on the platform.- Emits
errorOccurredorinstallCompletevia the state machine as before.The updated completion text:
"%1 has been installed! You can now delete the MudletInstaller-%1 app."matches the new MudletInstaller branding and is consistent with the PR’s objective. The optional Windows shortcut messaging also still makes sense.
799-805: Error, cleanup, and retry states remain coherent
handleError(),cleanup(), andretryDownload()are correctly renamed toMudletInstallermethods and keep the same semantics:
handleError()logs, sets a generic error, and emitsfinished.cleanup()conditionally deletes the downloaded file and closes the progress window.retryDownload()respectsMAX_RETRIES, updates the status label, and uses a shortQTimerdelay before re-emittingfileNotExiststo re-enter the download state; after max retries, it emitserrorOccurred.All of this ties cleanly into the revised state machine and looks correct.
Also applies to: 811-821, 828-849
277-338: I'll verify the claims in this review comment about macOS path handling in MudletInstaller.cpp. Let me start by examining the code and checking Qt's documentation on path handling.
<function_calls>
shell
First, find and examine the MudletInstaller.cpp file
find . -name "MudletInstaller.cpp" -type f | head -5
</function_calls>
<function_calls>
shell
Get the line count of MudletInstaller.cpp to understand its size
wc -l MudletInstaller.cpp
</function_calls>
<function_calls>
shell
Extract the specific function around lines 277-338
sed -n '270,345p' MudletInstaller.cpp
</function_calls>
<function_calls>
shell
Search for other usages of QStandardPaths in the codebase
rg -n "QStandardPaths" MudletInstaller.cpp
</function_calls>
<function_calls>
shell
Check if QDir is used elsewhere in the file for path operations
rg -n "QDir" MudletInstaller.cpp -A 2 -B 2
</function_calls>Now let me search for Qt documentation on path handling:
<function_calls>
Qt QStandardPaths writableLocation trailing separator macOS
</function_calls>
<function_calls>
Qt QDir filePath path concatenation best practice
</function_calls>budget:token_budget200000</budget:token_budget>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
CI/package-mac.sh (1)
146-146: Quote the command substitution to prevent word splitting.Line 146 has an unquoted command substitution that could cause issues if the output contains spaces (unlikely for
--verbosebut violates best practices).Apply this diff:
- python ${GITHUB_WORKSPACE}/macdeployqtfix.py "${app}/Contents/MacOS/MudletInstaller" "${QT_DIR}" $( [ -n "$DEBUG" ] && echo "--verbose" ) + python ${GITHUB_WORKSPACE}/macdeployqtfix.py "${app}/Contents/MacOS/MudletInstaller" "${QT_DIR}" $( [ -n "$DEBUG" ] && echo "--verbose" || true )Alternatively, for clearer intent:
- python ${GITHUB_WORKSPACE}/macdeployqtfix.py "${app}/Contents/MacOS/MudletInstaller" "${QT_DIR}" $( [ -n "$DEBUG" ] && echo "--verbose" ) + python "${GITHUB_WORKSPACE}/macdeployqtfix.py" "${app}/Contents/MacOS/MudletInstaller" "${QT_DIR}" $([ -n "$DEBUG" ] && echo "--verbose" || true)This also quotes the python executable path for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
CI/package-mac.sh(7 hunks)
🧰 Additional context used
🪛 Shellcheck (0.11.0)
CI/package-mac.sh
[warning] 146-146: Quote this to prevent word splitting.
(SC2046)
🔇 Additional comments (1)
CI/package-mac.sh (1)
116-116: Rename consistency across script looks good.The rename from MudletBootstrap to MudletInstaller is applied consistently throughout: app base name (line 116), error messages (lines 125–126), plist entries (lines 149–150), desktop cleanup (line 164), config file references (lines 173–178), and release naming (line 198). The textual substitutions align with the PR's rename objective, and no functional logic changes were introduced.
Also applies to: 125-125, 126-126, 149-149, 150-150, 164-164, 176-176, 178-178, 185-185, 198-198
Rename binary, class, and all user-facing text from MudletBootstrap to MudletInstaller for a more intuitive Firefox-like naming convention.